Re: [PATCH 4/7] target/riscv/kvm: add kvm_csr_cfgs[]

[Andrew Jones <ajones@ventanamicro.com>]

On Wed, Apr 23, 2025 at 04:45:29PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 4/23/25 12:15 PM, Andrew Jones wrote:
...
> >   if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint32_t)) {
> >       kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
> >   } else if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint64_t)) {
> >       kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
> >   } else {
> >       uh, oh...
> >   }
> 
> The idea with this logic is to handle the cases where there is a mismatch
> between the size of the KVM reg and the size of the flag we're using to
> store it. All CSR regs are of size target_ulong but not all CPURISCVState
> flags we're using are target_ulong. We're not storing the size of the KVM
> CSR, we're storing the flag size.
> 
> E.g:
> 
> KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),
> 
> For a 32 bit CPU we're writing a 32 bit ulong sstatus into the 64 bit mstatus
> field. If we assume that they're the same size we'll be read/writing a 32 bit val
> inside a 64 bit field, i.e. we'll be reading/writing the upper words only.

Hmm. I don't think this should be a problem since we're writing the field
offset, which is the low word. A problem may be that we don't clear the
upper word with the write, but it should have been initialized to zero and
never touched.

> 
> In theory this would be ok if we always read/write the same way but it can be
> a nuisance when trying to debug the value inside CPURISCVState.
> 
> One may argue whether we should change the type of these fields to match what
> Linux/ISA expect of them. Not sure how much work that is.

It shouldn't be a problem to use wider fields, we just need to ensure our
framework will be able to write the upper words when necessary, i.e. when
there's an *H counterpart CSR that needs to be written there or when the
upper word should be zeroed out.

> 
> 
> scounteren is a more serious case because we're writing an ulong KVM reg into
> a 32 bit field:

Sigh... it is a problem to use narrower fields... KVM should have defined
that as a __u32.

> 
>     KVM_CSR_CFG("scounteren", scounteren, sizeof(uint32_t),
>                 RISCV_CSR_REG(scounteren)),
> 
> struct CPUArchState {
>     target_ulong gpr[32];
>     (...)
>     uint32_t scounteren;
>     uint32_t mcounteren;
>     (...)
> 
> 
> Perhaps it's a good idea to change this CPURISCVState field to ulong before
> adding support for the scouteren KVM CSR.

Yes, I think we have to now. QEMU's fields need to have widths >= what
KVM says the register sizes are and we should write the amount of bytes
that KVM says we should write (even if we know it's wrong, like in the
case of scounteren). We'll just have to ignore the upper word that
shouldn't be there.

Thanks,
drew