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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/06/2020 11:32, Jan Beulich wrote:
> 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.

What kind of usecase do you have in mind for this?  We've got a load of
features which are blanket disabled for guests.  I suppose `ler` et al
ought to have an impact, except for the fact that LBR at that level
isn't architectural and always expected.

> 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.

If max were trivially available, I'd use use that, but its not, and host
is equivalent in the cases we care about.

>
>>>  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?

Almost none of this logic has any validation in the toolstack side (that
which does, was added by me fairly recently).  Its going to be one of
several large changes in the new world.

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

Thanks.

~Andrew