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

From: "Woods, Brian" <Brian.Woods@amd.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	"Woods, Brian" <Brian.Woods@amd.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
Date: Wed, 14 Nov 2018 00:20:35 +0000	[thread overview]
Message-ID: <20181114002020.GA3640@amd.com> (raw)
In-Reply-To: <5BE9373702000078001FAB51@prv1-mh.provo.novell.com>

On Mon, Nov 12, 2018 at 01:17:59AM -0700, Jan Beulich wrote:
> >>> On 09.11.18 at 18:16, <andrew.cooper3@citrix.com> wrote:
> > On 06/11/18 16:16, Jan Beulich wrote:
> >>>>> On 06.11.18 at 16:52, <andrew.cooper3@citrix.com> wrote:
> >>> On 06/11/18 15:38, Jan Beulich wrote:
> >>>>>>> On 05.11.18 at 12:21, <andrew.cooper3@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.
> 
> 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?
> 
> Jan

LWP support isn't enabled on F15h system with the latest Ucode and it
isn't available on F17h.  I don't see any reason to keep it if it's
hampering cleanup and reformatting.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel