Re: [PATCH v3 22/39] xen/riscv: look up the exception table for any trap taken in Xen context

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> do_trap() consulted the exception table only for CAUSE_ILLEGAL_INSTRUCTION,
> which covers csr_read_safe() but not the hlv/hlvx sequences reading guest
> memory: those fault with load/store (guest) page fault causes and would
> reach do_unexpected_trap() instead of their fixup.
> 
> Move the lookup ahead of the cause switch, and gate it on the trap having
> been taken in Xen context and not being an interrupt:
> 

This reads a bit confusingly: the colon makes the bullets look like the
conditions under which the lookup is done, yet the first one starts with
"a trap taken from the guest". Maybe state up front that the bullets are
the excluded cases, e.g.:
```
  Move the lookup ahead of the cause switch, and only do it for exceptions
  taken in Xen context. Both other cases must skip it:

  - Traps taken from the guest: sepc is then a guest address, which the
    guest can make equal to an address listed in the exception table. ...

  - Interrupts: an interrupt taken at an address listed in the table would
    be "fixed up" as if the access itself had faulted, ...
```
> [ ... 40 lines skipped ... ]
> @@ -196,11 +196,34 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>      unsigned long cause = csr_read(CSR_SCAUSE);
>      bool from_guest = cpu_regs->hstatus & HSTATUS_SPV;
>  
> +    /*
> +     * A synchronous trap taken in Xen context may come from an access done 
> on
> +     * a vCPU's behalf, e.g. the hlv/hlvx sequences in read_guest(),
> +     * or from a probing access like csr_read_safe(). Both are covered by
> +     * exception table entries which record the fault details for the caller
> +     * and resume execution past the faulting instruction.
> +     *
> +     * Traps taken from the guest must never be fixed up: sepc is then a 
> guest
> +     * address, which the guest could point at an address listed in the
> +     * exception table, making Xen act on an entry (and, for 
> EX_TYPE_TRAP_INFO,
> +     * write through a pointer) fully under guest control.
> +     *
> +     * Interrupts must be excluded too: one taken at an address which happens
> +     * to be listed in the exception table would otherwise be "fixed up" as 
> if
> +     * the access itself had faulted, silently skipping it.
> +     *
> +     * Returning early skips check_for_pcpu_work() below, which is correct:
> +     * that only runs for traps taken from the guest.
> +     */
> +    if ( !from_guest && !(cause & CAUSE_IRQ_FLAG) &&
> +         fixup_exception(cpu_regs, cause) )
> +        return;
> +
>      switch ( cause )
>      {
>      case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
>          /* CAUSE_VIRTUAL_SUPERVISOR_ECALL should come from VS-mode */
> -        BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));
> +        BUG_ON(!from_guest);
Does this hunk really belong to this patch? It seems it belongs to
previous patch where you introduced the local `from_guest`.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>