[v2,22/30] x86/domctl: Update PV domain cpumasks when setting cpuid policy

Commit Message

Andrew Cooper Feb. 5, 2016, 1:42 p.m. UTC

This allows PV domains with different featuresets to observe different values
from a native cpuid instruction, on supporting hardware.

Signed-off-by: Andrew Cooper <[e-mail]>
Reviewed-by: Jan Beulich <[e-mail]>
---
v2:
 * Use switch() rather than if/elseif chain
 * Clamp to static PV featuremask
---
 xen/arch/x86/domctl.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

Comments

Jan Beulich Feb. 17, 2016, 8:22 a.m. UTC | #1

>>> On 05.02.16 at 14:42, <[e-mail]> wrote:
> @@ -87,6 +88,93 @@ static void update_domain_cpuid_info(struct domain *d,
>          d->arch.x86_model = (ctl->eax >> 4) & 0xf;
>          if ( d->arch.x86 >= 0x6 )
>              d->arch.x86_model |= (ctl->eax >> 12) & 0xf0;
> +
> +        if ( is_pv_domain(d) )

For clarity, wouldn't it be reasonable to check the respective
capability flag in all of these conditionals, even if without such
checks what gets set below simply won't ever get used? Even
more, the earlier patch allocating d->arch.pv_domain.cpuidmasks
could skip this allocation if none of the masking capability bits
are set (in which case checking the pointer to be non-NULL would
seem to be the right check here then).

Either way, Reviewed-by: Jan Beulich <[e-mail]>

Jan

Andrew Cooper Feb. 17, 2016, 12:13 p.m. UTC | #2

On 17/02/16 08:22, Jan Beulich wrote:
>>>> On 05.02.16 at 14:42, <[e-mail]> wrote:
>> @@ -87,6 +88,93 @@ static void update_domain_cpuid_info(struct domain *d,
>>          d->arch.x86_model = (ctl->eax >> 4) & 0xf;
>>          if ( d->arch.x86 >= 0x6 )
>>              d->arch.x86_model |= (ctl->eax >> 12) & 0xf0;
>> +
>> +        if ( is_pv_domain(d) )
> For clarity, wouldn't it be reasonable to check the respective
> capability flag in all of these conditionals, even if without such
> checks what gets set below simply won't ever get used? Even
> more, the earlier patch allocating d->arch.pv_domain.cpuidmasks
> could skip this allocation if none of the masking capability bits
> are set (in which case checking the pointer to be non-NULL would
> seem to be the right check here then).

Can do.

~Andrew

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 55aecdc..f06bc02 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -36,6 +36,7 @@ 
 #include <asm/xstate.h>
 #include <asm/debugger.h>
 #include <asm/psr.h>
+#include <asm/cpuid.h>
 
 static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
 {
@@ -87,6 +88,93 @@  static void update_domain_cpuid_info(struct domain *d,
         d->arch.x86_model = (ctl->eax >> 4) & 0xf;
         if ( d->arch.x86 >= 0x6 )
             d->arch.x86_model |= (ctl->eax >> 12) & 0xf0;
+
+        if ( is_pv_domain(d) )
+        {
+            uint64_t mask = cpuidmask_defaults._1cd;
+            uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_1c];
+            uint32_t edx = ctl->edx & pv_featureset[FEATURESET_1d];
+
+            switch ( boot_cpu_data.x86_vendor )
+            {
+            case X86_VENDOR_INTEL:
+                mask &= ((uint64_t)edx << 32) | ecx;
+                break;
+
+            case X86_VENDOR_AMD:
+                mask &= ((uint64_t)ecx << 32) | edx;
+                break;
+            }
+
+            d->arch.pv_domain.cpuidmasks->_1cd = mask;
+        }
+        break;
+
+    case 6:
+        if ( is_pv_domain(d) )
+        {
+            uint64_t mask = cpuidmask_defaults._6c;
+
+            if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+                mask &= (~0ULL << 32) | ctl->ecx;
+
+            d->arch.pv_domain.cpuidmasks->_6c = mask;
+        }
+        break;
+
+    case 7:
+        if ( ctl->input[1] != 0 )
+            break;
+
+        if ( is_pv_domain(d) )
+        {
+            uint64_t mask = cpuidmask_defaults._7ab0;
+            uint32_t eax = ctl->eax;
+            uint32_t ebx = ctl->ebx & pv_featureset[FEATURESET_7b0];
+
+            if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+                mask &= ((uint64_t)eax << 32) | ebx;
+
+            d->arch.pv_domain.cpuidmasks->_7ab0 = mask;
+        }
+        break;
+
+    case 0xd:
+        if ( ctl->input[1] != 1 )
+            break;
+
+        if ( is_pv_domain(d) )
+        {
+            uint64_t mask = cpuidmask_defaults.Da1;
+            uint32_t eax = ctl->eax & pv_featureset[FEATURESET_Da1];
+
+            if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+                mask &= (~0ULL << 32) | eax;
+
+            d->arch.pv_domain.cpuidmasks->Da1 = mask;
+        }
+        break;
+
+    case 0x80000001:
+        if ( is_pv_domain(d) )
+        {
+            uint64_t mask = cpuidmask_defaults.e1cd;
+            uint32_t ecx = ctl->ecx & pv_featureset[FEATURESET_e1c];
+            uint32_t edx = ctl->edx & pv_featureset[FEATURESET_e1d];
+
+            switch ( boot_cpu_data.x86_vendor )
+            {
+            case X86_VENDOR_INTEL:
+                mask &= ((uint64_t)edx << 32) | ecx;
+                break;
+
+            case X86_VENDOR_AMD:
+                mask &= ((uint64_t)ecx << 32) | edx;
+                break;
+            }
+
+            d->arch.pv_domain.cpuidmasks->e1cd = mask;
+        }
         break;
     }
 }