Re: [Xen-devel] [PATCH v3 08/28] xen/x86: Annotate VM applicability in featureset

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.16 at 19:56, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/03/16 16:57, Jan Beulich wrote:
>>>>> On 15.03.16 at 16:35, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +XEN_CPUFEATURE(MTRR,          0*32+12) /*S  Memory Type Range Registers */
>> I thin I've said so before - this alters current behavior
> 
> Again, no it doesn't.  PV DomU's don't get to see MTRR; the feature is
> clobbered in the toolstack.

Ah, indeed.

>> , and is pretty certainly wrong for PV Dom0.
> 
> And again, no it very much isn't.  PV guests cannot use MTRRs, so
> shouldn't see the feature.
> 
> PVOPS specifically self-knobbles MTRR if it is found in the cpuid leaves
> (enlighten.c: xen_init_cpuid_mask()).  classic-xen kernels don't even
> look at the feature bit if they are not dom0.
> 
> I presume your complaint is because SUSE are still using a classic-xen
> dom0 kernel?  The check is already buggy because there is no guarantee
> (or reasonable expectation) that Xen has MTRRs to use in the first
> place.  Why don't you fix this by checking for what the mtrr driver is
> actually using, i.e. the presence of the XENPF_add_memtype hypercall,
> which could be implemented in Xen using PAT?

Well, it has always been my understanding that the CPUID MTRR
bit to a PV guest means just that - availability of the respective
hypercalls. Also note that whatever change I might do to the old
sources, it wouldn't help with existing kernel binaries, which you
must not break.

> I could be persuaded to implement a dom0 specific override in pv_cpuid()
> to cover the buggy cases, but this line of code is not changing.

And I'm fine with such an override, but please make not of such
special cases in the commit message, so the question won't come
up again and again (by me or future readers).

>>> +XEN_CPUFEATURE(X2APIC,        1*32+21) /*A  Extended xAPIC */
>> Does this make sense for PV?
> 
> It is currently visible, and we already have to leak APIC through to PV
> guests.

Having to leak on piece of state doesn't mean when need to leak
more, when that's clearly impossibly to be meaningful to a guest.

>>> +XEN_CPUFEATURE(HYPERVISOR,    1*32+31) /*A  Running under some hypervisor 
>>> */
>> Wouldn't this better be one of the special ones?
> 
> Why? It doesn't need any special handling in Xen.  For all intents and
> purposes, it is just like a regular feature bit.

Except that you don't pass through its host value, but instead
override it to 1. Which makes it kind of special, I would say.

>>> +XEN_CPUFEATURE(LM,            2*32+29) /*A  Long Mode (x86-64) */
>> I think I had asked before, but doesn't the customization needed
>> for 32-bit PV guests also rather make this a special one?
> 
> Why would it?  It is a simple feature which isn't present for 32bit guests.

Together with the above the question really is: What exactly makes
a feature special? The need for run time overrides, or something
more sophisticated?

Jan


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