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

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

On 18/03/16 16:57, Jan Beulich wrote:
>>>> On 15.03.16 at 16:35, <andrew.cooper3@citrix.com> wrote:
>> v3:
>>  * Rebase over the new namespaceing changes.
>>  * Expand commit message.
>>  * Correct PSE36 to being a HAP-only feature.
> As Tim has pointed out on IRC, this may need revisiting.

I am still debating how to fix this.  I absolutely don't want to pollute
Xen with an architecturally-wrong workaround for a bug some unidentified
version of HyperV from 2011.

>
>> +XEN_CPUFEATURE(MCE,           0*32+ 7) /*A  Machine Check Architecture */
> Exposing MCA makes sense, but do we really need to expose MCE
> too?

CR4.MCE leaks into PV guests.

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

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

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.

>
>>  XEN_CPUFEATURE(DS,            0*32+21) /*   Debug Store */
> Is leaving this unexposed compatible with vPMU (would then
> presumably also apply to DTES64 and DSCPL)?

vPMU modification happens after masking, so this doesn't interfere with
anything.

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

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

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

> Or if not, perhaps the commit message could be made say a word on the
> intention regarding runtime overrides to these statically determine
> sets?

In this series, libxc can calculate this and sets it appropriately for
each domain.

Xen currently lacks a per-domain policy, so doesn't audit the validity
of the data in the set_cpuid_policy hypercall.  As a buggy toolstack
could send bad policy data, the runtime checks are kept for safety.

Once this series is accepted, I will be working on the next phase, which
will include per-domain policies, and auditing for correctness at the
hypercall boundary.  Once this infrastructure is complete, the runtime
overrides will be removed.

>
>> +XEN_CPUFEATURE(LWP,           3*32+15) /*A  Light Weight Profiling */
> Right now this gets cleared by pv_cpuid(), so I think this needs to
> be S.#

So it does.  Yes it should.

~Andrew

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