[PATCH] x86: synchronize PCI config space access decoding

[PATCH] x86: synchronize PCI config space access decoding

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5055 bytes --]

Both PV and HVM logic have similar but not similar enough code here.
Synchronize the two so that
- in the HVM case we don't unconditionally try to access extended
  config space
- in the PV case we pass a correct range to the XSM hook
- in the PV case we don't needlessly deny access when the operation
  isn't really on PCI config space
All this along with sharing the macros HVM already had here.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
 static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
                                                         ioreq_t *p)
 {
-#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
-#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
-#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
-#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
-
     struct hvm_ioreq_server *s;
     uint32_t cf8;
     uint8_t type;
@@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
 
         type = IOREQ_TYPE_PCI_CONFIG;
         addr = ((uint64_t)sbdf << 32) |
-               CF8_ADDR_HI(cf8) |
                CF8_ADDR_LO(cf8) |
                (p->addr & 3);
+        /* AMD extended configuration space access? */
+        if ( CF8_ADDR_HI(cf8) &&
+             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
+        {
+            uint64_t msr_val;
+
+            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
+                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
+                addr |= CF8_ADDR_HI(cf8);
+        }
     }
     else
     {
@@ -2472,11 +2477,6 @@ static struct hvm_ioreq_server *hvm_sele
     }
 
     return d->arch.hvm_domain.default_ioreq_server;
-
-#undef CF8_ADDR_ENABLED
-#undef CF8_ADDR_HI
-#undef CF8_ADDR_LO
-#undef CF8_BDF
 }
 
 int hvm_buffered_io_send(ioreq_t *p)
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1770,14 +1770,19 @@ static int admin_io_okay(
     return ioports_access_permitted(v->domain, port, port + bytes - 1);
 }
 
-static int pci_cfg_ok(struct domain *d, int write, int size)
+static bool_t pci_cfg_ok(struct domain *d, bool_t write,
+                         uint16_t start, uint16_t size)
 {
     uint32_t machine_bdf;
-    uint16_t start, end;
-    if (!is_hardware_domain(d))
+    uint16_t end;
+
+    if ( !is_hardware_domain(d) )
         return 0;
 
-    machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
+    if ( !CF8_ENABLED(d->arch.pci_cf8) )
+        return 1;
+
+    machine_bdf = CF8_BDF(d->arch.pci_cf8);
     if ( write )
     {
         const unsigned long *ro_map = pci_get_ro_map(0);
@@ -1785,9 +1790,9 @@ static int pci_cfg_ok(struct domain *d, 
         if ( ro_map && test_bit(machine_bdf, ro_map) )
             return 0;
     }
-    start = d->arch.pci_cf8 & 0xFF;
+    start |= CF8_ADDR_LO(d->arch.pci_cf8);
     /* AMD extended configuration space access? */
-    if ( (d->arch.pci_cf8 & 0x0F000000) &&
+    if ( CF8_ADDR_HI(d->arch.pci_cf8) &&
          boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
          boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
     {
@@ -1796,7 +1801,7 @@ static int pci_cfg_ok(struct domain *d, 
         if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
             return 0;
         if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
-            start |= (d->arch.pci_cf8 >> 16) & 0xF00;
+            start |= CF8_ADDR_HI(d->arch.pci_cf8);
     }
     end = start + size - 1;
     if (xsm_pci_config_permission(XSM_HOOK, d, machine_bdf, start, end, write))
@@ -1855,7 +1860,7 @@ uint32_t guest_io_read(
             size = min(bytes, 4 - (port & 3));
             if ( size == 3 )
                 size = 2;
-            if ( pci_cfg_ok(v->domain, 0, size) )
+            if ( pci_cfg_ok(v->domain, 0, port & 3, size) )
                 sub_data = pci_conf_read(v->domain->arch.pci_cf8, port & 3, size);
         }
 
@@ -1928,7 +1933,7 @@ void guest_io_write(
             size = min(bytes, 4 - (port & 3));
             if ( size == 3 )
                 size = 2;
-            if ( pci_cfg_ok(v->domain, 1, size) )
+            if ( pci_cfg_ok(v->domain, 1, port & 3, size) )
                 pci_conf_write(v->domain->arch.pci_cf8, port & 3, size, data);
         }
 
--- a/xen/include/asm-x86/pci.h
+++ b/xen/include/asm-x86/pci.h
@@ -1,6 +1,11 @@
 #ifndef __X86_PCI_H__
 #define __X86_PCI_H__
 
+#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
+#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
+#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
+#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
+
 #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
                         || id == 0x01268086 || id == 0x01028086 \
                         || id == 0x01128086 || id == 0x01228086 \



[-- Attachment #2: x86-CF8-decode.patch --]
[-- Type: text/plain, Size: 5104 bytes --]

x86: synchronize PCI config space access decoding

Both PV and HVM logic have similar but not similar enough code here.
Synchronize the two so that
- in the HVM case we don't unconditionally try to access extended
  config space
- in the PV case we pass a correct range to the XSM hook
- in the PV case we don't needlessly deny access when the operation
  isn't really on PCI config space
All this along with sharing the macros HVM already had here.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
 static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
                                                         ioreq_t *p)
 {
-#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
-#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
-#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
-#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
-
     struct hvm_ioreq_server *s;
     uint32_t cf8;
     uint8_t type;
@@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
 
         type = IOREQ_TYPE_PCI_CONFIG;
         addr = ((uint64_t)sbdf << 32) |
-               CF8_ADDR_HI(cf8) |
                CF8_ADDR_LO(cf8) |
                (p->addr & 3);
+        /* AMD extended configuration space access? */
+        if ( CF8_ADDR_HI(cf8) &&
+             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
+        {
+            uint64_t msr_val;
+
+            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
+                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
+                addr |= CF8_ADDR_HI(cf8);
+        }
     }
     else
     {
@@ -2472,11 +2477,6 @@ static struct hvm_ioreq_server *hvm_sele
     }
 
     return d->arch.hvm_domain.default_ioreq_server;
-
-#undef CF8_ADDR_ENABLED
-#undef CF8_ADDR_HI
-#undef CF8_ADDR_LO
-#undef CF8_BDF
 }
 
 int hvm_buffered_io_send(ioreq_t *p)
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1770,14 +1770,19 @@ static int admin_io_okay(
     return ioports_access_permitted(v->domain, port, port + bytes - 1);
 }
 
-static int pci_cfg_ok(struct domain *d, int write, int size)
+static bool_t pci_cfg_ok(struct domain *d, bool_t write,
+                         uint16_t start, uint16_t size)
 {
     uint32_t machine_bdf;
-    uint16_t start, end;
-    if (!is_hardware_domain(d))
+    uint16_t end;
+
+    if ( !is_hardware_domain(d) )
         return 0;
 
-    machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
+    if ( !CF8_ENABLED(d->arch.pci_cf8) )
+        return 1;
+
+    machine_bdf = CF8_BDF(d->arch.pci_cf8);
     if ( write )
     {
         const unsigned long *ro_map = pci_get_ro_map(0);
@@ -1785,9 +1790,9 @@ static int pci_cfg_ok(struct domain *d, 
         if ( ro_map && test_bit(machine_bdf, ro_map) )
             return 0;
     }
-    start = d->arch.pci_cf8 & 0xFF;
+    start |= CF8_ADDR_LO(d->arch.pci_cf8);
     /* AMD extended configuration space access? */
-    if ( (d->arch.pci_cf8 & 0x0F000000) &&
+    if ( CF8_ADDR_HI(d->arch.pci_cf8) &&
          boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
          boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
     {
@@ -1796,7 +1801,7 @@ static int pci_cfg_ok(struct domain *d, 
         if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
             return 0;
         if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
-            start |= (d->arch.pci_cf8 >> 16) & 0xF00;
+            start |= CF8_ADDR_HI(d->arch.pci_cf8);
     }
     end = start + size - 1;
     if (xsm_pci_config_permission(XSM_HOOK, d, machine_bdf, start, end, write))
@@ -1855,7 +1860,7 @@ uint32_t guest_io_read(
             size = min(bytes, 4 - (port & 3));
             if ( size == 3 )
                 size = 2;
-            if ( pci_cfg_ok(v->domain, 0, size) )
+            if ( pci_cfg_ok(v->domain, 0, port & 3, size) )
                 sub_data = pci_conf_read(v->domain->arch.pci_cf8, port & 3, size);
         }
 
@@ -1928,7 +1933,7 @@ void guest_io_write(
             size = min(bytes, 4 - (port & 3));
             if ( size == 3 )
                 size = 2;
-            if ( pci_cfg_ok(v->domain, 1, size) )
+            if ( pci_cfg_ok(v->domain, 1, port & 3, size) )
                 pci_conf_write(v->domain->arch.pci_cf8, port & 3, size, data);
         }
 
--- a/xen/include/asm-x86/pci.h
+++ b/xen/include/asm-x86/pci.h
@@ -1,6 +1,11 @@
 #ifndef __X86_PCI_H__
 #define __X86_PCI_H__
 
+#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
+#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
+#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
+#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
+
 #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
                         || id == 0x01268086 || id == 0x01028086 \
                         || id == 0x01128086 || id == 0x01228086 \

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: synchronize PCI config space access decoding

[Andrew Cooper]

On 09/03/15 16:08, Jan Beulich wrote:
> Both PV and HVM logic have similar but not similar enough code here.
> Synchronize the two so that
> - in the HVM case we don't unconditionally try to access extended
>   config space
> - in the PV case we pass a correct range to the XSM hook
> - in the PV case we don't needlessly deny access when the operation
>   isn't really on PCI config space
> All this along with sharing the macros HVM already had here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
>  static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>                                                          ioreq_t *p)
>  {
> -#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
> -#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
> -#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
> -#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
> -
>      struct hvm_ioreq_server *s;
>      uint32_t cf8;
>      uint8_t type;
> @@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
>  
>          type = IOREQ_TYPE_PCI_CONFIG;
>          addr = ((uint64_t)sbdf << 32) |
> -               CF8_ADDR_HI(cf8) |
>                 CF8_ADDR_LO(cf8) |
>                 (p->addr & 3);
> +        /* AMD extended configuration space access? */
> +        if ( CF8_ADDR_HI(cf8) &&
> +             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
> +        {
> +            uint64_t msr_val;
> +
> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> +                addr |= CF8_ADDR_HI(cf8);

This is another example of host state which leaks into guests across
migrate, but in this case is also problematic at the host level.

As far as the host goes, MSR_AMD64_NB_CFG is a per-node msr and Xen
should verify that the AMD64_NB_CFG_CF8_EXT_ENABLE_BIT is consistent
across the system, or bits of emulate_privileged_op() are liable to
execute differently depending on which pcpu a vcpu happens to be scheduled.

Beyond that, for now there should be a __read_mostly bool_t based on the
system verification, which is used in preference to reading the MSR each
time a guest does a cf8 access.

Someone with many tuits can see about making the entire guest MSR state
move in the migration stream, or I will probably get to it after getting
the cpuid work done.

~Andrew

Re: [PATCH] x86: synchronize PCI config space access decoding

[Jan Beulich]

>>> On 09.03.15 at 19:49, <andrew.cooper3@citrix.com> wrote:
> On 09/03/15 16:08, Jan Beulich wrote:
>> Both PV and HVM logic have similar but not similar enough code here.
>> Synchronize the two so that
>> - in the HVM case we don't unconditionally try to access extended
>>   config space
>> - in the PV case we pass a correct range to the XSM hook
>> - in the PV case we don't needlessly deny access when the operation
>>   isn't really on PCI config space
>> All this along with sharing the macros HVM already had here.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
>>  static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>>                                                          ioreq_t *p)
>>  {
>> -#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
>> -#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
>> -#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
>> -#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
>> -
>>      struct hvm_ioreq_server *s;
>>      uint32_t cf8;
>>      uint8_t type;
>> @@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
>>  
>>          type = IOREQ_TYPE_PCI_CONFIG;
>>          addr = ((uint64_t)sbdf << 32) |
>> -               CF8_ADDR_HI(cf8) |
>>                 CF8_ADDR_LO(cf8) |
>>                 (p->addr & 3);
>> +        /* AMD extended configuration space access? */
>> +        if ( CF8_ADDR_HI(cf8) &&
>> +             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
>> +             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
>> +        {
>> +            uint64_t msr_val;
>> +
>> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
>> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
>> +                addr |= CF8_ADDR_HI(cf8);
> 
> This is another example of host state which leaks into guests across
> migrate, but in this case is also problematic at the host level.

Yes, but cross-vendor migration has (iirc) many more issues like this
(and considering the wide family range the risk of this breaking for
migration between AMD systems seems marginal).

> As far as the host goes, MSR_AMD64_NB_CFG is a per-node msr and Xen
> should verify that the AMD64_NB_CFG_CF8_EXT_ENABLE_BIT is consistent
> across the system, or bits of emulate_privileged_op() are liable to
> execute differently depending on which pcpu a vcpu happens to be scheduled.

I think this goes too far in mistrusting Dom0.

> Beyond that, for now there should be a __read_mostly bool_t based on the
> system verification, which is used in preference to reading the MSR each
> time a guest does a cf8 access.

But it is part of the change to _not_ do the MSR access on each
CF8 one: We first check whether this at all looks like an extended
config space access. I.e. I considered eliminating the rdmsr, but
didn't consider it worthwhile for the change here.

Jan

Re: [PATCH] x86: synchronize PCI config space access decoding

[Andrew Cooper]

On 10/03/15 07:30, Jan Beulich wrote:
>>>> On 09.03.15 at 19:49, <andrew.cooper3@citrix.com> wrote:
>> On 09/03/15 16:08, Jan Beulich wrote:
>>> Both PV and HVM logic have similar but not similar enough code here.
>>> Synchronize the two so that
>>> - in the HVM case we don't unconditionally try to access extended
>>>   config space
>>> - in the PV case we pass a correct range to the XSM hook
>>> - in the PV case we don't needlessly deny access when the operation
>>>   isn't really on PCI config space
>>> All this along with sharing the macros HVM already had here.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
>>>  static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>>>                                                          ioreq_t *p)
>>>  {
>>> -#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
>>> -#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
>>> -#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
>>> -#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
>>> -
>>>      struct hvm_ioreq_server *s;
>>>      uint32_t cf8;
>>>      uint8_t type;
>>> @@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
>>>  
>>>          type = IOREQ_TYPE_PCI_CONFIG;
>>>          addr = ((uint64_t)sbdf << 32) |
>>> -               CF8_ADDR_HI(cf8) |
>>>                 CF8_ADDR_LO(cf8) |
>>>                 (p->addr & 3);
>>> +        /* AMD extended configuration space access? */
>>> +        if ( CF8_ADDR_HI(cf8) &&
>>> +             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
>>> +             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
>>> +        {
>>> +            uint64_t msr_val;
>>> +
>>> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
>>> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
>>> +                addr |= CF8_ADDR_HI(cf8);
>> This is another example of host state which leaks into guests across
>> migrate, but in this case is also problematic at the host level.
> Yes, but cross-vendor migration has (iirc) many more issues like this
> (and considering the wide family range the risk of this breaking for
> migration between AMD systems seems marginal).

I wasn't even considering cross-vendor migration, but that is another
concern.  I was more concerned with leaking bios-configured state into
the guest.

>
>> As far as the host goes, MSR_AMD64_NB_CFG is a per-node msr and Xen
>> should verify that the AMD64_NB_CFG_CF8_EXT_ENABLE_BIT is consistent
>> across the system, or bits of emulate_privileged_op() are liable to
>> execute differently depending on which pcpu a vcpu happens to be scheduled.
> I think this goes too far in mistrusting Dom0.

The only case where dom0 could plausibly set this up consistently even
if wanted to, is when it has a vcpu for each pcpu and is using
dom0_vcpu_pin.  Either of these conditions is rare in practice.

I still think it is Xen which needs to set this up consistently on boot,
at which point removing all the the rdmsr_safe() from cf8 accesses is
trivial.

~Andrew

Re: [PATCH] x86: synchronize PCI config space access decoding

[Jan Beulich]

>>> On 10.03.15 at 12:24, <andrew.cooper3@citrix.com> wrote:
> On 10/03/15 07:30, Jan Beulich wrote:
>>>>> On 09.03.15 at 19:49, <andrew.cooper3@citrix.com> wrote:
>>> On 09/03/15 16:08, Jan Beulich wrote:
>>>> Both PV and HVM logic have similar but not similar enough code here.
>>>> Synchronize the two so that
>>>> - in the HVM case we don't unconditionally try to access extended
>>>>   config space
>>>> - in the PV case we pass a correct range to the XSM hook
>>>> - in the PV case we don't needlessly deny access when the operation
>>>>   isn't really on PCI config space
>>>> All this along with sharing the macros HVM already had here.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
>>>>  static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>>>>                                                          ioreq_t *p)
>>>>  {
>>>> -#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
>>>> -#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
>>>> -#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
>>>> -#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
>>>> -
>>>>      struct hvm_ioreq_server *s;
>>>>      uint32_t cf8;
>>>>      uint8_t type;
>>>> @@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
>>>>  
>>>>          type = IOREQ_TYPE_PCI_CONFIG;
>>>>          addr = ((uint64_t)sbdf << 32) |
>>>> -               CF8_ADDR_HI(cf8) |
>>>>                 CF8_ADDR_LO(cf8) |
>>>>                 (p->addr & 3);
>>>> +        /* AMD extended configuration space access? */
>>>> +        if ( CF8_ADDR_HI(cf8) &&
>>>> +             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
>>>> +             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
>>>> +        {
>>>> +            uint64_t msr_val;
>>>> +
>>>> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
>>>> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
>>>> +                addr |= CF8_ADDR_HI(cf8);
>>> This is another example of host state which leaks into guests across
>>> migrate, but in this case is also problematic at the host level.
>> Yes, but cross-vendor migration has (iirc) many more issues like this
>> (and considering the wide family range the risk of this breaking for
>> migration between AMD systems seems marginal).
> 
> I wasn't even considering cross-vendor migration, but that is another
> concern.  I was more concerned with leaking bios-configured state into
> the guest.
> 
>>
>>> As far as the host goes, MSR_AMD64_NB_CFG is a per-node msr and Xen
>>> should verify that the AMD64_NB_CFG_CF8_EXT_ENABLE_BIT is consistent
>>> across the system, or bits of emulate_privileged_op() are liable to
>>> execute differently depending on which pcpu a vcpu happens to be scheduled.
>> I think this goes too far in mistrusting Dom0.
> 
> The only case where dom0 could plausibly set this up consistently even
> if wanted to, is when it has a vcpu for each pcpu and is using
> dom0_vcpu_pin.  Either of these conditions is rare in practice.

Did you look at Linux? In order to avoid these preconditions, I
specifically made it try via PCI config space accesses first a couple
of years ago.

> I still think it is Xen which needs to set this up consistently on boot,
> at which point removing all the the rdmsr_safe() from cf8 accesses is
> trivial.

Since Xen is not itself using this mechanism (maybe it should), it
seems wrong for it to configure it any specific way (i.e. Dom0 may
also rely on it being off). I btw also don't think the BIOS should
enable this, at least not without being told so. And if it does, then
please consistently for the whole system.

Jan