[Xen-devel] [PATCH v2 0/3] x86: Protected Processor Inventory Number (PPIN) support

Jan Beulich posted 3 patches 35 weeks ago
Only 0 patches received!

[Xen-devel] [PATCH v2 0/3] x86: Protected Processor Inventory Number (PPIN) support

Posted by Jan Beulich 35 weeks ago
1: include the PPIN in MCE records when available
2: explicitly disallow guest access to PPIN
3: provide Dom0 access to PPIN via XENPF_resource_op

I have yet to get around to post the Linux side consumer
patch of the interface addition in patch 1.

Jan

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

Re: [Xen-devel] [PATCH v2 0/3] x86: Protected Processor Inventory Number (PPIN) support

Posted by Andrew Cooper 30 weeks ago
On 08/11/2019 15:22, Jan Beulich wrote:
> 1: include the PPIN in MCE records when available
> 2: explicitly disallow guest access to PPIN
> 3: provide Dom0 access to PPIN via XENPF_resource_op
>
> I have yet to get around to post the Linux side consumer
> patch of the interface addition in patch 1.

What about printing the PPIN information when Xen writes MCE details to
the console?

~Andrew

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

Re: [Xen-devel] [PATCH v2 0/3] x86: Protected Processor Inventory Number (PPIN) support

Posted by Jan Beulich 29 weeks ago
On 13.12.2019 20:50, Andrew Cooper wrote:
> On 08/11/2019 15:22, Jan Beulich wrote:
>> 1: include the PPIN in MCE records when available
>> 2: explicitly disallow guest access to PPIN
>> 3: provide Dom0 access to PPIN via XENPF_resource_op
>>
>> I have yet to get around to post the Linux side consumer
>> patch of the interface addition in patch 1.
> 
> What about printing the PPIN information when Xen writes MCE details to
> the console?

Do you mean in x86_mcinfo_dump(), or mc_panic(), or yet somewhere
else? In any event if we did so I'd want to arrange for each PPIN
to get logged at most once, to reduce unnecessary redundancy. Of
course there's a comment ahead of x86_mcinfo_dump() mentioning
that the output is to be parseable by mcelog, so I'm not sure if
we can reasonably change what we produce. Otoh I'm also not sure
the comment hasn't become stale with its (presumed) porting from
Linux anyway.

Jan

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

Re: [Xen-devel] [PATCH v2 0/3] x86: Protected Processor Inventory Number (PPIN) support

Posted by Andrew Cooper 29 weeks ago
On 16/12/2019 11:33, Jan Beulich wrote:
> On 13.12.2019 20:50, Andrew Cooper wrote:
>> On 08/11/2019 15:22, Jan Beulich wrote:
>>> 1: include the PPIN in MCE records when available
>>> 2: explicitly disallow guest access to PPIN
>>> 3: provide Dom0 access to PPIN via XENPF_resource_op
>>>
>>> I have yet to get around to post the Linux side consumer
>>> patch of the interface addition in patch 1.
>> What about printing the PPIN information when Xen writes MCE details to
>> the console?
> Do you mean in x86_mcinfo_dump(), or mc_panic(), or yet somewhere
> else?

Something which ends up on the console.

> In any event if we did so I'd want to arrange for each PPIN
> to get logged at most once, to reduce unnecessary redundancy.

Getting it printed, even redundantly is more helpful than not having it
printed.

> Of course there's a comment ahead of x86_mcinfo_dump() mentioning
> that the output is to be parseable by mcelog, so I'm not sure if
> we can reasonably change what we produce. Otoh I'm also not sure
> the comment hasn't become stale with its (presumed) porting from
> Linux anyway.

That comment is long obsolete.  What Xen prints needs substantial
rearranging for mcelog to parse it these days.

~Andrew

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

[Xen-devel] [PATCH v2 1/3] x86: include the PPIN in MCE records when available

Posted by Jan Beulich 35 weeks ago
Quoting the respective Linux commit:

    Intel Xeons from Ivy Bridge onwards support a processor identification
    number set in the factory. To the user this is a handy unique number to
    identify a particular CPU. Intel can decode this to the fab/production
    run to track errors. On systems that have it, include it in the machine
    check record. I'm told that this would be helpful for users that run
    large data centers with multi-socket servers to keep track of which CPUs
    are seeing errors.

Newer AMD CPUs support this too, at different MSR numbers.

Take the opportunity and hide __MC_NMSRS from the public interface going
forward.

[Linux commit 3f5a7896a5096fd50030a04d4c3f28a7441e30a5]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also update xen-cpuid and libxl_cpuid.c.

--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -261,6 +261,7 @@ int libxl_cpuid_parse_config(libxl_cpuid
         {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
         {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
         {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
+        {"ppin",         0x80000008, NA, CPUID_REG_EBX, 23,  1},
 
         {"nc",           0x80000008, NA, CPUID_REG_ECX,  0,  8},
         {"apicidsize",   0x80000008, NA, CPUID_REG_ECX, 12,  4},
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -150,6 +150,8 @@ static const char *const str_e8b[32] =
     /* [ 8] */            [ 9] = "wbnoinvd",
 
     [12] = "ibpb",
+
+    /* [22] */                 [23] = "ppin",
 };
 
 static const char *const str_7d0[32] =
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -35,6 +35,7 @@ bool __read_mostly mce_broadcast;
 bool is_mc_panic;
 DEFINE_PER_CPU_READ_MOSTLY(unsigned int, nr_mce_banks);
 unsigned int __read_mostly firstbank;
+unsigned int __read_mostly ppin_msr;
 uint8_t __read_mostly cmci_apic_vector;
 
 DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, poll_bankmask);
@@ -999,10 +1000,17 @@ static void do_mc_get_cpu_info(void *v)
     /*
      * This part needs to run on the CPU itself.
      */
-    xcp->mc_nmsrvals = __MC_NMSRS;
+    xcp->mc_nmsrvals = 1;
     xcp->mc_msrvalues[0].reg = MSR_IA32_MCG_CAP;
     rdmsrl(MSR_IA32_MCG_CAP, xcp->mc_msrvalues[0].value);
 
+    if ( ppin_msr && xcp->mc_nmsrvals < ARRAY_SIZE(xcp->mc_msrvalues) )
+    {
+        xcp->mc_msrvalues[xcp->mc_nmsrvals].reg = ppin_msr;
+        rdmsrl(ppin_msr, xcp->mc_msrvalues[xcp->mc_nmsrvals].value);
+        ++xcp->mc_nmsrvals;
+    }
+
     if ( c->cpuid_level >= 1 )
     {
         cpuid(1, &junk, &ebx, &junk, &junk);
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -49,6 +49,7 @@ enum mcheck_type intel_mcheck_init(struc
 void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c);
 
 extern unsigned int firstbank;
+extern unsigned int ppin_msr;
 
 struct mcinfo_extended *intel_get_extended_msrs(
     struct mcinfo_global *mig, struct mc_info *mi);
--- a/xen/arch/x86/cpu/mcheck/mce_amd.c
+++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
@@ -315,6 +315,26 @@ amd_mcheck_init(struct cpuinfo_x86 *ci)
     if ( quirkflag == MCEQUIRK_F10_GART )
         mcequirk_amd_apply(quirkflag);
 
+    if ( cpu_has(ci, X86_FEATURE_AMD_PPIN) &&
+         (ci == &boot_cpu_data || ppin_msr) )
+    {
+        uint64_t val;
+
+        rdmsrl(MSR_AMD_PPIN_CTL, val);
+
+        /* If PPIN is disabled, but not locked, try to enable. */
+        if ( !(val & (PPIN_ENABLE | PPIN_LOCKOUT)) )
+        {
+            wrmsr_safe(MSR_PPIN_CTL, val | PPIN_ENABLE);
+            rdmsrl(MSR_AMD_PPIN_CTL, val);
+        }
+
+        if ( (val & (PPIN_ENABLE | PPIN_LOCKOUT)) != PPIN_ENABLE )
+            ppin_msr = 0;
+        else if ( ci == &boot_cpu_data )
+            ppin_msr = MSR_AMD_PPIN;
+    }
+
     x86_mce_callback_register(amd_f10_handler);
     mce_recoverable_register(mc_amd_recoverable_scan);
     mce_register_addrcheck(mc_amd_addrcheck);
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -853,6 +853,43 @@ static void intel_init_mce(void)
     mce_uhandler_num = ARRAY_SIZE(intel_mce_uhandlers);
 }
 
+static void intel_init_ppin(const struct cpuinfo_x86 *c)
+{
+    /*
+     * Even if testing the presence of the MSR would be enough, we don't
+     * want to risk the situation where other models reuse this MSR for
+     * other purposes.
+     */
+    switch ( c->x86_model )
+    {
+        uint64_t val;
+
+    case 0x3e: /* IvyBridge X */
+    case 0x3f: /* Haswell X */
+    case 0x4f: /* Broadwell X */
+    case 0x55: /* Skylake X */
+    case 0x56: /* Broadwell Xeon D */
+    case 0x57: /* Knights Landing */
+    case 0x85: /* Knights Mill */
+
+        if ( (c != &boot_cpu_data && !ppin_msr) ||
+             rdmsr_safe(MSR_PPIN_CTL, val) )
+            return;
+
+        /* If PPIN is disabled, but not locked, try to enable. */
+        if ( !(val & (PPIN_ENABLE | PPIN_LOCKOUT)) )
+        {
+            wrmsr_safe(MSR_PPIN_CTL, val | PPIN_ENABLE);
+            rdmsr_safe(MSR_PPIN_CTL, val);
+        }
+
+        if ( (val & (PPIN_ENABLE | PPIN_LOCKOUT)) != PPIN_ENABLE )
+            ppin_msr = 0;
+        else if ( c == &boot_cpu_data )
+            ppin_msr = MSR_PPIN;
+    }
+}
+
 static void cpu_mcabank_free(unsigned int cpu)
 {
     struct mca_banks *cmci = per_cpu(no_cmci_banks, cpu);
@@ -941,6 +978,8 @@ enum mcheck_type intel_mcheck_init(struc
 
     intel_init_thermal(c);
 
+    intel_init_ppin(c);
+
     return mcheck_intel;
 }
 
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -45,6 +45,13 @@
 #define MSR_PRED_CMD			0x00000049
 #define PRED_CMD_IBPB			(_AC(1, ULL) << 0)
 
+/* Intel Protected Processor Inventory Number */
+#define MSR_PPIN_CTL			0x0000004e
+#define MSR_PPIN			0x0000004f
+
+#define PPIN_LOCKOUT			(_AC(1, ULL) << 0)
+#define PPIN_ENABLE			(_AC(1, ULL) << 1)
+
 #define MSR_ARCH_CAPABILITIES		0x0000010a
 #define ARCH_CAPS_RDCL_NO		(_AC(1, ULL) << 0)
 #define ARCH_CAPS_IBRS_ALL		(_AC(1, ULL) << 1)
@@ -278,6 +285,10 @@
 #define MSR_AMD_OSVW_ID_LENGTH          0xc0010140
 #define MSR_AMD_OSVW_STATUS             0xc0010141
 
+/* AMD Protected Processor Inventory Number */
+#define MSR_AMD_PPIN_CTL                0xc00102f0
+#define MSR_AMD_PPIN                    0xc00102f1
+
 /* K6 MSRs */
 #define MSR_K6_EFER			0xc0000080
 #define MSR_K6_STAR			0xc0000081
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -247,6 +247,7 @@ XEN_CPUFEATURE(RDPRU,         8*32+ 4) /
 XEN_CPUFEATURE(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A  (F)X{SAVE,RSTOR} always saves/restores FPU Error pointers */
 XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*   WBNOINVD instruction */
 XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by AMD) */
+XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory Number */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
 XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A  AVX512 Neural Network Instructions */
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -246,7 +246,9 @@ typedef struct mc_info mc_info_t;
 DEFINE_XEN_GUEST_HANDLE(mc_info_t);
 
 #define __MC_MSR_ARRAYSIZE 8
+#if __XEN_INTERFACE_VERSION__ <= 0x00040d00
 #define __MC_NMSRS 1
+#endif
 #define MC_NCAPS	7	/* 7 CPU feature flag words */
 #define MC_CAPS_STD_EDX	0	/* cpuid level 0x00000001 (%edx) */
 #define MC_CAPS_AMD_EDX	1	/* cpuid level 0x80000001 (%edx) */
--- a/xen/include/public/xen-compat.h
+++ b/xen/include/public/xen-compat.h
@@ -27,7 +27,7 @@
 #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
 #define __XEN_PUBLIC_XEN_COMPAT_H__
 
-#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040d00
+#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040e00
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 /* Xen is built with matching headers and implements the latest interface. */


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

Re: [Xen-devel] [PATCH v2 1/3] x86: include the PPIN in MCE records when available

Posted by Andrew Cooper 30 weeks ago
On 08/11/2019 15:23, Jan Beulich wrote:
> Quoting the respective Linux commit:
>
>     Intel Xeons from Ivy Bridge onwards support a processor identification
>     number set in the factory. To the user this is a handy unique number to
>     identify a particular CPU. Intel can decode this to the fab/production
>     run to track errors. On systems that have it, include it in the machine
>     check record. I'm told that this would be helpful for users that run
>     large data centers with multi-socket servers to keep track of which CPUs
>     are seeing errors.
>
> Newer AMD CPUs support this too, at different MSR numbers.
>
> Take the opportunity and hide __MC_NMSRS from the public interface going
> forward.
>
> [Linux commit 3f5a7896a5096fd50030a04d4c3f28a7441e30a5]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Jan Beulich 35 weeks ago
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>
---
v2: Use "cp" consistently. Re-base.

--- 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 ( cp->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;
 


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

Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Andrew Cooper 30 weeks ago
On 08/11/2019 15:24, 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.

Why not call it as it is?  They leak through due to bugs in MSR handling.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Use "cp" consistently. Re-base.
>
> --- 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 ( cp->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;
> +

The "not offered to guests" blocks should always be symmetric.  What
you've done here is half-virtualise something we have no intention to
ever virtualise for guests.

Both of these should be blanket #GP faults.  AMD should never be in the
position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
while Intel does have model specific behaviour, whatever else might be
behind that MSR obviously shouldn't be leaking though either.

>          /*
>           * 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:

... these should be in the lower block, as "not offered to guests" is
logically different from "we virtualise them, but they are read only".

~Andrew

>          /* 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;
>  
>


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

Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Jan Beulich 29 weeks ago
On 13.12.2019 20:47, Andrew Cooper wrote:
> On 08/11/2019 15:24, 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.
> 
> Why not call it as it is?  They leak through due to bugs in MSR handling.
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Use "cp" consistently. Re-base.
>>
>> --- 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 ( cp->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;
>> +
> 
> The "not offered to guests" blocks should always be symmetric.

Well, the wrmsr side of things simply was the wrong way round
(see below) - with it flipped they then are symmetric.

>  What
> you've done here is half-virtualise something we have no intention to
> ever virtualise for guests.
> 
> Both of these should be blanket #GP faults.  AMD should never be in the
> position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
> while Intel does have model specific behaviour, whatever else might be
> behind that MSR obviously shouldn't be leaking though either.

In the interest of getting this ack-ed I might switch to the
blanket-#GP as you suggest, but I'm having trouble seeing why
giving back sane (and safe) values is wrong. This isn't meant
to indicate we might virtualize more of this. But why incur an
unnecessary #GP(0) in the guest when we can indicate the same
in a more "friendly" manner?

>> @@ -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:
> 
> ... these should be in the lower block, as "not offered to guests" is
> logically different from "we virtualise them, but they are read only".

Hmm, yes, I got these and ...

>>          /* 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;

... these the wrong way round.

Jan

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

Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Andrew Cooper 29 weeks ago
On 16/12/2019 11:47, Jan Beulich wrote:
>>   What
>> you've done here is half-virtualise something we have no intention to
>> ever virtualise for guests.
>>
>> Both of these should be blanket #GP faults.  AMD should never be in the
>> position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
>> while Intel does have model specific behaviour, whatever else might be
>> behind that MSR obviously shouldn't be leaking though either.
> In the interest of getting this ack-ed I might switch to the
> blanket-#GP as you suggest, but I'm having trouble seeing why
> giving back sane (and safe) values is wrong. This isn't meant
> to indicate we might virtualize more of this. But why incur an
> unnecessary #GP(0) in the guest when we can indicate the same
> in a more "friendly" manner?

Why add dead code to Xen?

It is unnecessary complexity in some already-complicated functions which
are going to become far more complicated by the time we get Xen's MSR
behaviour into a somewhat-reasonable state.

~Andrew

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

Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Jan Beulich 29 weeks ago
On 16.12.2019 13:26, Andrew Cooper wrote:
> On 16/12/2019 11:47, Jan Beulich wrote:
>>>   What
>>> you've done here is half-virtualise something we have no intention to
>>> ever virtualise for guests.
>>>
>>> Both of these should be blanket #GP faults.  AMD should never be in the
>>> position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
>>> while Intel does have model specific behaviour, whatever else might be
>>> behind that MSR obviously shouldn't be leaking though either.
>> In the interest of getting this ack-ed I might switch to the
>> blanket-#GP as you suggest, but I'm having trouble seeing why
>> giving back sane (and safe) values is wrong. This isn't meant
>> to indicate we might virtualize more of this. But why incur an
>> unnecessary #GP(0) in the guest when we can indicate the same
>> in a more "friendly" manner?
> 
> Why add dead code to Xen?

Well, as you said yourself - at least the Intel part of this
isn't really dead. Of course we _expect_ guest kernels to cope
with #GP here, but there are many expectations of ours which
get violated ... (To give a concrete example in this very area
of code: A customer of ours noticed the x2APIC MSRs having
become inaccessible at some point. Clearly we didn't expect PV
guests to try to access them.)

> It is unnecessary complexity in some already-complicated functions which
> are going to become far more complicated by the time we get Xen's MSR
> behaviour into a somewhat-reasonable state.

If this was really about "complexity", I'd fully agree. But
we talk about two instance of almost the same 5-line piece of
code.

Jan

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

Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

Posted by Andrew Cooper 29 weeks ago
On 16/12/2019 12:37, Jan Beulich wrote:
> On 16.12.2019 13:26, Andrew Cooper wrote:
>> On 16/12/2019 11:47, Jan Beulich wrote:
>>>>   What
>>>> you've done here is half-virtualise something we have no intention to
>>>> ever virtualise for guests.
>>>>
>>>> Both of these should be blanket #GP faults.  AMD should never be in the
>>>> position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
>>>> while Intel does have model specific behaviour, whatever else might be
>>>> behind that MSR obviously shouldn't be leaking though either.
>>> In the interest of getting this ack-ed I might switch to the
>>> blanket-#GP as you suggest, but I'm having trouble seeing why
>>> giving back sane (and safe) values is wrong. This isn't meant
>>> to indicate we might virtualize more of this. But why incur an
>>> unnecessary #GP(0) in the guest when we can indicate the same
>>> in a more "friendly" manner?
>> Why add dead code to Xen?
> Well, as you said yourself - at least the Intel part of this
> isn't really dead. Of course we _expect_ guest kernels to cope
> with #GP here, but there are many expectations of ours which
> get violated ... (To give a concrete example in this very area
> of code: A customer of ours noticed the x2APIC MSRs having
> become inaccessible at some point. Clearly we didn't expect PV
> guests to try to access them.)

This is a concrete example proves my point.

We would never have been in this mess to begin with if Xen didn't have
broken MSR handling.  It was an information leak (at best) to offer PV
guests even read-only access to the LAPIC.

The rest is incorrect pv-ops in Linux, caused by the fact that things
which shouldn't leak through, leaked through.  This breakage is still
present - a PV guest has no business accessing MSR_APIC_BASE, let alone
depending on its contents.

>
>> It is unnecessary complexity in some already-complicated functions which
>> are going to become far more complicated by the time we get Xen's MSR
>> behaviour into a somewhat-reasonable state.
> If this was really about "complexity", I'd fully agree. But
> we talk about two instance of almost the same 5-line piece of
> code.

We are talking about complexity.  I said "of the function", not of the
individual case blocks.

~Andrew

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

[Xen-devel] [PATCH v2 3/3] x86: provide Dom0 access to PPIN via XENPF_resource_op

Posted by Jan Beulich 35 weeks ago
It was requested that we provide a way independent of the MCE reporting
interface that Dom0 software could use to get hold of the values for
particular CPUs.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

TBD: I couldn't identify any suitable utility under tools/misc/ that I
     would have felt like making the frontend of this extension.

--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -30,6 +30,7 @@
 #include <asm/mtrr.h>
 #include <asm/io_apic.h>
 #include <asm/setup.h>
+#include "cpu/mcheck/mce.h"
 #include "cpu/mtrr/mtrr.h"
 #include <xsm/xsm.h>
 
@@ -94,6 +95,9 @@ void check_resource_access(struct resour
         switch ( entry->u.cmd )
         {
         case XEN_RESOURCE_OP_MSR_READ:
+            if ( ppin_msr && entry->idx == ppin_msr )
+                break;
+            /* fall through */
         case XEN_RESOURCE_OP_MSR_WRITE:
             if ( entry->idx >> 32 )
                 ret = -EINVAL;


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

Re: [Xen-devel] [PATCH v2 3/3] x86: provide Dom0 access to PPIN via XENPF_resource_op

Posted by Andrew Cooper 30 weeks ago
On 08/11/2019 15:24, Jan Beulich wrote:
> It was requested that we provide a way independent of the MCE reporting
> interface that Dom0 software could use to get hold of the values for
> particular CPUs.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This hypercall is a total gross bodge, but I suppose it is the least bad
place to add more bodges.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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