Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 15.11.18 at 15:23, wrote: > On 09/11/2018 17:16, Andrew Cooper wrote: >> >>> 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 < 0x807. > > Actually, I remember now that I tried fixing this once before. It's not > possible without DOMCTL_set_cpu_policy because the current API with > DOMCTL_set_cpuid provides leaves piecewise and there is no point at > which Xen can safely zero the out-of-range leaves without loosing > information later if max_leaf is increased. There could be such a point if an arch call was added at the point where d->creation_finished gets set the first time. Looking at the domctl side flow I'm wondering what the domain pausing is good for there, now that we don't allow changes once d->creation_finished is set. Was not dropping this an oversight of 3d0cab7b5d? > The content of cpuid_policy will be safe for the emulator to use, > because it will be bounded by real hardware support. Except that it voids all respective uses of vcpu_has_*() - just cpu_has_* would then be sufficient. And overall behavior is inconsistent between bit-wise leveling and full leaf hiding. > As levelling like > this is an extreme corner case which doesn't work at the moment for > other reasons, I'd like to take this series now and fix up the behaviour > later, to reduce the dependencies in the remaining CPUID/MSR work - its > already complicated enough. Hmm, okay, as long as a respective remark gets added at least to the commit message, I could live with that. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
On 09/11/2018 17:16, Andrew Cooper wrote: > >> 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 < 0x807. Actually, I remember now that I tried fixing this once before. It's not possible without DOMCTL_set_cpu_policy because the current API with DOMCTL_set_cpuid provides leaves piecewise and there is no point at which Xen can safely zero the out-of-range leaves without loosing information later if max_leaf is increased. The content of cpuid_policy will be safe for the emulator to use, because it will be bounded by real hardware support. As levelling like this is an extreme corner case which doesn't work at the moment for other reasons, I'd like to take this series now and fix up the behaviour later, to reduce the dependencies in the remaining CPUID/MSR work - its already complicated enough. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 14.11.18 at 01:20, wrote: > On Mon, Nov 12, 2018 at 01:17:59AM -0700, Jan Beulich wrote: >> >>> On 09.11.18 at 18:16, wrote: >> > 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? > > 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. Okay, this confirms your agreement with the removal plans, but is there any chance the patch removing it could actually come from an AMD person, as suggested above? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
On Mon, Nov 12, 2018 at 01:17:59AM -0700, Jan Beulich wrote: > >>> On 09.11.18 at 18:16, wrote: > > On 06/11/18 16:16, Jan Beulich wrote: > > On 06.11.18 at 16:52, wrote: > >>> On 06/11/18 15:38, Jan Beulich wrote: > >>> On 05.11.18 at 12:21, 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
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 09.11.18 at 18:16, wrote: > On 06/11/18 16:16, Jan Beulich wrote: > On 06.11.18 at 16:52, wrote: >>> On 06/11/18 15:38, Jan Beulich wrote: >>> On 05.11.18 at 12:21, 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
On 06/11/18 16:16, Jan Beulich wrote:
On 06.11.18 at 16:52, wrote:
>> On 06/11/18 15:38, Jan Beulich wrote:
>> On 05.11.18 at 12:21, 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 < 0x807.
> 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
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 06.11.18 at 16:52, wrote: > On 06/11/18 15:38, Jan Beulich wrote: > On 05.11.18 at 12:21, 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. 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? 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). 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. Plus there's SYSCALL, the insn emulation of which currently doesn't check the (dynamically adjusted) CPUID bit. And finally LWP, which again we can only hope we'll never have to emulate. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
On 06/11/18 15:38, Jan Beulich wrote: On 05.11.18 at 12:21, 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". ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 05.11.18 at 12:21, 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? Folding the two identical implementations, otoh, is fine with me. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
