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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.02.16 at 18:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/02/16 17:05, Jan Beulich wrote:
>>>>> On 05.02.16 at 14:42, <andrew.cooper3@xxxxxxxxxx> 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.

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

>>> +#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().

>>> +#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).

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel