Re: [PATCH v2 04/39] xen/riscv: introduce csr_read64()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/27/26 5:36 PM, Andrew Cooper wrote:
> On 27/08/2026 4:20 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/riscv/include/asm/csr.h b/xen/arch/riscv/include/asm/csr.h
> > index 888d6a2a86d6..a5cdd6f99c8e 100644
> > --- a/xen/arch/riscv/include/asm/csr.h
> > +++ b/xen/arch/riscv/include/asm/csr.h
> > @@ -39,12 +39,36 @@
> >       csr_write(csr, v_);             \
> >       csr_write(csr ## H, v_ >> 32);  \
> >   })
> > +
> > +/*
> > + * The two halves are read by separate instructions, so a CSR which hardware
> > + * increments can carry from the low half into the high one in between,
> > + * yielding a value the CSR never held. Re-read the high half and retry the
> > + * sequence if it changed.
> > + */
> > +#define csr_read64(csr)                         \
> > +({                                              \
> > +    uint32_t hi_, lo_;                          \
> > +                                                \
> > +    do {                                        \
> > +        hi_ = csr_read(csr ## H);               \
> > +        lo_ = csr_read(csr);                    \
> > +    } while ( hi_ != csr_read(csr ## H) );      \
> > +                                                \
> > +    ((uint64_t)hi_ << 32) | lo_;                \
> > +})
> This double reads H in the looping case.  You want something more like:
>
> hi = csr_read();
> do {
>      old = hi;
>      lo = csr_read();
> } while ( (hi = csr_read()) != old );
Good point. I'll apply that.

> Still, this only matters for volatile CSRs, and is unnecessary in the
> general case.  I'd suggest naming it csr_volatile_read64(). 
Yes, that makes sense. I will rename it to csr_volatile_read64().

> Most CSRs
> can use a simple split access.
I may have misunderstood you here, but wouldn't it still make sense to 
have a macro covering the case where a register is 64-bit on RV32 yet 
accessed through two CSRs? VSIE and VSIEH, for example.
My plan was to use a single csr_read64() (but while loop then really 
isn't needed in this case) call to abstract the access to VSIE, so that 
the code looks the same on RV32 and RV64.
Does that make sense, or would it be better to have separate vsie and 
vsieh fields instead?
~ Oleksii