Re: [PATCH v2 13/39] xen/riscv: save and restore AIA state on vCPU context switch

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/4/26 11:52 AM, Baptiste Le Duc wrote:
>> vsiselect and hviprio{1,2} are per-hart CSRs which a guest can change, so
>> they have to be part of the vCPU context:
> Where in the spec did you see that? Because in AIA spec section 6.3.1,
> it is written that "When vsiselect has a value in the range 0x30-0x3F,
> an attempt from VS-mode to access sireg (really vsireg) causes a virtual
> instruction exception" and this even when hstateen0.CSRIND is set as hstateen0
> just control whether a guest/S-mode is allowed to access a CSR (exactly
> as you described below).

Your understanding is correct, it was me who confused the things. Sorry 
for that.

> 
> Therefore, the hypervisor has two options to modify the priority of a
> major irq:
> - emulate the iprio array in software.
> - Use hviprio1/hviprio2 (only 10 irqs configurable).
> 
> But the guest shouldn't be able to modify h CSRs at all, in any case, or
> I may have misunderstood a part of the spec.
> 
> For the moment I don't see any catch of possible instruction exception
> in do_trap().

There is no such because we don't emulate range 0x30-0x3F. We don't have 
such use cases now.

I think that I have to recheck what should be saved/restored now.

There is no need to save/restore CSR_HVIPRIO* during context switch as 
we don't have support of handling of 0x30-0x3f. I will introduce that 
later when we really will need that.

VSISELECT should be save/restored then only in this patch as we have 
hstateen0.SMSTATEEN0_SVSLCT set so guest could change VSISELECT directly 
so we need to store/restore.

Am I missing something?

With having only VSISELECT saved/restored in this patch I think the 
commit message should be:

xen/riscv: save and restore vsiselect on vCPU context switch

vsiselect is a per-hart CSR which a guest changes on its own: when V=1,
VS-mode accesses to siselect are really accesses to vsiselect.
Architecturally a vCPU has to find there the value it last wrote, but as
long as the CSR isn't part of the vCPU context it finds whatever selector
the vCPU which ran on the hart before it left behind. A guest which writes
siselect, is descheduled and then reads sireg without rewriting siselect
therefore reaches a register it never selected, and it can also observe
another guest's selector value.

When Smstateen is implemented, access to vsiselect and vsireg is gated by
hstateen0.CSRIND (bit 60, SMSTATEEN0_SVSLCT in Xen's headers), and
v->arch.hstateen0 holds the bits vcpu_csr_init() ended up with. A clear
bit there covers the two cases in which the CSR has to be skipped:

- Xen didn't hand the guest access to it, so the guest can't have changed
      the CSR and there is no state to preserve;
- M-mode denied the state altogether. Smstateen makes a bit which is zero
      in mstateen0 read-only zero in hstateen0, and a zero bit in mstateen0
      traps accesses from every privilege mode less privileged than M-mode,
      HS-mode included, so Xen couldn't even read the CSR to save it.

Without Smstateen no bit controls access to the CSR, so it is saved and
restored whenever Ssaia is available.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
- Update the commit message.
- Save and restore only VSISELECT.
---
Changes in v2:
- New  patch.
---

diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 0ad851ee0f5f..1085ef152b8b 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -327,6 +327,28 @@ int arch_domain_create(struct domain *d,
      return rc;
  }

+/*
+ * vsiselect is a per-hart CSR, but a guest changes it on its own: when 
V=1,
+ * VS-mode accesses to siselect are really accesses to vsiselect. Hence 
it is
+ * part of the vCPU context.
+ *
+ * When Smstateen is implemented, hstateen0.CSRIND (SMSTATEEN0_SVSLCT) 
gates
+ * that access, and a bit staying clear in v->arch.hstateen0 (see
+ * vcpu_csr_init()) means either that the guest was never given access 
to the
+ * CSR, and so can't have changed it, or that M-mode denied the state
+ * altogether, in which case the CSR can't be accessed from HS-mode either.
+ */
+static bool vcpu_can_access_vsiselect(const struct vcpu *v)
+{
+    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
+        return false;
+
+    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
+        return true;
+
+    return v->arch.hstateen0 & SMSTATEEN0_SVSLCT;
+}
+
  static void save_csr_regs(struct vcpu *p)
  {
      /*
@@ -354,6 +376,9 @@ static void save_csr_regs(struct vcpu *p)
      p->arch.vscause = csr_read(CSR_VSCAUSE);
      p->arch.vstval = csr_read(CSR_VSTVAL);
      p->arch.vsepc = csr_read(CSR_VSEPC);
+
+    if ( vcpu_can_access_vsiselect(p) )
+        p->arch.vsiselect = csr_read(CSR_VSISELECT);
  }

  static void restore_csr_regs(struct vcpu *n)
@@ -375,6 +400,9 @@ static void restore_csr_regs(struct vcpu *n)
      csr_write(CSR_VSCAUSE, n->arch.vscause);
      csr_write(CSR_VSTVAL, n->arch.vstval);
      csr_write(CSR_VSEPC, n->arch.vsepc);
+
+    if ( vcpu_can_access_vsiselect(n) )
+        csr_write(CSR_VSISELECT, n->arch.vsiselect);
  }

  static void ctxt_switch_from(struct vcpu *p)
diff --git a/xen/arch/riscv/include/asm/domain.h 
b/xen/arch/riscv/include/asm/domain.h
index 58d1e8076876..b0824d7f9add 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -75,6 +75,7 @@ struct arch_vcpu {
      register_t vscause;
      register_t vsepc;
      uint64_t   vsie;
+    register_t vsiselect;
      register_t vsscratch;
      register_t vsstatus;
      register_t vstval;


Does it make sense to you?



> 
>>   - vsiselect is written directly by VS-mode through siselect;
>>   - hviprio1 and hviprio2 hold the priorities of the local interrupts which
>>     VS-mode reaches through the iprio array of vsiselect/vsireg, so writes
>>     the guest performs there land in these CSRs.
> hviprio1 and hviprio2 hold priorities for interrupts 1 (SSI), 5 (STI),
> 13 (counter overflow), and 14-23 (local) so calling all of them "local"
> is wrong.

Agree, it is incorrect to call them "local"

>> Without saving them, one vCPU's selector leaks into another vCPU's vsireg
>> accesses and one guest's interrupt priorities apply to the next guest which
>> runs on the same hart.
>>
>> Whether the CSRs may be touched at all is gated by hstateen0 when Smstateen
>> is implemented: SVSLCT for vsiselect/vsireg and AIA for the rest of the AIA
> I couldn't find any reference to SVSLCT in the spec. I assume you wanted
> to refer to CSRIND and SVSLCT is an OpenSBI's own nickname.
> 

Indeed, SVSLCT is the OpenSBI nickname/macro definition for this feature 
and CSRIND would be better to use in commit message.

Thanks.

~ Oleksii