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?
And of course "structured" also touches on proper parenthesization of the
statement above.
Jan
