[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

>>> On 09.11.18 at 18:16, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

It was a general example, not something that specifically matters here.

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

I certainly don't mind; I'd prefer though if such a withdrawal of
functionality came actually from AMD. Brian?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.