vpci: fix updating the command register

When switching the memory decoding bit in the command register the
rest of the changes where dropped, leading to only the memory decoding
bit being updated.

Fix this by writing the command register once the guest physmap
manipulations are done if there are changes to the memory decoding
bit.

Note that when only mapping/unmapping the ROM BAR a fabricated command
register value is passed to modify_bars which is only used to signal
whether the action is a mapping or unmapping, but the value is never
written to the device command register. Turn the maodify_decoding
ASSERT into an ASSERT_UNREACHABLE and make sure that non-debug builds
won't end up writing to the command register if only modifying the ROM
BAR.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4573ccadf0a83f00ea03f5f061e957efc783276b..39dffb21fb641ec89aa864f125c27181cda66792 100644 (file)
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -81,11 +81,12 @@ static int map_range(unsigned long s, unsigned long e, void *data,
  * BAR's enable bit has changed with the memory decoding bit already enabled.
  * If rom_only is not set then it's the memory decoding bit that changed.
  */
-static void modify_decoding(const struct pci_dev *pdev, bool map, bool rom_only)
+static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
+                            bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
-    uint16_t cmd;
+    bool map = cmd & PCI_COMMAND_MEMORY;
     unsigned int i;
 
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
@@ -110,12 +111,11 @@ static void modify_decoding(const struct pci_dev *pdev, bool map, bool rom_only)
             header->bars[i].enabled = map;
     }
 
-    ASSERT(!rom_only);
-    cmd = pci_conf_read16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND);
-    cmd &= ~PCI_COMMAND_MEMORY;
-    cmd |= map ? PCI_COMMAND_MEMORY : 0;
-    pci_conf_write16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND,
-                     cmd);
+    if ( !rom_only )
+        pci_conf_write16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND,
+                         cmd);
+    else
+        ASSERT_UNREACHABLE();
 }
 
 bool vpci_process_pending(struct vcpu *v)
@@ -124,7 +124,7 @@ bool vpci_process_pending(struct vcpu *v)
     {
         struct map_data data = {
             .d = v->domain,
-            .map = v->vpci.map,
+            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
         };
         int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
 
@@ -133,7 +133,8 @@ bool vpci_process_pending(struct vcpu *v)
 
         spin_lock(&v->vpci.pdev->vpci->lock);
         /* Disable memory decoding unconditionally on failure. */
-        modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
+        modify_decoding(v->vpci.pdev,
+                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
                         !rc && v->vpci.rom_only);
         spin_unlock(&v->vpci.pdev->vpci->lock);
 
@@ -154,7 +155,7 @@ bool vpci_process_pending(struct vcpu *v)
 }
 
 static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
-                            struct rangeset *mem)
+                            struct rangeset *mem, uint16_t cmd)
 {
     struct map_data data = { .d = d, .map = true };
     int rc;
@@ -163,13 +164,13 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         process_pending_softirqs();
     rangeset_destroy(mem);
     if ( !rc )
-        modify_decoding(pdev, true, false);
+        modify_decoding(pdev, cmd, false);
 
     return rc;
 }
 
 static void defer_map(struct domain *d, struct pci_dev *pdev,
-                      struct rangeset *mem, bool map, bool rom_only)
+                      struct rangeset *mem, uint16_t cmd, bool rom_only)
 {
     struct vcpu *curr = current;
 
@@ -181,11 +182,11 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
      */
     curr->vpci.pdev = pdev;
     curr->vpci.mem = mem;
-    curr->vpci.map = map;
+    curr->vpci.cmd = cmd;
     curr->vpci.rom_only = rom_only;
 }
 
-static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
+static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct rangeset *mem = rangeset_new(NULL, NULL, 0);
@@ -305,11 +306,11 @@ static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
          * be called iff the memory decoding bit is enabled, thus the operation
          * will always be to establish mappings and process all the BARs.
          */
-        ASSERT(map && !rom_only);
-        return apply_map(pdev->domain, pdev, mem);
+        ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
+        return apply_map(pdev->domain, pdev, mem, cmd);
     }
 
-    defer_map(dev->domain, dev, mem, map, rom_only);
+    defer_map(dev->domain, dev, mem, cmd, rom_only);
 
     return 0;
 }
@@ -332,7 +333,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
          * memory decoding bit has not been changed, so leave everything as-is,
          * hoping the guest will realize and try again.
          */
-        modify_bars(pdev, cmd & PCI_COMMAND_MEMORY, false);
+        modify_bars(pdev, cmd, false);
     else
         pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
 }
@@ -413,7 +414,11 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
         header->rom_enabled = new_enabled;
         pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
     }
-    else if ( modify_bars(pdev, new_enabled, true) )
+    /*
+     * Pass PCI_COMMAND_MEMORY or 0 to signal a map/unmap request, note that
+     * this fabricated command is never going to be written to the register.
+     */
+    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
         /*
          * No memory has been added or removed from the p2m (because the actual
          * p2m changes are deferred in defer_map) and the ROM enable bit has
@@ -549,7 +554,7 @@ static int init_bars(struct pci_dev *pdev)
             rom->type = VPCI_BAR_EMPTY;
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
 }
 REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
 

diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index af2b8580ee793ac3f6934464549d261e42a383bc..44104b75b6232ead07338d81fbff527b388b6031 100644 (file)
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -145,7 +145,7 @@ struct vpci_vcpu {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
     struct rangeset *mem;
     struct pci_dev *pdev;
-    bool map      : 1;
+    uint16_t cmd;
     bool rom_only : 1;
 };