On 06/11/18 16:16, Jan Beulich wrote: >>>> On 06.11.18 at 16:52, <andrew.coop...@citrix.com> wrote: >> On 06/11/18 15:38, Jan Beulich wrote: >>>>>> On 05.11.18 at 12:21, <andrew.coop...@citrix.com> wrote: >>>> They are identical, so provide a single x86emul_cpuid() instead. >>>> >>>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID >> instructions, >>>> the hook can be omitted from all special-purpose emulation ops. >>> So I was expecting the hook to go away altogether, but I >>> now realize that it can't because of some of the customization >>> that's needed. That, in turn, means that the removal of the >>> hook specification as per above will get us into problems the >>> moment we need to check a feature that can't be taken >>> straight from the policy object. I'm therefore unconvinced we >>> actually want to go this far. It'll require enough care already >>> to not blindly clone a new vcpu_has_...() wrongly from the >>> many pre-existing examples in such a case. Thoughts? >> All dynamic bits in CPUID are derived from other control state. e.g. we >> check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC, >> which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4. >> >> In the emulator itself, I think it would be a bug if we ever had >> vcpu_has_osxsave etc, because that isn't how pipelines actually behave. >> The feature checks here are semantically equivalent to "do the >> instruction decode and execution units have silicon to cope with these >> instructions". > I agree that vcpu_has_os* makes no sense, but the APIC bit for > example isn't really pipeline / decoder related.
Correct, so why do you think APIC matters? All interaction with the APIC is via its MMIO/MSR interface, rather than via dedicated instructions. > However, one > issue already might be that in order for bits in a (sub)leaf above > (guest) limits to come out all clear, it is guest_cpuid() which cuts > things off. Neither cpuid_featureset_to_policy() nor its inverse > nor sanitise_featureset() look to zap fields above limits, in case > they've been previously set to non-zero values. Am I overlooking > something? No - that is an aspect I'd overlooked, because the DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet. I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf settings, because the intention was always that a flat cpuid_policy was safe to use in this manner. I think there is an existing corner case when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x8000007. > Furthermore I wouldn't exclude that we may need to look at a > hypervisor or Viridian leaf at some point. The dynamic vPMU > adjustments also look potentially problematic, if we were to > emulate RDPMC (albeit they're DS-related only right now). The only reason vPMU is dynamic is because we don't (yet) have a split between default and max policies. Fixing this is on the todo list, albeit behind a fairly long chain of dependencies. > And then there's the dynamic exposing of MONITOR for PV; granted > I don't expect us to ever emulate this for PV, but it shows the > general issue. That is to work around a deficiency in how Linux behaves. It isn't for allowing PV guests to use MONITOR. > Plus there's SYSCALL, the insn emulation of which > currently doesn't check the (dynamically adjusted) CPUID bit. No, nor should it. Intel's objection to the SYSCALL/SYSRET instructions is mode based. (As a demonstration which proves the point of this patch, if you hide the SYSCALL bit using masking, the instruction still operates fine). It seems I never got around to submitting my XSA-204 followup patch which fixes many emulation bugs with SYS{CALL,RET,ENTER,EXIT}. > And finally LWP, which again we can only hope we'll never have > to emulate. LWP doesn't exist any more, even on the hardware it used to exist on. It was never implemented on Fam17h, and was removed from Fam15/16h in a microcode update to make room to implement IBPB for Spectre v2 mitigations. I recommend we purge the support completely. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel