Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/11/18 16:16, Jan Beulich wrote:
>>>> On 06.11.18 at 16:52, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 06/11/18 15:38, Jan Beulich wrote:
>>>>>> On 05.11.18 at 12:21, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel