Re: [PATCH v5 01/14] xen/riscv: detect and store supported hypervisor CSR bits at boot

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/24/26 12:32 PM, Jan Beulich wrote:
> On 24.02.2026 12:25, Oleksii Kurochko wrote:
> > On 2/24/26 11:47 AM, Jan Beulich wrote:
> > > On 24.02.2026 11:42, Oleksii Kurochko wrote:
> > > > On 2/24/26 11:16 AM, Jan Beulich wrote:
> > > > > On 24.02.2026 10:44, Oleksii Kurochko wrote:
> > > > > > On 2/24/26 9:07 AM, Jan Beulich wrote:
> > > > > > > On 20.02.2026 17:18, Oleksii Kurochko wrote:
> > > > > > > > --- a/xen/arch/riscv/domain.c
> > > > > > > > +++ b/xen/arch/riscv/domain.c
> > > > > > > > @@ -2,9 +2,39 @@
> > > > > > > >      
> > > > > > > >      #include <xen/init.h>
> > > > > > > >      #include <xen/mm.h>
> > > > > > > > +#include <xen/sections.h>
> > > > > > > >      #include <xen/sched.h>
> > > > > > > >      #include <xen/vmap.h>
> > > > > > > >      
> > > > > > > > +#include <asm/cpufeature.h>
> > > > > > > > +#include <asm/csr.h>
> > > > > > > > +
> > > > > > > > +struct csr_masks {
> > > > > > > > +    register_t hedeleg;
> > > > > > > > +    register_t henvcfg;
> > > > > > > > +    register_t hideleg;
> > > > > > > > +    register_t hstateen0;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static struct csr_masks __ro_after_init csr_masks;
> > > > > > > > +
> > > > > > > > +void __init init_csr_masks(void)
> > > > > > > > +{
> > > > > > > > +#define INIT_CSR_MASK(csr, field) do { \
> > > > > > > > +    register_t old; \
> > > > > > > > +    old = csr_read(CSR_##csr); \
> > > > > > > Can't this be the initializer of the variable? Can't ...
> > > > > > I agree that this is change is okay to be done but I am not sure about ...
> > > > > >
> > > > > > > > +    csr_set(CSR_##csr, ULONG_MAX); \
> > > > > > > ... csr_swap() be used here, too?
> > > > > > ... as after re-reading spec again I am not sure that we can do in this way
> > > > > > at all.
> > > > > >
> > > > > > Initially I used csr_set() instead of csr_swap() or csr_write() as csr_set() to
> > > > > > take into account a writability of the bit, so it won't touch a bit if it
> > > > > > is r/o.
> > > > > To me the spec isn't quite clear enough in this regard.
> > > > >
> > > > > > But it seems like this approach won't work with**WLRL or WPRI fields as these
> > > > > > fields aren't r/o,
> > > > > In the CSRs presently dealt with, are there any WLRL fields at all? (I don't
> > > > > think WPRI fields represent much of an issue for the purpose here.)
> > > > Agree, currently used CSRs in this function don't have WLRL feilds, but I 
> > > > suppose
> > > > we want to have the similar treatment (read WLRL fields and reuse what was read)
> > > > for WLRL fields as these fields expect only valid value and so all 1s for this
> > > > fields can be wrong to use.
> > > >
> > > > > > but they only support specific value and for example:
> > > > > >
> > > > > > - Implementations are permitted but not required to raise an
> > > > > >       illegal-instruction exception if an instruction attempts to write a
> > > > > >       non-supported value to a WLRL field.
> > > > > > - For these reserved fields, software is required to preserve the existing
> > > > > >       values when writing to other fields in the same register. Overwriting them
> > > > > >       with 1s could be considered non-compliant behavior.
> > > > > >
> > > > > > So it seems like we can't just do csr_swap() with all 1s and it is needed
> > > > > > to pass a mask to INIT_CSR_MASK() which will tell which bits should be
> > > > > > ignored and don't touched.
> > > > > > For example, HENVCFG and HSTATEEN0 has WPRI fields.
> > > > > >
> > > > > > reserved_bits_mask = 0x1FFFFFFCFFFFFF00;
> > > > > > tmp = csr_read(HENVCFG);
> > > > > > tmp=(~reserved_bits_mask) |(tmp&reserved_bits_mask); csr_set(HENVCFG, tmp);
> > > > > What I find further concerning is that HSTATEEN0 also may have read-only-1
> > > > > fields. You don't currently cope with that, I think.
> > > > Because of this:
> > > >      A bit in an hstateen CSR cannot be read-only one unless the same bit is
> > > >      read-only one in the matching mstateen CSR.
> > > > ?
> > > >
> > > > I expect that it will be covered by csr_set() which will touch only writable
> > > > bits and ignore read-only. So doesn't matter if a bit is read only 1 or 0
> > > > it still shouldn't be in the final mask.
> > > But the hypervisor view of the register value seen by guests won't be correct.
> > > Recall that you moved to probing to make sure that the cached values we have
> > > in the hypervisor properly match the values seen by the guest?
> > Then we have to store what csr_read(hstateen0) returns in struct csr_masks in
> > new field hstateen0_ro_ones. And then in the next patch apply that new field
> > in vcpu_csr_init():
> >     v->arch.hstateen0 = hstateen0 & csr_masks.hstateen0 |
> >                         csr_masks.hstateen0_ro_ones;
> >
> > Are you okay with such changes?
> Properly structured, sure. That's pretty much unavoidable, isn't it?
>
> As to "structured", for example I wonder whether hstateen0_ro_ones isn't
> going to lead to redundancies once further registers appear which may have
> r/o-1 fields. Maybe there should be "ro_one" sub-struct right away?
Maybe, it makes sense. I will then update the csr_masks structure in the
following way:

struct csr_masks {
    register_t hedeleg;
    register_t henvcfg;
    register_t hideleg;
    register_t hstateen0;
    
    struct {
        hstateen0;
    } ro_one;
};

> And of course "structured" also touches on proper parenthesization of the
> statement above.
According to https://en.cppreference.com/w/c/language/operator_precedence.html
& has bigger priority, so I haven't put parenthesizes.

~ Oleksii