Re: [PATCH v1 03/15] xen/riscv: implement vcpu_csr_init()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/12/26 4:54 PM, Jan Beulich wrote:
> On 12.01.2026 16:46, Oleksii Kurochko wrote:
> > On 1/12/26 3:28 PM, Jan Beulich wrote:
> > > On 12.01.2026 13:59, Oleksii Kurochko wrote:
> > > > On 1/7/26 9:46 AM, Jan Beulich wrote:
> > > > > Also, wouldn't you better keep internal state in line with what hardware
> > > > > actually supports? CSRIND may be read-only-zero in the real register, in
> > > > > which case having the bit set in the "cached" copy can be misleading.
> > > > [...]
> > > >
> > > > > (This may similarly apply to at least hedeleg and hideleg, btw.)
> > > > Regarding the previous bits, I can understand that it would be an issue:
> > > > if SSAIA isn’t supported, then it is incorrect to update the corresponding
> > > > bits of|hstateen0|.
> > > >
> > > > However, I’m not really sure I understand what the issue is with|h{i,e}deleg|.
> > > > All writable bits there don’t depend on hardware support. Am I missing 
> > > > something?
> > > My reading of the doc was that any of the bits can be r/o 0, with - yes -
> > > no dependencies on particular extensions.
> > Just to be sure that I get your idea correctly.
> >
> > Based on the priv. spec:
> >     Each bit of hedeleg shall be either writable or read-only zero. Many bits of
> >     hedeleg are required specifically to be writable or zero, as enumerated in
> >     Table 29.
> >
> > Now let’s take hedeleg.bit1, which is marked as writable according to Table 29.
> > Your point is that even though hedeleg.bit1 is defined as writable, it could 
> > still
> > be read-only zero, right?
> >
> > In general, I agree with that. It is possible that M-mode software decides, for
> > some reason (for example, because the implementation does not support delegation
> > of bit1 to a lower mode), not to delegate medeleg.bit1 to HS-mode. In that case,
> > hedeleg.bit1 would always be read-only zero.
> >
> > >    In which case you'd need to do
> > > the delegation in software. For which it might be helpful to know what
> > > the two registers are actually set to in hardware (i.e. the cached values
> > > wanting to match the real ones).
> > Does it make sense then to have the following
> >         ...
> >         v->arch.hedeleg = hedeleg;
> >         vcpu->arch.hedeleg = csr_read(CSR_HEDELEG);
> > in arch_vcpu_create()?
> The above makes no sense to me, with or without s/vcpu/v/.
Right...

It should be also csr_write() before csr_read():
 csr_write(CSR_HEDELEG, hedeleg);
 v->arch.hedeleg = csr_read(CSR_HEDELEG);

> > Or I can just add the comment that it will be sync-ed with the corresponding
> > hardware CSR later as ,actually, there is some h{i,e}deleg synchronization
> > happening during context_switch() (this code is at the moment in downstream),
> > because restore_csr_regs() is executed and re-reads CSR_H{I,E}DELEG:
> >     static void restore_csr_regs(struct vcpu *vcpu)
> >     {
> >         csr_write(CSR_HEDELEG, vcpu->arch.hedeleg);
> >         csr_write(CSR_HIDELEG, vcpu->arch.hideleg);
> >         ...
> > As a result, vcpu->arch.h{I,E}deleg is kept in sync with the corresponding
> > hardware CSR.
> No, the r/o bits will continue to be out-of-sync between the hw register and
> the struct arch_vcpu field.
Yes, it would be out-of-sync until|save_csr_regs()| is called, where
|csr_read(CSR_HEDELEG)| is executed. So the value remains out-of-sync until a
trap to the hypervisor occurs and a vCPU context switch happens, which triggers
|save_csr_regs()|.
So that’s not an option. The best choice is the one mentioned above.

~ Oleksii