Re: [Qemu-devel] [PATCH] WHPX fixes an issue with CPUID 1 not returning CPUID_EXT_HYPERVISOR

[Justin Terry (VM)]

Hey Eduardo

Responses inline. Thanks!

> -----Original Message-----
> From: Eduardo Habkost <address@hidden>
> Sent: Wednesday, March 28, 2018 10:51 AM
> To: Justin Terry (VM) <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: [PATCH] WHPX fixes an issue with CPUID 1 not returning
> CPUID_EXT_HYPERVISOR
> 
> On Mon, Mar 26, 2018 at 10:06:58AM -0700, Justin Terry (VM) wrote:
> > Implements the CPUID trap for CPUID 1 to include the
> > CPUID_EXT_HYPERVISOR flag in the ECX results. This was preventing some
> > older linux kernels from booting when trying to access MSR's that dont
> > make sense when virtualized.
> >
> > Signed-off-by: Justin Terry (VM) <address@hidden>
> > ---
> >  target/i386/whpx-all.c | 79
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 78 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c index
> > bf33d320bf..58435178a4 100644
> > --- a/target/i386/whpx-all.c
> > +++ b/target/i386/whpx-all.c
> > @@ -911,12 +911,62 @@ static int whpx_vcpu_run(CPUState *cpu)
> >              ret = 1;
> >              break;
> >
> > +        case WHvRunVpExitReasonX64Cpuid: {
> > +            WHV_REGISTER_VALUE reg_values[5] = {0};
> > +            WHV_REGISTER_NAME reg_names[5];
> > +            UINT32 reg_count = 5;
> > +            UINT64 rip, rax, rcx, rdx, rbx;
> > +
> > +            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;
> > +            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;
> 
> Interesting, so the WHPX API already tries to provide default values for the
> CPUID leaves.  Would it make sense to try and use the values returned by
> cpu_x86_cpuid() in the future?
> 
> Is there a way to get the default CPUID results from the WHPX API without
> calling WHvRunVirtualProcessor(), so QEMU can be aware of what exactly
> the guest is seeing on CPUID?

The platform now has two ways to interact with CPUID.

1. (As the code is doing now). At partition creation time you can register for 
specific CPUID exits and then respond to the CPUID with your custom answer or 
with the Hypervisor defaults that were forwarded to you. Unfortunately, QEMU 
has no way to know the Hypervisor default ahead of time but QEMU can make at 
least make a runtime decision about how to respond.
2. At partition creation time the platform allows QEMU to inject (set) the 
default responses for specific CPUID exits. This can now be done by setting the 
 `WHV_X64_CPUID_RESULT` in the `CpuidResultList` of `WHV_PARTITION_PROPERTY` to 
the exit values QEMU wants. So effectively you can know the answers ahead of 
time for any that you set but the answers are not dynamic.

The only issues/questions I have there are:

If we use [1] (like the code is now) I don't see any way to keep the exits in 
cpu_x86_cpuid() matched up with the registered exits to WHPX. This means that 
WHPX would not be called in these cases and would instead get the Hypervisor 
default rather than the answer from cpu_x86_cpuid().

If we use [2] to inject the answers at creation time WHPX needs access to the 
CPUX86State at accel init which also doesn't seem to be possible in QEMU today. 
WHPX could basically just call cpu_x86_cpuid() for each CPUID QEMU cares about 
and plumb the answer before start. This has the best performance as we avoid 
the additional exits but has an issue in that the results must be known ahead 
of time.

And, we could obviously use a hybrid of the two for cases we know. Do you have 
any ideas that I could try out here on how you would like to see this work?

Thanks,
Justin

> 
> 
> > +            }
> > +
> > +            reg_names[0] = WHvX64RegisterRip;
> > +            reg_names[1] = WHvX64RegisterRax;
> > +            reg_names[2] = WHvX64RegisterRcx;
> > +            reg_names[3] = WHvX64RegisterRdx;
> > +            reg_names[4] = WHvX64RegisterRbx;
> > +
> > +            reg_values[0].Reg64 = rip;
> > +            reg_values[1].Reg64 = rax;
> > +            reg_values[2].Reg64 = rcx;
> > +            reg_values[3].Reg64 = rdx;
> > +            reg_values[4].Reg64 = rbx;
> > +
> > +            hr = WHvSetVirtualProcessorRegisters(whpx->partition,
> > +                                                 cpu->cpu_index,
> > +                                                 reg_names,
> > +                                                 reg_count,
> > +                                                 reg_values);
> > +
> > +            if (FAILED(hr)) {
> > +                error_report("WHPX: Failed to set CpuidAccess state 
> > registers,"
> > +                             " hr=%08lx", hr);
> > +            }
> > +            ret = 0;
> > +            break;
> > +        }
> >          case WHvRunVpExitReasonNone:
> >          case WHvRunVpExitReasonUnrecoverableException:
> >          case WHvRunVpExitReasonInvalidVpRegisterValue:
> >          case WHvRunVpExitReasonUnsupportedFeature:
> >          case WHvRunVpExitReasonX64MsrAccess:
> > -        case WHvRunVpExitReasonX64Cpuid:
> >          case WHvRunVpExitReasonException:
> >          default:
> >              error_report("WHPX: Unexpected VP exit code %d", @@
> > -1272,6 +1322,33 @@ static int whpx_accel_init(MachineState *ms)
> >          goto error;
> >      }
> >
> > +    memset(&prop, 0, sizeof(WHV_PARTITION_PROPERTY));
> > +    prop.ExtendedVmExits.X64CpuidExit = 1;
> > +    hr = WHvSetPartitionProperty(whpx->partition,
> > +                                 WHvPartitionPropertyCodeExtendedVmExits,
> > +                                 &prop,
> > +                                 sizeof(WHV_PARTITION_PROPERTY));
> > +
> > +    if (FAILED(hr)) {
> > +        error_report("WHPX: Failed to enable partition extended
> X64CpuidExit"
> > +                     " hr=%08lx", hr);
> > +        ret = -EINVAL;
> > +        goto error;
> > +    }
> > +
> > +    UINT32 cpuidExitList[] = {1};
> > +    hr = WHvSetPartitionProperty(whpx->partition,
> > +                                 WHvPartitionPropertyCodeCpuidExitList,
> > +                                 cpuidExitList,
> > +                                 RTL_NUMBER_OF(cpuidExitList) *
> > + sizeof(UINT32));
> > +
> > +    if (FAILED(hr)) {
> > +        error_report("WHPX: Failed to set partition CpuidExitList 
> > hr=%08lx",
> > +                     hr);
> > +        ret = -EINVAL;
> > +        goto error;
> > +    }
> > +
> >      hr = WHvSetupPartition(whpx->partition);
> >      if (FAILED(hr)) {
> >          error_report("WHPX: Failed to setup partition, hr=%08lx",
> > hr);
> > --
> > 2.11.0
> >
> 
> --
> Eduardo