[Xen-devel] [PATCH] x86: synchronize PCI config space access decoding

["Jan Beulich" <JBeulich@xxxxxxxx>]

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@xxxxxxxx>

--- 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: x86-CF8-decode.patch
Description: Text document

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