On Wed, Aug 9, 2023 at 6:17 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
> Drew,
>
> On 8/3/23 09:05, Andrew Jones wrote:
> > On Thu, Aug 03, 2023 at 08:36:57AM -0300, Daniel Henrique Barboza wrote:
> >>
> >>
> >> On 8/3/23 06:29, Andrew Jones wrote:
> >>> On Wed, Aug 02, 2023 at 03:00:58PM -0300, Daniel Henrique Barboza wrote:
> >>>> cpu->cfg.mvendorid is a 32 bit field and kvm_set_one_reg() always write
> >>>> a target_ulong val, i.e. a 64 bit field in a 64 bit host.
> >>>>
> >>>> Given that we're passing a pointer to the mvendorid field, the reg is
> >>>> reading 64 bits starting from mvendorid and going 32 bits in the next
> >>>> field, marchid. Here's an example:
> >>>>
> >>>> $ ./qemu-system-riscv64 -machine virt,accel=kvm -m 2G -smp 1 \
> >>>> -cpu rv64,marchid=0xab,mvendorid=0xcd,mimpid=0xef(...)
> >>>>
> >>>> (inside the guest)
> >>>> # cat /proc/cpuinfo
> >>>> processor : 0
> >>>> hart : 0
> >>>> isa : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> >>>> mmu : sv57
> >>>> mvendorid : 0xab000000cd
> >>>> marchid : 0xab
> >>>> mimpid : 0xef
> >>>>
> >>>> 'mvendorid' was written as a combination of 0xab (the value from the
> >>>> adjacent field, marchid) and its intended value 0xcd.
> >>>>
> >>>> Fix it by assigning cpu->cfg.mvendorid to a target_ulong var 'reg' and
> >>>> use it as input for kvm_set_one_reg(). Here's the result with this patch
> >>>> applied and using the same QEMU command line:
> >>>>
> >>>> # cat /proc/cpuinfo
> >>>> processor : 0
> >>>> hart : 0
> >>>> isa : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> >>>> mmu : sv57
> >>>> mvendorid : 0xcd
> >>>> marchid : 0xab
> >>>> mimpid : 0xef
> >>>>
> >>>> This bug affects only the generic (rv64) CPUs when running with KVM in a
> >>>> 64 bit env since the 'host' CPU does not allow the machine IDs to be
> >>>> changed via command line.
> >>>>
> >>>> Fixes: 1fb5a622f7 ("target/riscv: handle mvendorid/marchid/mimpid for
> >>>> KVM CPUs")
> >>>> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> >>>> ---
> >>>> target/riscv/kvm.c | 9 ++++++++-
> >>>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> >>>> index 9d8a8982f9..b1fd2233c0 100644
> >>>> --- a/target/riscv/kvm.c
> >>>> +++ b/target/riscv/kvm.c
> >>>> @@ -852,12 +852,19 @@ void kvm_arch_init_irq_routing(KVMState *s)
> >>>> static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
> >>>> {
> >>>> CPURISCVState *env = &cpu->env;
> >>>> + target_ulong reg;
> >>>
> >>> We can use the type of cfg since KVM just gets an address and uses the
> >>> KVM register type to determine the size. So here,
> >>>
> >>> uint32_t reg = cpu->cfg.mvendorid;
> >>>
> >>> and then...
> >>>
> >>>> uint64_t id;
> >>>> int ret;
> >>>> id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
> >>>> KVM_REG_RISCV_CONFIG_REG(mvendorid));
> >>>> - ret = kvm_set_one_reg(cs, id, &cpu->cfg.mvendorid);
> >>>> + /*
> >>>> + * cfg.mvendorid is an uint32 but a target_ulong will
> >>>> + * be written. Assign it to a target_ulong var to avoid
> >>>> + * writing pieces of other cpu->cfg fields in the reg.
> >>>> + */
> >>>
> >>> ...we don't need this comment since we're not doing anything special.
> >>
> >> I tried it out and it doesn't seem to work. Here's the result:
> >>
> >> / # cat /proc/cpuinfo
> >> processor : 0
> >> hart : 0
> >> isa : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> >> mmu : sv57
> >> mvendorid : 0xaaaaaa000000cd
> >> marchid : 0xab
> >> mimpid : 0xef
> >>
> >>
> >> The issue here is that the kernel considers 'mvendorid' as an unsigned
> >> long (or
> >> what QEMU calls target_ulong). kvm_set_one_reg() will write an unsigned
> >> long
> >> regardless of the uint32_t typing of 'reg', meaning that it'll end up
> >> writing
> >> 32 bits of uninitialized stuff from the stack.
> >
> > Indeed, I managed to reverse the problem in my head. We need to to worry
> > about KVM's notion of the type, not QEMU's. I feel like we still need some
> > sort of helper, but one that takes the type of the KVM register into
> > account. KVM_REG_RISCV_CONFIG registers are KVM ulongs, which may be
> > different than QEMU's ulongs, if we ever supported 32-bit userspace on
> > 64-bit kernels. Also, not all KVM register types are ulong, some are
> > explicitly u32s and others u64s. I see kvm_riscv_reg_id() is used to try
> > and get the right KVM reg size set, but it's broken for RISCV_FP_F_REG(),
> > since those are all u32s, even when KVM has 64-bit ulong (I guess nobody
> > is testing get/set-one-reg with F registers using that helper, otherwise
> > we'd be getting EINVAL from KVM). And KVM_REG_RISCV_FP_D_REG(fcsr) is also
> > broken and RISCV_TIMER_REG() looks broken with 32-bit KVM, since it should
> > always be u64. I guess all that stuff needs an audit.
> >
> > So, I think we need a helper that has a switch on the KVM register type
> > and provides the right sized buffer for each case.
>
> Is this a suggestion to do this right now in this patch? I didn't understand
> whether you're ok with the fix as is for 8.1 or if you want more things done
> right away.
Do you want this in for 8.1? It might be a bit late. It helps a lot if
you put in the title [PATCH 8.1]
Alistair
>
>
> Thanks,
>
> Daniel
>
> >
> > Thanks,
> > drew
> >
> >
> >>
> >> target_ulong seems that the right choice here. We could perhaps work with
> >> uint64_t (other parts of the code does that) but target_ulong is nicer with
> >> 32-bit setups.
> >>
> >>
> >> Thanks,
> >>
> >> Daniel
> >>
> >>>
> >>>> + reg = cpu->cfg.mvendorid;
> >>>> + ret = kvm_set_one_reg(cs, id, ®);
> >>>> if (ret != 0) {
> >>>> return ret;
> >>>> }
> >>>> --
> >>>> 2.41.0
> >>>>
> >>>
> >>> We should audit and fix all uses of &cpu->cfg.* with KVM ioctls. We can
> >>> also consider introducing wrappers like
> >>>
> >>> #define kvm_set_one_reg_safe(cs, id, addr) \
> >>> ({ \
> >>> typeof(*(addr)) _addr = *(addr); \
> >>> kvm_set_one_reg(cs, id, &_addr) \
> >>> })
> >>>
> >>> Thanks,
> >>> drew
>
