Re: [PATCH v2 10/30] xen/x86: Annotate VM applicability in featureset

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/02/16 09:20, Jan Beulich wrote:
>>>> On 12.02.16 at 18:42, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/16 17:05, Jan Beulich wrote:
>>>>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
>>>> +#define X86_FEATURE_VMXE          ( 1*32+ 5) /*S  Virtual Machine Extensions */
>>> Shouldn't this get a "nested-only" class?
>> I am not sure that would be appropriate.  On the Intel side, this bit is
>> the only option in cpuid; the VT-x features need MSR-levelling, which is
>> moderately far away on my TODO list.
>>
>> Having said that, the AMD side has all nested features in cpuid.  I
>> guess this is more a problem for whomever steps up and makes nested virt
>> a properly supported option, but this is way off at this point.
> Okay then. My hope was that introducing the extra category
> wouldn't be too much extra effort inside this series.
>>> Also don't we currently require HAP for nested mode to work?
>> Experimentally, the p2m lock contention caused by Shadow and Nested virt
>> caused Xen to fall over very frequently with watchdog timeouts.
>>
>> Having said that, nothing formal is written down one way or another, and
>> it is possible in limited scenarios to make nested virt work without
>> hap.  FWIW, I would be happy with a blanket "no nested virt without HAP"
>> statement for Xen.
> Same here.

Where possible, I am deliberately trying not to make policy changes
hidden inside a large functional series.

As far as nested virt specifically goes, I am still not sure what is the
best approach, so would prefer not to change things at this time.

>
>>>>  #define X86_FEATURE_OSXSAVE       ( 1*32+27) /*   OSXSAVE */
>>> Leaving this untouched warrants at least a comment in the commit
>>> message I would think.
>> The handling for the magic bits
> Unfinished sentence?

Oops yes, although it was going to be statement rather than a query.

>
>>>> +#define X86_FEATURE_RDTSCP        ( 2*32+27) /*S  RDTSCP */
>>> Hmm, I'm confused - on one hand we currently clear this bit for
>>> PV guests, but otoh do_invalid_op() emulates it.
>> Urgh yes - I had forgotten about this gem.  I lacked sufficient tuits to
>> untangle the swamp which is the vtsc subsystem.
>>
>> Currently, the dynamic vtsc setting controls whether the RDTSCP feature
>> flag is visible.
> I don't see where that would be happening - all I see is a single
>
>         __clear_bit(X86_FEATURE_RDTSCP % 32, &d);
>
> in pv_cpuid().

The HVM side is more dynamic.  Either way, the subsystem is a mess.

>
>>>> +#define X86_FEATURE_LM            ( 2*32+29) /*A  Long Mode (x86-64) */
>>>> [...]
>>>> -#define X86_FEATURE_LAHF_LM       ( 3*32+ 0) /*   LAHF/SAHF in long mode */
>>>> +#define X86_FEATURE_LAHF_LM       ( 3*32+ 0) /*A  LAHF/SAHF in long mode */
>>> How do you intend to handle exposing these to 64-bit PV guests,
>>> but not to 32-bit ones?
>> At the end of this series, the deep dependency logic used by the
>> toolstack, and some remaining dynamic checks in the intercept hooks.
> Okay, I'll try to remember to look there once I get to that point in
> the series.
>
>>>>  #define X86_FEATURE_EXTAPIC       ( 3*32+ 3) /*   Extended APIC space */
>>> This currently is left untouched for HVM guests, and gets cleared
>>> only when !cpu_has_apic (i.e. effectively never) for PV ones.
>> There is no HVM support for handling a guest trying to use EXTAPIC, and
>> PV guests don't get to play with the hardware APIC anyway.  As far as I
>> can tell, it has always been wrong to ever expose this feature.
> Well, that's a fair statement, but should be made in the commit
> message (after all it's a behavioral change).

I will include it in the commit message in the future.

>
>>>>  #define X86_FEATURE_MWAITX        ( 3*32+29) /*   MWAIT extension 
>> (MONITORX/MWAITX) */
>>> Why not exposed to HVM (also for _MWAIT as I now notice)?
>> Because that is a good chunk of extra work to support.  We would need to
>> use 4K monitor widths, and extra p2m handling.
> I don't understand: The base (_MWAIT) feature being exposed to
> guests today, and kernels making use of the feature when available
> suggests to me that things work. Are you saying you know
> otherwise? (And if there really is a reason to mask the feature all of
> the sudden, this should again be justified in the commit message.)

PV guests had it clobbered by Xen in traps.c

HVM guests have:

vmx.c:
    case EXIT_REASON_MWAIT_INSTRUCTION:
    case EXIT_REASON_MONITOR_INSTRUCTION:
    case EXIT_REASON_GETSEC:
       
/*                                                                                                                                                                              

         * We should never exit on GETSEC because CR4.SMXE is always 0
when                                                                                                             

         * running in guest context, and the CPU checks that before
getting                                                                                                             

         * as far as
vmexit.                                                                                                                                                            

         */
        WARN_ON(exit_reason == EXIT_REASON_GETSEC);
    hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
        break;

and svm.c:
    case VMEXIT_MONITOR:
    case VMEXIT_MWAIT:
        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
        break;

I don't see how a guest could actually use this feature.

~Andrew