Re: [PATCH 7/9] x86/hvm: Disable MPX by default

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2020 18:15, Andrew Cooper wrote:
> On 16/06/2020 10:33, Jan Beulich wrote:
>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>>> domid, bool restore,
>>>          goto out;
>>>      }
>>>  
>>> +    /*
>>> +     * Account for feature which have been disabled by default since Xen 
>>> 4.13,
>>> +     * so migrated-in VM's don't risk seeing features disappearing.
>>> +     */
>>> +    if ( restore )
>>> +    {
>>> +        if ( di.hvm )
>>> +        {
>>> +            p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>> Why do you derive this from the host featureset instead of the max
>> one for the guest type?
> 
> Because that is how the logic worked for 4.13.
> 
> Also, because we don't have easy access to the actual guest max
> featureset at this point.  I could add two new sysctl subops to
> get_featureset, but the reason for not doing so before are still
> applicable now.
> 
> There is a theoretical case where host MPX is visible but guest max is
> hidden, and that is down to the vmentry controls.  As this doesn't exist
> in real hardware, I'm not terribly concerned about it.

I'd also see us allow features to be kept for the host, but masked
off of the/some guest feature sets, by way of a to-be-introduced
command line option.

I take your reply to mean that you agree that conceptually it
ought to be max which gets used here, but there's no practical
difference at this point.

>>  Also, while you modify p here, ...
>>
>>> +        }
>>> +    }
>>> +
>>>      if ( featureset )
>>>      {
>>>          uint32_t disabled_features[FEATURESET_NR_ENTRIES],
>> ... the code in this if()'s body ignores p altogether.
> 
> That is correct.
> 
>> I realize the
>> only caller of the function passes NULL for "featureset", but I'd
>> like to understand the rationale here anyway before giving an R-b.
> 
> The meaning of 'featureset' is "here are the exact bits I want you to use".

With validation to happen only in the hypervisor then, I suppose?

If for both parts my understanding is correct:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan