[PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

From: Roger Pau Monne <roger.pau@xxxxxxxxxx>

Deferring the actual write of the PCI register bit, either the memory
decoding or the ROM enable is not helpful, and adds an unnecessary amount of
complexity to the preemptible handling of BAR related p2m modifications.

In the hardware domain case, whether the PCI register write is done ahead
or after the p2m changes doesn't matter, a hardware domain has plenty of
ways to mess with the PCI register state if it wants to.  Any poking at the
BAR p2m regions ahead of the guest write having completed will be
undefined.

On the other hand, for domUs the memory decoding bit shouldn't really
change as a result of guest actions, and should always be enabled.  Guest
toggling the memory decoding command register should only result in p2m
modifications, but no propagation to the device PCI registers.  Having
memory decoding unconditionally enabled ensures the domU attempting to
perform p2m accesses while the p2m changes are taking place will always
access the BAR contents. This is not the current behavior for domUs, so add
a note that it would preferably done that way.

This allows to get rid of modify_decoding(), as writing the command
register can easily be done without the need for an external helper.

Resolves: https://gitlab.com/xen-project/xen/-/issues/98
Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
---
Since the full command register value is no longer needed after p2m
operations, we could reasonably replace the cmd parameter to
modify_bars() with 'bool map' immediately after this patch. However, I
chose to wait until the end of the series to reduce the diffstat and
make review easier.

Note that in rom_write(), when rom->enabled and new_enabled are both
false, rom->addr/guest_addr will be written twice. This quirk is
preexisting, so I didn't think it was in the scope for this patch to
address it. Do you think it's worth addressing separately?

v1->v5:
* rebase
* reorder within series
* in rom_write(), retain bars_mapped / enable toggle gate for invoking
  modify_bars()
* add Resolves: tag
---
 xen/drivers/vpci/header.c | 80 +++++++++++----------------------------
 1 file changed, 22 insertions(+), 58 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index cf9d0bac8876..908adf0b3f4c 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -102,47 +102,6 @@ static int cf_check map_range(
     return rc;
 }
 
-/*
- * The rom_only parameter is used to signal the map/unmap helpers that the ROM
- * 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, uint16_t cmd,
-                            bool rom_only)
-{
-    struct vpci_header *header = &pdev->vpci->header;
-    bool map = cmd & PCI_COMMAND_MEMORY;
-    unsigned int i;
-
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-    {
-        struct vpci_bar *bar = &header->bars[i];
-
-        if ( !MAPPABLE_BAR(bar) )
-            continue;
-
-        if ( rom_only && bar->type == VPCI_BAR_ROM )
-        {
-            unsigned int rom_pos = (i == PCI_HEADER_NORMAL_NR_BARS)
-                                   ? PCI_ROM_ADDRESS : PCI_ROM_ADDRESS1;
-            uint32_t val = bar->addr |
-                           (map ? PCI_ROM_ADDRESS_ENABLE : 0);
-
-            header->rom_enabled = map;
-            pci_conf_write32(pdev->sbdf, rom_pos, val);
-            return;
-        }
-    }
-
-    if ( !rom_only )
-    {
-        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
-        header->bars_mapped = map;
-    }
-    else
-        ASSERT_UNREACHABLE();
-}
-
 bool vpci_process_pending(struct vcpu *v)
 {
     const struct pci_dev *pdev = v->vpci.pdev;
@@ -202,10 +161,6 @@ bool vpci_process_pending(struct vcpu *v)
     }
     v->vpci.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
-
     read_unlock(&v->domain->pci_lock);
 
     return false;
@@ -241,8 +196,6 @@ static int __init apply_map(struct domain *d, const struct 
pci_dev *pdev,
             write_lock(&d->pci_lock);
         }
     }
-    if ( !rc )
-        modify_decoding(pdev, cmd, false);
 
     return rc;
 }
@@ -534,22 +487,29 @@ static void cf_check cmd_write(
      * decoding one. Bits that are not allowed for DomU are already
      * handled above and by the rsvdp_mask.
      */
-    if ( header->bars_mapped != new_enabled )
+    if ( header->bars_mapped != new_enabled &&
+         modify_bars(pdev, cmd, false) )
         /*
          * Ignore the error. No memory has been added or removed from the p2m
          * (because the actual p2m changes are deferred in defer_map) and the
          * memory decoding bit has not been changed, so leave everything as-is,
          * hoping the guest will realize and try again.
          */
-        modify_bars(pdev, cmd, false);
-    else
-        pci_conf_write16(pdev->sbdf, reg, cmd);
+        return;
 
 #ifdef CONFIG_HAS_PCI_MSI
     /* Unpopulate MSI-X table region, so accesses trap into Xen. */
     if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
         return;
 #endif
+
+    /*
+     * FIXME: for domUs we don't want the guest toggling the memory decoding
+     * bit.  It should be set in vpci_init_header() and guest attempts to
+     * modify it should only lead to guest p2m changes.
+     */
+    header->bars_mapped = new_enabled;
+    pci_conf_write16(pdev->sbdf, reg, cmd);
 }
 
 static uint32_t cf_check guest_cmd_read(
@@ -705,17 +665,12 @@ static void cf_check rom_write(
         rom->guest_addr = rom->addr;
     }
 
-    if ( !header->bars_mapped || rom->enabled == new_enabled )
-    {
-        /* Just update the ROM BAR field. */
-        header->rom_enabled = new_enabled;
-        pci_conf_write32(pdev->sbdf, reg, val);
-    }
     /*
      * 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) )
+    if ( header->bars_mapped && rom->enabled != new_enabled &&
+         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
@@ -726,6 +681,8 @@ static void cf_check rom_write(
          */
         return;
 
+    header->rom_enabled = new_enabled;
+    pci_conf_write32(pdev->sbdf, reg, val);
     if ( !new_enabled )
     {
         rom->addr = val & PCI_ROM_ADDRESS_MASK;
@@ -927,6 +884,13 @@ int vpci_init_header(struct pci_dev *pdev)
             goto fail;
     }
 
+    if ( cmd & PCI_COMMAND_MEMORY )
+    {
+        /* Restore command register value. */
+        header->bars_mapped = true;
+        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+    }
+
     return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
 
  fail:
-- 
2.54.0