| Message ID | 005a7254-aae8-ec7c-6e65-9dfe06803208@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Protected Processor Inventory Number (PPIN) support | expand |
On 30/10/2019 10:39, Jan Beulich wrote: > To fulfill the "protected" in its name, don't let the real hardware > values "shine through". Report a control register value expressing this. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Do we want to permit Dom0 access? I would recommend reordering the two patches and putting this one first (along with the enumeration details, along with a pair of feature strings in xen-cpuid). This patch at least wants backporting. This would be far more simple if we don't permit dom0 access. Yes, it shares platform responsibility with Xen, but it also can't do anything more with the value than Xen can, which is to simply print it out for #MCEs. Avoiding giving dom0 access would remove the need to attempt to virtualise something which is model specific on the Intel side, and allow all 4 MSRs to be unconditional #GP's. I for one don't want to have to consider the migration implications of letting guests see this. ~Andrew
On 30.10.2019 12:43, Andrew Cooper wrote: > On 30/10/2019 10:39, Jan Beulich wrote: >> To fulfill the "protected" in its name, don't let the real hardware >> values "shine through". Report a control register value expressing this. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> TBD: Do we want to permit Dom0 access? > > I would recommend reordering the two patches and putting this one first > (along with the enumeration details, along with a pair of feature > strings in xen-cpuid). This patch at least wants backporting. Well, the reason for this ordering is because this way Dom0 doesn't transiently lose all access. As to xen-cpuid - I admit I simply forgot to update it, largely due to there not being any CPUID bit on the Intel side. That part would obviously live in whichever patch we elect to be first. > This would be far more simple if we don't permit dom0 access. Yes, it > shares platform responsibility with Xen, but it also can't do anything > more with the value than Xen can, which is to simply print it out for #MCEs. Okay, then let's not expose it. I'll drop the TBD. > Avoiding giving dom0 access would remove the need to attempt to > virtualise something which is model specific on the Intel side, and > allow all 4 MSRs to be unconditional #GP's. I for one don't want to > have to consider the migration implications of letting guests see this. I don't understand the connection between Dom0 access and possible migration implications. I'm also struggling to see how, for a well behaved guest, there would need to be any migration considerations in the first place: Once it has read the control register and found the lockout bit set, it shouldn't make any further access attempts. Similarly once it got a #GP(0) upon access, it wouldn't try again. There would be an issue only if we actually supplied PPIN values, even if it were just fake ones. Jan
Thanks for this series, Jan. On 30.10.19 11:39, Jan Beulich wrote: > To fulfill the "protected" in its name, don't let the real hardware > values "shine through". Report a control register value expressing this. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Do we want to permit Dom0 access? It would be nice to give an administrator a way to get PPIN outside the context of an MCE when needed. > > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -135,6 +135,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t > case MSR_TSX_FORCE_ABORT: > case MSR_AMD64_LWP_CFG: > case MSR_AMD64_LWP_CBADDR: > + case MSR_PPIN: > + case MSR_AMD_PPIN: > /* Not offered to guests. */ > goto gp_fault; > > @@ -237,6 +239,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t > ARRAY_SIZE(msrs->dr_mask))]; > break; > > + case MSR_PPIN_CTL: > + if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL ) > + goto gp_fault; > + *val = PPIN_LOCKOUT; > + break; > + > + case MSR_AMD_PPIN_CTL: > + if ( !cp->extd.amd_ppin ) > + goto gp_fault; > + *val = PPIN_LOCKOUT; > + break; > + nit: It is not clear to me why you use "d->arch.cpuid->.." (and not "cp->..") in the first if condition. -- Eslam
On 01/11/2019 14:00, Eslam Elnikety wrote: > Thanks for this series, Jan. > > On 30.10.19 11:39, Jan Beulich wrote: >> To fulfill the "protected" in its name, don't let the real hardware >> values "shine through". Report a control register value expressing this. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> TBD: Do we want to permit Dom0 access? > > It would be nice to give an administrator a way to get PPIN outside > the context of an MCE when needed. I suppose this is a reasonable request. We should expose it to the hardware domain. ~Andrew
On 30/10/2019 12:02, Jan Beulich wrote: > On 30.10.2019 12:43, Andrew Cooper wrote: >> On 30/10/2019 10:39, Jan Beulich wrote: >>> To fulfill the "protected" in its name, don't let the real hardware >>> values "shine through". Report a control register value expressing this. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> TBD: Do we want to permit Dom0 access? >> I would recommend reordering the two patches and putting this one first >> (along with the enumeration details, along with a pair of feature >> strings in xen-cpuid). This patch at least wants backporting. > Well, the reason for this ordering is because this way Dom0 > doesn't transiently lose all access. Nothing pre-existing can be used reliably by dom0 because of the raz/write-discard behaviour. I wouldn't complicate patch ordering because of this. > > As to xen-cpuid - I admit I simply forgot to update it, largely > due to there not being any CPUID bit on the Intel side. That part > would obviously live in whichever patch we elect to be first. > >> This would be far more simple if we don't permit dom0 access. Yes, it >> shares platform responsibility with Xen, but it also can't do anything >> more with the value than Xen can, which is to simply print it out for #MCEs. > Okay, then let's not expose it. I'll drop the TBD. I'll re-review with dom0 access in mind, but start a new thread. ~Andrew
On 01/11/2019 14:29, Andrew Cooper wrote: > On 01/11/2019 14:00, Eslam Elnikety wrote: >> Thanks for this series, Jan. >> >> On 30.10.19 11:39, Jan Beulich wrote: >>> To fulfill the "protected" in its name, don't let the real hardware >>> values "shine through". Report a control register value expressing this. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> TBD: Do we want to permit Dom0 access? >> It would be nice to give an administrator a way to get PPIN outside >> the context of an MCE when needed. > I suppose this is a reasonable request. We should expose it to the > hardware domain. Actually on further thoughts, I'm going to backtrack slightly. It is reasonable to give to dom0, but it is not reasonable to provide it by emulating the MSR interface. The problem is that dom0's result is sensitive to where it happens to be scheduled. The only sane way of letting dom0 have access is via a hypercall which returns "no PPIN" or all sockets information in one go, irrespective of which socket the current vcpu happens to be executing on. This leaves us back in the (substantially easier) position of not having to virtualise the MSR interface to begin with. ~Andrew
On 01.11.2019 15:29, Andrew Cooper wrote: > On 01/11/2019 14:00, Eslam Elnikety wrote: >> Thanks for this series, Jan. >> >> On 30.10.19 11:39, Jan Beulich wrote: >>> To fulfill the "protected" in its name, don't let the real hardware >>> values "shine through". Report a control register value expressing this. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> TBD: Do we want to permit Dom0 access? >> >> It would be nice to give an administrator a way to get PPIN outside >> the context of an MCE when needed. > > I suppose this is a reasonable request. We should expose it to the > hardware domain. Via (new) sysctl (or platform op) or by allowing direct MSR read access? (If the former, I'd want to make this addition a separate patch.) Jan
On 01.11.2019 15:00, Eslam Elnikety wrote: > On 30.10.19 11:39, Jan Beulich wrote: >> @@ -237,6 +239,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t >> ARRAY_SIZE(msrs->dr_mask))]; >> break; >> >> + case MSR_PPIN_CTL: >> + if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL ) >> + goto gp_fault; >> + *val = PPIN_LOCKOUT; >> + break; >> + >> + case MSR_AMD_PPIN_CTL: >> + if ( !cp->extd.amd_ppin ) >> + goto gp_fault; >> + *val = PPIN_LOCKOUT; >> + break; >> + > > nit: It is not clear to me why you use "d->arch.cpuid->.." (and not > "cp->..") in the first if condition. Simple oversight; corrected for v2. Jan
On 01.11.2019 19:35, Andrew Cooper wrote: > On 30/10/2019 12:02, Jan Beulich wrote: >> On 30.10.2019 12:43, Andrew Cooper wrote: >>> On 30/10/2019 10:39, Jan Beulich wrote: >>>> To fulfill the "protected" in its name, don't let the real hardware >>>> values "shine through". Report a control register value expressing this. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> TBD: Do we want to permit Dom0 access? >>> I would recommend reordering the two patches and putting this one first >>> (along with the enumeration details, along with a pair of feature >>> strings in xen-cpuid). This patch at least wants backporting. >> Well, the reason for this ordering is because this way Dom0 >> doesn't transiently lose all access. > > Nothing pre-existing can be used reliably by dom0 because of the > raz/write-discard behaviour. Why "raz" - default behavior for "un-enumerated" MSRs is to hand out the raw hardware value. I agree Dom0 can't _enable_ the PPIN MSR (due to the write-discard default behavior), but on systems where the firmware does the enabling it could still have read the values. Jan
On 01.11.2019 19:49, Andrew Cooper wrote: > On 01/11/2019 14:29, Andrew Cooper wrote: >> On 01/11/2019 14:00, Eslam Elnikety wrote: >>> Thanks for this series, Jan. >>> >>> On 30.10.19 11:39, Jan Beulich wrote: >>>> To fulfill the "protected" in its name, don't let the real hardware >>>> values "shine through". Report a control register value expressing this. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> TBD: Do we want to permit Dom0 access? >>> It would be nice to give an administrator a way to get PPIN outside >>> the context of an MCE when needed. >> I suppose this is a reasonable request. We should expose it to the >> hardware domain. > > Actually on further thoughts, I'm going to backtrack slightly. > > It is reasonable to give to dom0, but it is not reasonable to provide it > by emulating the MSR interface. The problem is that dom0's result is > sensitive to where it happens to be scheduled. > > The only sane way of letting dom0 have access is via a hypercall which > returns "no PPIN" or all sockets information in one go, irrespective of > which socket the current vcpu happens to be executing on. > > This leaves us back in the (substantially easier) position of not having > to virtualise the MSR interface to begin with. Right, hence my question whether to make this a new sysctl subop, or whether to permit reading of this one MSR via XENPF_resource_op (or yet some other mechanism). Personally I'd go the XENPF_resource_op route. Jan
--- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -135,6 +135,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t case MSR_TSX_FORCE_ABORT: case MSR_AMD64_LWP_CFG: case MSR_AMD64_LWP_CBADDR: + case MSR_PPIN: + case MSR_AMD_PPIN: /* Not offered to guests. */ goto gp_fault; @@ -237,6 +239,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t ARRAY_SIZE(msrs->dr_mask))]; break; + case MSR_PPIN_CTL: + if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL ) + goto gp_fault; + *val = PPIN_LOCKOUT; + break; + + case MSR_AMD_PPIN_CTL: + if ( !cp->extd.amd_ppin ) + goto gp_fault; + *val = PPIN_LOCKOUT; + break; + /* * TODO: Implement when we have better topology representation. case MSR_INTEL_CORE_THREAD_COUNT: @@ -273,10 +287,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t case MSR_INTEL_CORE_THREAD_COUNT: case MSR_INTEL_PLATFORM_INFO: case MSR_ARCH_CAPABILITIES: + case MSR_PPIN: + case MSR_AMD_PPIN: /* Read-only */ case MSR_TSX_FORCE_ABORT: case MSR_AMD64_LWP_CFG: case MSR_AMD64_LWP_CBADDR: + case MSR_PPIN_CTL: + case MSR_AMD_PPIN_CTL: /* Not offered to guests. */ goto gp_fault;
To fulfill the "protected" in its name, don't let the real hardware values "shine through". Report a control register value expressing this. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- TBD: Do we want to permit Dom0 access?