Re: [PATCH v2 2/2] [RfC] expose host-phys-bits to guest

[Michael S. Tsirkin]

On Fri, Sep 09, 2022 at 07:18:17AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > @@ -424,7 +426,10 @@ static void 
> > > microvm_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > >  {
> > >      X86CPU *cpu = X86_CPU(dev);
> > >  
> > > -    cpu->host_phys_bits = true; /* need reliable phys-bits */
> > > +    /* need reliable phys-bits */
> > > +    cpu->env.features[FEAT_KVM_HINTS] |=
> > > +        (1 << KVM_HINTS_PHYS_ADDRESS_SIZE_DATA_VALID);
> > > +
> > 
> > Do we need compat machinery for this?
> 
> Don't think so, microvm has no versioned machine types anyway.
> 
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> 
> > >      [FEAT_KVM_HINTS] = {
> > >          .type = CPUID_FEATURE_WORD,
> > >          .feat_names = {
> > > -            "kvm-hint-dedicated", NULL, NULL, NULL,
> > > +            "kvm-hint-dedicated", "host-phys-bits", NULL, NULL,
> 
> > > -    DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> 
> > > -    if (cpu->host_phys_bits) {
> > > +    if (cpu->env.features[FEAT_KVM_HINTS] &
> > > +        (1 << KVM_HINTS_PHYS_ADDRESS_SIZE_DATA_VALID)) {
> > >          /* The user asked for us to use the host physical bits */
> > >          phys_bits = host_phys_bits;
> > >          if (cpu->host_phys_bits_limit &&
> > 
> > I think we still want to key this one off host_phys_bits
> > so it works for e.g. hyperv emulation too.
> 
> I think that should be the case.  The chunks above change the
> host-phys-bits option from setting cpu->host_phys_bits to setting
> the FEAT_KVM_HINTS bit.  That should also happen with hyperv emulation
> enabled, and the bit should also be visible to the guest then, just at
> another location (base 0x40000100 instead of 0x40000000).
> 
> take care,
>   Gerd


You are right, I forgot. Hmm, ok. What about !cpu->expose_kvm ?

We have

    if (!kvm_enabled() || !cpu->expose_kvm) {
        env->features[FEAT_KVM] = 0;
    }   
        
This is quick grep, I didn't check whether this is called
after the point where you currently use it, but
it frankly seems fragile to pass a generic user specified flag
inside a cpuid where everyone pokes at it.


-- 
MST