On 18/03/16 16:57, Jan Beulich wrote: >>>> On 15.03.16 at 16:35, <andrew.coop...@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
