RE: [EXTERNAL] Re: [PATCH] WHPX: Use QEMU values for trapped CPUID

[Sunil Muthuswamy]

> -----Original Message-----
> From: Eduardo Habkost <address@hidden>
> Sent: Thursday, February 27, 2020 1:10 PM
> To: Sunil Muthuswamy <address@hidden>
> Cc: Paolo Bonzini <address@hidden>; Richard Henderson <address@hidden>; 
> address@hidden; Stefan Weil
> <address@hidden>
> Subject: [EXTERNAL] Re: [PATCH] WHPX: Use QEMU values for trapped CPUID
> 
> On Thu, Feb 27, 2020 at 09:01:04PM +0000, Sunil Muthuswamy wrote:
> > Currently, WHPX is using some default values for the trapped CPUID
> > functions. These were not in sync with the QEMU values because the
> > CPUID values were never set with WHPX during VCPU initialization.
> > Additionally, at the moment, WHPX doesn't support setting CPUID
> > values in the hypervisor at runtime (i.e. after the partition has
> > been setup). That is needed to be able to set the CPUID values in
> > the hypervisor during VCPU init.
> > Until that support comes, use the QEMU values for the trapped CPUIDs.
> >
> > Signed-off-by: Sunil Muthuswamy <address@hidden>
> 
> I like the change, but I wonder if any if your users would still
> prefer to use the default result chosen by WHPX instead of the
> ones chosen by QEMU.
> 

Note that the current patch only applies to the trapped CPUIDs, which for
WHPX are currently only {1, 0x80000001}. WHPX will still provide most
of the values.

> On the KVM side I have always wondered if we should have a mode
> where all CPUID leaves are the ones chosen by KVM, making no
> KVM_SET_CPUID calls.  It would be useful for experimentation and
> debugging of KVM/QEMU defaults.
> 
Agreed. I think such an option could be useful debugging tool.

> 
> > ---
> >  target/i386/whpx-all.c | 42 ++++++++++++++++++------------------------
> >  1 file changed, 18 insertions(+), 24 deletions(-)
> >
> > diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
> > index 35601b8176..4fe5a78b29 100644
> > --- a/target/i386/whpx-all.c
> > +++ b/target/i386/whpx-all.c
> > @@ -980,38 +980,32 @@ static int whpx_vcpu_run(CPUState *cpu)
> >              WHV_REGISTER_VALUE reg_values[5];
> >              WHV_REGISTER_NAME reg_names[5];
> >              UINT32 reg_count = 5;
> > -            UINT64 rip, rax, rcx, rdx, rbx;
> > +            UINT64 cpuid_fn, rip = 0, rax = 0, rcx = 0, rdx = 0, rbx = 0;
> > +            X86CPU *x86_cpu = X86_CPU(cpu);
> > +            CPUX86State *env = &x86_cpu->env;
> >
> >              memset(reg_values, 0, sizeof(reg_values));
> >
> >              rip = vcpu->exit_ctx.VpContext.Rip +
> >                    vcpu->exit_ctx.VpContext.InstructionLength;
> > -            switch (vcpu->exit_ctx.CpuidAccess.Rax) {
> > -            case 1:
> > -                rax = vcpu->exit_ctx.CpuidAccess.DefaultResultRax;
> > -                /* Advertise that we are running on a hypervisor */
> > -                rcx =
> > -                    vcpu->exit_ctx.CpuidAccess.DefaultResultRcx |
> > -                    CPUID_EXT_HYPERVISOR;
> > -
> > -                rdx = vcpu->exit_ctx.CpuidAccess.DefaultResultRdx;
> > -                rbx = vcpu->exit_ctx.CpuidAccess.DefaultResultRbx;
> > -                break;
> > +            cpuid_fn = vcpu->exit_ctx.CpuidAccess.Rax;
> > +
> > +            /*
> > +             * Ideally, these should be supplied to the hypervisor during 
> > VCPU
> > +             * initialization and it should be able to satisfy this 
> > request.
> > +             * But, currently, WHPX doesn't support setting CPUID values 
> > in the
> > +             * hypervisor once the partition has been setup, which is too 
> > late
> > +             * since VCPUs are realized later. For now, use the values from
> > +             * QEMU to satisfy these requests, until WHPX adds support for
> > +             * being able to set these values in the hypervisor at runtime.
> > +             */
> > +            cpu_x86_cpuid(env, cpuid_fn, 0, (UINT32 *)&rax, (UINT32 *)&rbx,
> > +                (UINT32 *)&rcx, (UINT32 *)&rdx);
> > +            switch (cpuid_fn) {
> >              case 0x80000001:
> > -                rax = vcpu->exit_ctx.CpuidAccess.DefaultResultRax;
> >                  /* Remove any support of OSVW */
> > -                rcx =
> > -                    vcpu->exit_ctx.CpuidAccess.DefaultResultRcx &
> > -                    ~CPUID_EXT3_OSVW;
> > -
> > -                rdx = vcpu->exit_ctx.CpuidAccess.DefaultResultRdx;
> > -                rbx = vcpu->exit_ctx.CpuidAccess.DefaultResultRbx;
> > +                rcx &= ~CPUID_EXT3_OSVW;
> >                  break;
> > -            default:
> > -                rax = vcpu->exit_ctx.CpuidAccess.DefaultResultRax;
> > -                rcx = vcpu->exit_ctx.CpuidAccess.DefaultResultRcx;
> > -                rdx = vcpu->exit_ctx.CpuidAccess.DefaultResultRdx;
> > -                rbx = vcpu->exit_ctx.CpuidAccess.DefaultResultRbx;
> >              }
> >
> >              reg_names[0] = WHvX64RegisterRip;
> > --
> > 2.17.1
> >
> 
> --
> Eduardo