[PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding handling

[PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding handling

[Roger Pau Monne]

Hello,

This patch series attempts to fix the regressions caused by 75cc460a1b
('xen/pci: detect when BARs are not suitably positioned') and the last
patch relaxes the check done when attempting to write to BARs with
memory decoding enabled.

I consider all of them bug fixes, albeit the last patch is not fixing a
regression (since vPCI code has always behaved this way).

Thanks, Roger.

Roger Pau Monne (5):
  vpci: don't assume that vpci per-device data exists unconditionally
  vpci/msix: remove from table list on detach
  vpci: introduce a local vpci_bar variable to modify_decoding()
  pci: do not disable memory decoding for devices
  vpci: refuse BAR writes only if the BAR is mapped

 xen/drivers/passthrough/pci.c | 69 -----------------------------------
 xen/drivers/vpci/header.c     | 52 ++++++++++++++++++--------
 xen/drivers/vpci/vpci.c       | 14 ++++---
 xen/include/xen/vpci.h        |  6 +++
 4 files changed, 52 insertions(+), 89 deletions(-)

-- 
2.37.3

[PATCH for-4.17 v2 1/5] vpci: don't assume that vpci per-device data exists unconditionally

[Roger Pau Monne]

It's possible for a device to be assigned to a domain but have no
vpci structure if vpci_process_pending() failed and called
vpci_remove_device() as a result.  The unconditional accesses done by
vpci_{read,write}() and vpci_remove_device() to pdev->vpci would
then trigger a NULL pointer dereference.

Add checks for pdev->vpci presence in the affected functions.

Fixes: 9c244fdef7 ('vpci: add header handlers')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/vpci/vpci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 3467c0de86..647f7af679 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -37,7 +37,7 @@ extern vpci_register_init_t *const __end_vpci_array[];
 
 void vpci_remove_device(struct pci_dev *pdev)
 {
-    if ( !has_vpci(pdev->domain) )
+    if ( !has_vpci(pdev->domain) || !pdev->vpci )
         return;
 
     spin_lock(&pdev->vpci->lock);
@@ -326,7 +326,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
 
     /* Find the PCI dev matching the address. */
     pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev )
+    if ( !pdev || !pdev->vpci )
         return vpci_read_hw(sbdf, reg, size);
 
     spin_lock(&pdev->vpci->lock);
@@ -436,7 +436,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
      * Passthrough everything that's not trapped.
      */
     pdev = pci_get_pdev(d, sbdf);
-    if ( !pdev )
+    if ( !pdev || !pdev->vpci )
     {
         vpci_write_hw(sbdf, reg, size, data);
         return;
-- 
2.37.3

[PATCH for-4.17 v2 2/5] vpci/msix: remove from table list on detach

[Roger Pau Monne]

Teardown of MSIX vPCI related data doesn't currently remove the MSIX
device data from the list of MSIX tables handled by the domain,
leading to a use-after-free of the data in the msix structure.

Remove the structure from the list before freeing in order to solve
it.

Reported-by: Jan Beulich <jbeulich@suse.com>
Fixes: d6281be9d0 ('vpci/msix: add MSI-X handlers')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/vpci/vpci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 647f7af679..98198dc2c9 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -51,8 +51,12 @@ void vpci_remove_device(struct pci_dev *pdev)
         xfree(r);
     }
     spin_unlock(&pdev->vpci->lock);
-    if ( pdev->vpci->msix && pdev->vpci->msix->pba )
-        iounmap(pdev->vpci->msix->pba);
+    if ( pdev->vpci->msix )
+    {
+        list_del(&pdev->vpci->msix->next);
+        if ( pdev->vpci->msix->pba )
+            iounmap(pdev->vpci->msix->pba);
+    }
     xfree(pdev->vpci->msix);
     xfree(pdev->vpci->msi);
     xfree(pdev->vpci);
-- 
2.37.3

[PATCH for-4.17 v2 3/5] vpci: introduce a local vpci_bar variable to modify_decoding()

[Roger Pau Monne]

This is done to shorten line length in the function in preparation for
adding further usages of the vpci_bar data structure.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/vpci/header.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index a1c928a0d2..eb9219a49a 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -103,24 +103,26 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
-        if ( !MAPPABLE_BAR(&header->bars[i]) )
+        struct vpci_bar *bar = &header->bars[i];
+
+        if ( !MAPPABLE_BAR(bar) )
             continue;
 
-        if ( rom_only && header->bars[i].type == VPCI_BAR_ROM )
+        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 = header->bars[i].addr |
+            uint32_t val = bar->addr |
                            (map ? PCI_ROM_ADDRESS_ENABLE : 0);
 
-            header->bars[i].enabled = header->rom_enabled = map;
+            bar->enabled = header->rom_enabled = map;
             pci_conf_write32(pdev->sbdf, rom_pos, val);
             return;
         }
 
         if ( !rom_only &&
-             (header->bars[i].type != VPCI_BAR_ROM || header->rom_enabled) )
-            header->bars[i].enabled = map;
+             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
+            bar->enabled = map;
     }
 
     if ( !rom_only )
-- 
2.37.3

[PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Roger Pau Monne]

Commit 75cc460a1b added checks to ensure the position of the BARs from
PCI devices don't overlap with regions defined on the memory map.
When there's a collision memory decoding is left disabled for the
device, assuming that dom0 will reposition the BAR if necessary and
enable memory decoding.

While this would be the case for devices being used by dom0, devices
being used by the firmware itself that have no driver would usually be
left with memory decoding disabled by dom0 if that's the state dom0
found them in, and thus firmware trying to make use of them will not
function correctly.

The initial intent of 75cc460a1b was to prevent vPCI from creating
MMIO mappings on the dom0 p2m over regions that would otherwise
already have mappings established.  It's my view now that we likely
went too far with 75cc460a1b, and Xen disabling memory decoding of
devices (as buggy as they might be) is harmful, and reduces the set of
hardware on which Xen works.

This commits reverts most of 75cc460a1b, and instead adds checks to
vPCI in order to prevent misplaced BARs from being added to the
hardware domain p2m.  Signaling on whether BARs are mapped is tracked
in the vpci structure, so that misplaced BARs are not mapped, and thus
Xen won't attempt to unmap them when memory decoding is disabled.

This restores the behavior of Xen for PV dom0 to the state it was
previous to 75cc460a1b, while also introducing a more contained fix
for the vPCI BAR mapping issues.

Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
AT Citrix we have a system with a device with the following BARs:

BAR [0xfe010, 0xfe010] -> in a EfiMemoryMappedIO region
BAR [0, 0x1fff] -> not positioned, outside host bridge window

And memory decoding enabled by the firmware.  With the current code
(or any of the previous fix proposals), Xen would still disable memory
decoding for the device, and the system will freeze when attempting to
set EFI vars.

I'm afraid the best solution to avoid regressions caused by 75cc460a1b
is the proposal here.
---
 xen/drivers/passthrough/pci.c | 69 -----------------------------------
 xen/drivers/vpci/header.c     | 22 ++++++++++-
 2 files changed, 20 insertions(+), 71 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 149f68bb6e..b42acb8d7c 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -233,9 +233,6 @@ static void check_pdev(const struct pci_dev *pdev)
      PCI_STATUS_REC_TARGET_ABORT | PCI_STATUS_REC_MASTER_ABORT | \
      PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY)
     u16 val;
-    unsigned int nbars = 0, rom_pos = 0, i;
-    static const char warn[] = XENLOG_WARNING
-        "%pp disabled: %sBAR [%#lx, %#lx] overlaps with memory map\n";
 
     if ( command_mask )
     {
@@ -254,8 +251,6 @@ static void check_pdev(const struct pci_dev *pdev)
     switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
     {
     case PCI_HEADER_TYPE_BRIDGE:
-        nbars = PCI_HEADER_BRIDGE_NR_BARS;
-        rom_pos = PCI_ROM_ADDRESS1;
         if ( !bridge_ctl_mask )
             break;
         val = pci_conf_read16(pdev->sbdf, PCI_BRIDGE_CONTROL);
@@ -272,75 +267,11 @@ static void check_pdev(const struct pci_dev *pdev)
         }
         break;
 
-    case PCI_HEADER_TYPE_NORMAL:
-        nbars = PCI_HEADER_NORMAL_NR_BARS;
-        rom_pos = PCI_ROM_ADDRESS;
-        break;
-
     case PCI_HEADER_TYPE_CARDBUS:
         /* TODO */
         break;
     }
 #undef PCI_STATUS_CHECK
-
-    /* Check if BARs overlap with other memory regions. */
-    val = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
-    if ( !(val & PCI_COMMAND_MEMORY) || pdev->ignore_bars )
-        return;
-
-    pci_conf_write16(pdev->sbdf, PCI_COMMAND, val & ~PCI_COMMAND_MEMORY);
-    for ( i = 0; i < nbars; )
-    {
-        uint64_t addr, size;
-        unsigned int reg = PCI_BASE_ADDRESS_0 + i * 4;
-        int rc = 1;
-
-        if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE) !=
-             PCI_BASE_ADDRESS_SPACE_MEMORY )
-            goto next;
-
-        rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
-                              (i == nbars - 1) ? PCI_BAR_LAST : 0);
-        if ( rc < 0 )
-            /* Unable to size, better leave memory decoding disabled. */
-            return;
-        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
-                                    maddr_to_mfn(addr + size - 1)) )
-        {
-            /*
-             * Return without enabling memory decoding if BAR position is not
-             * in IO suitable memory. Let the hardware domain re-position the
-             * BAR.
-             */
-            printk(warn,
-                   &pdev->sbdf, "", PFN_DOWN(addr), PFN_DOWN(addr + size - 1));
-            return;
-        }
-
- next:
-        ASSERT(rc > 0);
-        i += rc;
-    }
-
-    if ( rom_pos &&
-         (pci_conf_read32(pdev->sbdf, rom_pos) & PCI_ROM_ADDRESS_ENABLE) )
-    {
-        uint64_t addr, size;
-        int rc = pci_size_mem_bar(pdev->sbdf, rom_pos, &addr, &size,
-                                  PCI_BAR_ROM);
-
-        if ( rc < 0 )
-            return;
-        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
-                                    maddr_to_mfn(addr + size - 1)) )
-        {
-            printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr),
-                   PFN_DOWN(addr + size - 1));
-            return;
-        }
-    }
-
-    pci_conf_write16(pdev->sbdf, PCI_COMMAND, val);
 }
 
 static void apply_quirks(struct pci_dev *pdev)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index eb9219a49a..4d7f8f4a30 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -121,7 +121,9 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
         }
 
         if ( !rom_only &&
-             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
+             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
+             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
+                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
             bar->enabled = map;
     }
 
@@ -234,9 +236,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
         if ( !MAPPABLE_BAR(bar) ||
              (rom_only ? bar->type != VPCI_BAR_ROM
-                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
+                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
+             /* Skip BARs already in the requested state. */
+             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
             continue;
 
+        /*
+         * Only do BAR position checks for the hardware domain, for guests it's
+         * assumed that the hardware domain has changed the position of any
+         * problematic BARs.
+         */
+        if ( is_hardware_domain(pdev->domain) &&
+             !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pp: not mapping BAR [%lx, %lx] invalid position\n",
+                   &pdev->sbdf, start, end);
+            continue;
+        }
+
         rc = rangeset_add_range(mem, start, end);
         if ( rc )
         {
-- 
2.37.3

[PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Roger Pau Monne]

Writes to the BARs are ignored if memory decoding is enabled for the
device, and the same happen with ROM BARs if the write is an attempt
to change the position of the BAR without disabling it first.

The reason of ignoring such writes is a limitation in Xen, as it would
need to unmap the BAR, change the address, and remap the BAR at the
new position, which the current logic doesn't support.

Some devices however seem to (wrongly) have the memory decoding bit
hardcoded to enabled, and attempts to disable it don't get reflected
on the command register.

This causes issues for well behaved guests that disable memory
decoding and then try to size the BARs, as vPCI will think memory
decoding is still enabled and ignore the write.

Since vPCI doesn't explicitly care about whether the memory decoding
bit is disabled as long as the BAR is not mapped in the guest p2m use
the information in the vpci_bar to check whether the BAR is mapped,
and refuse writes only based on that information.  This workarounds
the issue, and allows guests to size and reposition the BARs properly.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Cache setting of memory decoding in command register.
 - Reword some log messages.
---
 xen/drivers/vpci/header.c | 18 ++++++++++--------
 xen/include/xen/vpci.h    |  6 ++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4d7f8f4a30..ecd95059b2 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -128,7 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
     }
 
     if ( !rom_only )
+    {
         pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+        header->bars_mapped = map;
+    }
     else
         ASSERT_UNREACHABLE();
 }
@@ -355,13 +358,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 static void cf_check cmd_write(
     const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
 {
-    uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
+    struct vpci_header *header = data;
 
     /*
      * Let Dom0 play with all the bits directly except for the memory
      * decoding one.
      */
-    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
+    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
         /*
          * 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
@@ -388,12 +391,12 @@ static void cf_check bar_write(
     else
         val &= PCI_BASE_ADDRESS_MEM_MASK;
 
-    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
+    if ( bar->enabled )
     {
         /* If the value written is the current one avoid printing a warning. */
         if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
             gprintk(XENLOG_WARNING,
-                    "%pp: ignored BAR %zu write with memory decoding enabled\n",
+                    "%pp: ignored BAR %zu write while mapped\n",
                     &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
         return;
     }
@@ -422,13 +425,12 @@ static void cf_check rom_write(
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct vpci_bar *rom = data;
-    uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
     bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE;
 
-    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled )
+    if ( header->bars_mapped && header->rom_enabled && new_enabled )
     {
         gprintk(XENLOG_WARNING,
-                "%pp: ignored ROM BAR write with memory decoding enabled\n",
+                "%pp: ignored ROM BAR write while mapped\n",
                 &pdev->sbdf);
         return;
     }
@@ -440,7 +442,7 @@ static void cf_check rom_write(
          */
         rom->addr = val & PCI_ROM_ADDRESS_MASK;
 
-    if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled )
+    if ( !header->bars_mapped || header->rom_enabled == new_enabled )
     {
         /* Just update the ROM BAR field. */
         header->rom_enabled = new_enabled;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 67c9a0c631..d8acfeba8a 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -88,6 +88,12 @@ struct vpci {
          * is mapped into guest p2m) if there's a ROM BAR on the device.
          */
         bool rom_enabled      : 1;
+        /*
+         * Cache whether memory decoding is enabled from our PoV.
+         * Some devices have a sticky memory decoding so that can't be relied
+         * upon to know whether BARs are mapped into the guest p2m.
+         */
+        bool bars_mapped      : 1;
         /* FIXME: currently there's no support for SR-IOV. */
     } header;
 
-- 
2.37.3

Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Andrew Cooper]

On 25/10/2022 15:44, Roger Pau Monne wrote:
> Commit 75cc460a1b added checks to ensure the position of the BARs from
> PCI devices don't overlap with regions defined on the memory map.
> When there's a collision memory decoding is left disabled for the
> device, assuming that dom0 will reposition the BAR if necessary and
> enable memory decoding.
>
> While this would be the case for devices being used by dom0, devices
> being used by the firmware itself that have no driver would usually be
> left with memory decoding disabled by dom0 if that's the state dom0
> found them in, and thus firmware trying to make use of them will not
> function correctly.
>
> The initial intent of 75cc460a1b was to prevent vPCI from creating
> MMIO mappings on the dom0 p2m over regions that would otherwise
> already have mappings established.  It's my view now that we likely
> went too far with 75cc460a1b, and Xen disabling memory decoding of
> devices (as buggy as they might be) is harmful, and reduces the set of
> hardware on which Xen works.
>
> This commits reverts most of 75cc460a1b, and instead adds checks to
> vPCI in order to prevent misplaced BARs from being added to the
> hardware domain p2m.  Signaling on whether BARs are mapped is tracked
> in the vpci structure, so that misplaced BARs are not mapped, and thus
> Xen won't attempt to unmap them when memory decoding is disabled.
>
> This restores the behavior of Xen for PV dom0 to the state it was
> previous to 75cc460a1b, while also introducing a more contained fix
> for the vPCI BAR mapping issues.
>
> Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> AT Citrix we have a system with a device with the following BARs:
>
> BAR [0xfe010, 0xfe010] -> in a EfiMemoryMappedIO region
> BAR [0, 0x1fff] -> not positioned, outside host bridge window

This needs a bit more explanation (even if only in the email thread). 
The first item here is permitted under the UEFI spec, and exists on
production systems.  We've got several examples.

The second has only been seen on an SDP, and is hopefully a firmware bug
that doesn't get out to production, but we should boot nevertheless.

~Andrew

Re: [PATCH for-4.17 v2 2/5] vpci/msix: remove from table list on detach

[Jan Beulich]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> Teardown of MSIX vPCI related data doesn't currently remove the MSIX
> device data from the list of MSIX tables handled by the domain,
> leading to a use-after-free of the data in the msix structure.
> 
> Remove the structure from the list before freeing in order to solve
> it.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Fixes: d6281be9d0 ('vpci/msix: add MSI-X handlers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Jan Beulich]

On 25.10.2022 16:57, Andrew Cooper wrote:
> On 25/10/2022 15:44, Roger Pau Monne wrote:
>> Commit 75cc460a1b added checks to ensure the position of the BARs from
>> PCI devices don't overlap with regions defined on the memory map.
>> When there's a collision memory decoding is left disabled for the
>> device, assuming that dom0 will reposition the BAR if necessary and
>> enable memory decoding.
>>
>> While this would be the case for devices being used by dom0, devices
>> being used by the firmware itself that have no driver would usually be
>> left with memory decoding disabled by dom0 if that's the state dom0
>> found them in, and thus firmware trying to make use of them will not
>> function correctly.
>>
>> The initial intent of 75cc460a1b was to prevent vPCI from creating
>> MMIO mappings on the dom0 p2m over regions that would otherwise
>> already have mappings established.  It's my view now that we likely
>> went too far with 75cc460a1b, and Xen disabling memory decoding of
>> devices (as buggy as they might be) is harmful, and reduces the set of
>> hardware on which Xen works.
>>
>> This commits reverts most of 75cc460a1b, and instead adds checks to
>> vPCI in order to prevent misplaced BARs from being added to the
>> hardware domain p2m.  Signaling on whether BARs are mapped is tracked
>> in the vpci structure, so that misplaced BARs are not mapped, and thus
>> Xen won't attempt to unmap them when memory decoding is disabled.
>>
>> This restores the behavior of Xen for PV dom0 to the state it was
>> previous to 75cc460a1b, while also introducing a more contained fix
>> for the vPCI BAR mapping issues.
>>
>> Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> AT Citrix we have a system with a device with the following BARs:
>>
>> BAR [0xfe010, 0xfe010] -> in a EfiMemoryMappedIO region
>> BAR [0, 0x1fff] -> not positioned, outside host bridge window
> 
> This needs a bit more explanation (even if only in the email thread). 
> The first item here is permitted under the UEFI spec, and exists on
> production systems.  We've got several examples.

Afaict it is at best unclear whether this is really permitted. Generally
BARs are not supposed to be covered by memory map entries, be it E820 or
EFI.

Jan

> The second has only been seen on an SDP, and is hopefully a firmware bug
> that doesn't get out to production, but we should boot nevertheless.
> 
> ~Andrew

Re: [PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding handling

[Jan Beulich]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> Hello,
> 
> This patch series attempts to fix the regressions caused by 75cc460a1b
> ('xen/pci: detect when BARs are not suitably positioned') and the last
> patch relaxes the check done when attempting to write to BARs with
> memory decoding enabled.
> 
> I consider all of them bug fixes, albeit the last patch is not fixing a
> regression (since vPCI code has always behaved this way).
> 
> Thanks, Roger.
> 
> Roger Pau Monne (5):
>   vpci: don't assume that vpci per-device data exists unconditionally
>   vpci/msix: remove from table list on detach
>   vpci: introduce a local vpci_bar variable to modify_decoding()
>   pci: do not disable memory decoding for devices
>   vpci: refuse BAR writes only if the BAR is mapped

Looks like you've lost Henry's release-ack that was given for all of v1
(i.e. only patch 2 isn't covered by it).

Jan

Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Roger Pau Monné]

On Tue, Oct 25, 2022 at 02:57:57PM +0000, Andrew Cooper wrote:
> On 25/10/2022 15:44, Roger Pau Monne wrote:
> > Commit 75cc460a1b added checks to ensure the position of the BARs from
> > PCI devices don't overlap with regions defined on the memory map.
> > When there's a collision memory decoding is left disabled for the
> > device, assuming that dom0 will reposition the BAR if necessary and
> > enable memory decoding.
> >
> > While this would be the case for devices being used by dom0, devices
> > being used by the firmware itself that have no driver would usually be
> > left with memory decoding disabled by dom0 if that's the state dom0
> > found them in, and thus firmware trying to make use of them will not
> > function correctly.
> >
> > The initial intent of 75cc460a1b was to prevent vPCI from creating
> > MMIO mappings on the dom0 p2m over regions that would otherwise
> > already have mappings established.  It's my view now that we likely
> > went too far with 75cc460a1b, and Xen disabling memory decoding of
> > devices (as buggy as they might be) is harmful, and reduces the set of
> > hardware on which Xen works.
> >
> > This commits reverts most of 75cc460a1b, and instead adds checks to
> > vPCI in order to prevent misplaced BARs from being added to the
> > hardware domain p2m.  Signaling on whether BARs are mapped is tracked
> > in the vpci structure, so that misplaced BARs are not mapped, and thus
> > Xen won't attempt to unmap them when memory decoding is disabled.
> >
> > This restores the behavior of Xen for PV dom0 to the state it was
> > previous to 75cc460a1b, while also introducing a more contained fix
> > for the vPCI BAR mapping issues.
> >
> > Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > AT Citrix we have a system with a device with the following BARs:
> >
> > BAR [0xfe010, 0xfe010] -> in a EfiMemoryMappedIO region
> > BAR [0, 0x1fff] -> not positioned, outside host bridge window
> 
> This needs a bit more explanation (even if only in the email thread). 
> The first item here is permitted under the UEFI spec, and exists on
> production systems.  We've got several examples.
> 
> The second has only been seen on an SDP, and is hopefully a firmware bug
> that doesn't get out to production, but we should boot nevertheless.

I already saw the second on production systems, as is what triggered
the change in 75cc460a1b.  I might not have seen both in conjunction
on the same device on a production system.

Thanks, Roger.

Re: [PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding handling

[Roger Pau Monné]

On Tue, Oct 25, 2022 at 05:02:41PM +0200, Jan Beulich wrote:
> On 25.10.2022 16:44, Roger Pau Monne wrote:
> > Hello,
> > 
> > This patch series attempts to fix the regressions caused by 75cc460a1b
> > ('xen/pci: detect when BARs are not suitably positioned') and the last
> > patch relaxes the check done when attempting to write to BARs with
> > memory decoding enabled.
> > 
> > I consider all of them bug fixes, albeit the last patch is not fixing a
> > regression (since vPCI code has always behaved this way).
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (5):
> >   vpci: don't assume that vpci per-device data exists unconditionally
> >   vpci/msix: remove from table list on detach
> >   vpci: introduce a local vpci_bar variable to modify_decoding()
> >   pci: do not disable memory decoding for devices
> >   vpci: refuse BAR writes only if the BAR is mapped
> 
> Looks like you've lost Henry's release-ack that was given for all of v1
> (i.e. only patch 2 isn't covered by it).

I was worry about adding it again for the whole series (except patch
2), as I think release-acks are conditional to the time they are
given.  IOW: a release-ack given for a previous series sent maybe
weeks ago shouldn't be carried over because conditions to get changes
accepted might be tighter as we progress with the release.

I think Henry would be fine to reassess the suitability of the series
once it gets properly Acked.

Thanks, Roger.

RE: [PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding handling

[Henry Wang]

Hi Roger and Jan,

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH for-4.17 v2 0/5] (v)pci: fixes related to memory decoding
> handling
> 
> On Tue, Oct 25, 2022 at 05:02:41PM +0200, Jan Beulich wrote:
> > On 25.10.2022 16:44, Roger Pau Monne wrote:
> > > Hello,
> > >
> > > This patch series attempts to fix the regressions caused by 75cc460a1b
> > > ('xen/pci: detect when BARs are not suitably positioned') and the last
> > > patch relaxes the check done when attempting to write to BARs with
> > > memory decoding enabled.
> > >
> > > I consider all of them bug fixes, albeit the last patch is not fixing a
> > > regression (since vPCI code has always behaved this way).
> > >
> > > Thanks, Roger.
> > >
> > > Roger Pau Monne (5):
> > >   vpci: don't assume that vpci per-device data exists unconditionally
> > >   vpci/msix: remove from table list on detach
> > >   vpci: introduce a local vpci_bar variable to modify_decoding()
> > >   pci: do not disable memory decoding for devices
> > >   vpci: refuse BAR writes only if the BAR is mapped
> >
> > Looks like you've lost Henry's release-ack that was given for all of v1
> > (i.e. only patch 2 isn't covered by it).
> 
> I was worry about adding it again for the whole series (except patch
> 2), as I think release-acks are conditional to the time they are
> given.  IOW: a release-ack given for a previous series sent maybe
> weeks ago shouldn't be carried over because conditions to get changes
> accepted might be tighter as we progress with the release.

Thanks for being considerate! The release ack still holds.

> 
> I think Henry would be fine to reassess the suitability of the series
> once it gets properly Acked.

No problem, for the whole series (given that this is a bug fix series)

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

But a reminder: patch #4 and #5 need review/ack from maintainers.

Kind regards,
Henry

> 
> Thanks, Roger.

Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Jan Beulich]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -121,7 +121,9 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>          }
>  
>          if ( !rom_only &&
> -             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
> +             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
> +             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
> +                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
>              bar->enabled = map;
>      }

What about the ROM handling immediately above from here?

> @@ -234,9 +236,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  
>          if ( !MAPPABLE_BAR(bar) ||
>               (rom_only ? bar->type != VPCI_BAR_ROM
> -                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
> +                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
> +             /* Skip BARs already in the requested state. */
> +             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
>              continue;
>  
> +        /*
> +         * Only do BAR position checks for the hardware domain, for guests it's
> +         * assumed that the hardware domain has changed the position of any
> +         * problematic BARs.
> +         */
> +        if ( is_hardware_domain(pdev->domain) &&
> +             !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pp: not mapping BAR [%lx, %lx] invalid position\n",
> +                   &pdev->sbdf, start, end);
> +            continue;
> +        }

I'm not convinced of it being appropriate to skip the check for DomU.
I'd rather consider this a "fixme", as (perhaps somewhere else) we
should return an error if a misconfigured device was passed. We cannot
blindly leave the security of the system to tool stack + Dom0 kernel
imo.

And then, if this is Dom0-only, I think it wants to be XENLOG_WARNING,
i.e. without the G infix.

Jan

Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> Writes to the BARs are ignored if memory decoding is enabled for the
> device, and the same happen with ROM BARs if the write is an attempt
> to change the position of the BAR without disabling it first.
> 
> The reason of ignoring such writes is a limitation in Xen, as it would
> need to unmap the BAR, change the address, and remap the BAR at the
> new position, which the current logic doesn't support.
> 
> Some devices however seem to (wrongly) have the memory decoding bit
> hardcoded to enabled, and attempts to disable it don't get reflected
> on the command register.
> 
> This causes issues for well behaved guests that disable memory
> decoding and then try to size the BARs, as vPCI will think memory
> decoding is still enabled and ignore the write.

Just to avoid misunderstandings: "guests" here includes Dom0? In such
cases we typically prefer to say "domains". This then also affects
the next (final) paragraph.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -128,7 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>      }
>  
>      if ( !rom_only )
> +    {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> +        header->bars_mapped = map;
> +    }
>      else
>          ASSERT_UNREACHABLE();
>  }
> @@ -355,13 +358,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>  static void cf_check cmd_write(
>      const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
>  {
> -    uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> +    struct vpci_header *header = data;
>  
>      /*
>       * Let Dom0 play with all the bits directly except for the memory
>       * decoding one.
>       */
> -    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> +    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
>          /*
>           * 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
> @@ -388,12 +391,12 @@ static void cf_check bar_write(
>      else
>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>  
> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> +    if ( bar->enabled )

In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
not clear to me why you don't here, could you explain this to me? (I'm
therefore undecided whether this is merely a cosmetic [consistency] issue.)

>      {
>          /* If the value written is the current one avoid printing a warning. */
>          if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>              gprintk(XENLOG_WARNING,
> -                    "%pp: ignored BAR %zu write with memory decoding enabled\n",
> +                    "%pp: ignored BAR %zu write while mapped\n",
>                      &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>          return;
>      }
> @@ -422,13 +425,12 @@ static void cf_check rom_write(
>  {
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *rom = data;
> -    uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>      bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE;
>  
> -    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled )
> +    if ( header->bars_mapped && header->rom_enabled && new_enabled )
>      {
>          gprintk(XENLOG_WARNING,
> -                "%pp: ignored ROM BAR write with memory decoding enabled\n",
> +                "%pp: ignored ROM BAR write while mapped\n",
>                  &pdev->sbdf);
>          return;
>      }
> @@ -440,7 +442,7 @@ static void cf_check rom_write(
>           */
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  
> -    if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled )
> +    if ( !header->bars_mapped || header->rom_enabled == new_enabled )
>      {
>          /* Just update the ROM BAR field. */
>          header->rom_enabled = new_enabled;

Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Roger Pau Monné]

On Wed, Oct 26, 2022 at 02:35:44PM +0200, Jan Beulich wrote:
> On 25.10.2022 16:44, Roger Pau Monne wrote:
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -121,7 +121,9 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
> >          }
> >  
> >          if ( !rom_only &&
> > -             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
> > +             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
> > +             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
> > +                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
> >              bar->enabled = map;
> >      }
> 
> What about the ROM handling immediately above from here?

Needs fixing indeed.  I had plans to short circuit the ROM only
mapping path earlier if the BAR wasn't correctly positioned, but
decided it was too complicated and then forgot to adjust the
conditional.

> > @@ -234,9 +236,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> >  
> >          if ( !MAPPABLE_BAR(bar) ||
> >               (rom_only ? bar->type != VPCI_BAR_ROM
> > -                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
> > +                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
> > +             /* Skip BARs already in the requested state. */
> > +             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
> >              continue;
> >  
> > +        /*
> > +         * Only do BAR position checks for the hardware domain, for guests it's
> > +         * assumed that the hardware domain has changed the position of any
> > +         * problematic BARs.
> > +         */
> > +        if ( is_hardware_domain(pdev->domain) &&
> > +             !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pp: not mapping BAR [%lx, %lx] invalid position\n",
> > +                   &pdev->sbdf, start, end);
> > +            continue;
> > +        }
> 
> I'm not convinced of it being appropriate to skip the check for DomU.
> I'd rather consider this a "fixme", as (perhaps somewhere else) we
> should return an error if a misconfigured device was passed. We cannot
> blindly leave the security of the system to tool stack + Dom0 kernel
> imo.
> 
> And then, if this is Dom0-only, I think it wants to be XENLOG_WARNING,
> i.e. without the G infix.

OK, I don't mind removing the is_hardware_domain() condition, it's
still not clear how we want to handle all this when domU support is
added.

Thanks, Roger.

Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Roger Pau Monné]

On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
> On 25.10.2022 16:44, Roger Pau Monne wrote:
> > Writes to the BARs are ignored if memory decoding is enabled for the
> > device, and the same happen with ROM BARs if the write is an attempt
> > to change the position of the BAR without disabling it first.
> > 
> > The reason of ignoring such writes is a limitation in Xen, as it would
> > need to unmap the BAR, change the address, and remap the BAR at the
> > new position, which the current logic doesn't support.
> > 
> > Some devices however seem to (wrongly) have the memory decoding bit
> > hardcoded to enabled, and attempts to disable it don't get reflected
> > on the command register.
> > 
> > This causes issues for well behaved guests that disable memory
> > decoding and then try to size the BARs, as vPCI will think memory
> > decoding is still enabled and ignore the write.
> 
> Just to avoid misunderstandings: "guests" here includes Dom0? In such
> cases we typically prefer to say "domains". This then also affects
> the next (final) paragraph.

Right, will adjust.

> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -128,7 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
> >      }
> >  
> >      if ( !rom_only )
> > +    {
> >          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> > +        header->bars_mapped = map;
> > +    }
> >      else
> >          ASSERT_UNREACHABLE();
> >  }
> > @@ -355,13 +358,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> >  static void cf_check cmd_write(
> >      const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
> >  {
> > -    uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> > +    struct vpci_header *header = data;
> >  
> >      /*
> >       * Let Dom0 play with all the bits directly except for the memory
> >       * decoding one.
> >       */
> > -    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> > +    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
> >          /*
> >           * 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
> > @@ -388,12 +391,12 @@ static void cf_check bar_write(
> >      else
> >          val &= PCI_BASE_ADDRESS_MEM_MASK;
> >  
> > -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> > +    if ( bar->enabled )
> 
> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
> not clear to me why you don't here, could you explain this to me? (I'm
> therefore undecided whether this is merely a cosmetic [consistency] issue.)

No, it's intended to use bar->enabled here rather than
header->bars_mapped.

It's possible to have header->bars_mapped == true, but bar->enabled ==
false if memory decoding is enabled, but this BAR specifically has
failed to be mapped in the guest p2m, which means dom0 is safe to move
it for what Xen cares (ie: it won't mess with p2m mappings because
there are none for the BAR).

We could be more strict and use header->bars_mapped, but I don't think
there's a need for it.

What about adding a comment with:

/*
 * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
 * writes as long as the BAR is not mapped into the p2m.
 */

Otherwise I can switch to using header->bars_mapped if you think
that's clearer.

Thanks, Roger.

Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich]

On 26.10.2022 15:58, Roger Pau Monné wrote:
> On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
>> On 25.10.2022 16:44, Roger Pau Monne wrote:
>>> @@ -388,12 +391,12 @@ static void cf_check bar_write(
>>>      else
>>>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>  
>>> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>>> +    if ( bar->enabled )
>>
>> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
>> not clear to me why you don't here, could you explain this to me? (I'm
>> therefore undecided whether this is merely a cosmetic [consistency] issue.)
> 
> No, it's intended to use bar->enabled here rather than
> header->bars_mapped.
> 
> It's possible to have header->bars_mapped == true, but bar->enabled ==
> false if memory decoding is enabled, but this BAR specifically has
> failed to be mapped in the guest p2m, which means dom0 is safe to move
> it for what Xen cares (ie: it won't mess with p2m mappings because
> there are none for the BAR).
> 
> We could be more strict and use header->bars_mapped, but I don't think
> there's a need for it.
> 
> What about adding a comment with:
> 
> /*
>  * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
>  * writes as long as the BAR is not mapped into the p2m.
>  */
> 
> Otherwise I can switch to using header->bars_mapped if you think
> that's clearer.

It's not so much a matter of being clearer, but a matter of consistency:
Why does the same consideration not apply in rom_write()? In fact both
uses there are (already before the change) combined with further
conditions (checking header->rom_enabled and new_enabled). If the
inconsistency is on purpose (and perhaps necessary), I'd like to first
understand why that is before deciding what to do about it. A comment
like you suggest it _may_ be the route to go.

Jan

Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Roger Pau Monné]

On Wed, Oct 26, 2022 at 04:06:40PM +0200, Jan Beulich wrote:
> On 26.10.2022 15:58, Roger Pau Monné wrote:
> > On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
> >> On 25.10.2022 16:44, Roger Pau Monne wrote:
> >>> @@ -388,12 +391,12 @@ static void cf_check bar_write(
> >>>      else
> >>>          val &= PCI_BASE_ADDRESS_MEM_MASK;
> >>>  
> >>> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> >>> +    if ( bar->enabled )
> >>
> >> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
> >> not clear to me why you don't here, could you explain this to me? (I'm
> >> therefore undecided whether this is merely a cosmetic [consistency] issue.)
> > 
> > No, it's intended to use bar->enabled here rather than
> > header->bars_mapped.
> > 
> > It's possible to have header->bars_mapped == true, but bar->enabled ==
> > false if memory decoding is enabled, but this BAR specifically has
> > failed to be mapped in the guest p2m, which means dom0 is safe to move
> > it for what Xen cares (ie: it won't mess with p2m mappings because
> > there are none for the BAR).
> > 
> > We could be more strict and use header->bars_mapped, but I don't think
> > there's a need for it.
> > 
> > What about adding a comment with:
> > 
> > /*
> >  * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
> >  * writes as long as the BAR is not mapped into the p2m.
> >  */
> > 
> > Otherwise I can switch to using header->bars_mapped if you think
> > that's clearer.
> 
> It's not so much a matter of being clearer, but a matter of consistency:
> Why does the same consideration not apply in rom_write()? In fact both
> uses there are (already before the change) combined with further
> conditions (checking header->rom_enabled and new_enabled). If the
> inconsistency is on purpose (and perhaps necessary), I'd like to first
> understand why that is before deciding what to do about it. A comment
> like you suggest it _may_ be the route to go.

ROM register is more complex to handle, because the same register
that's used to store the address also contains the bit that can
trigger whether it's mapped into the guest p2m or not
(PCI_ROM_ADDRESS_ENABLE).  So ROM BAR writes with the ROM BAR mapped
and the PCI_ROM_ADDRESS_ENABLE bit also set in the to be written value
will be rejected, because we only allow to first disable the ROM and
then change the address (which is likely to not play well with OSes,
but so far I haven't been able to test ROM BAR register usage on PVH).

I do think for consistency it would be better to use rom->enabled in
the first conditional of rom_write() check, so it would be:

    if ( rom->enabled && new_enabled )
    {
        gprintk(XENLOG_WARNING,
                "%pp: ignored ROM BAR write while mapped\n",
                &pdev->sbdf);
        return;
    }

So that we also allow changing the address of ROM BARs even with
memory decoding and PCI_ROM_ADDRESS_ENABLE as long as the ROM BAR is
not mapped in the p2m.

Would you be fine with the comment in the previous email added and
rom_write() adjusted as suggested above?

Thanks, Roger.

Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich]

On 26.10.2022 18:01, Roger Pau Monné wrote:
> On Wed, Oct 26, 2022 at 04:06:40PM +0200, Jan Beulich wrote:
>> On 26.10.2022 15:58, Roger Pau Monné wrote:
>>> On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
>>>> On 25.10.2022 16:44, Roger Pau Monne wrote:
>>>>> @@ -388,12 +391,12 @@ static void cf_check bar_write(
>>>>>      else
>>>>>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>>>  
>>>>> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>>>>> +    if ( bar->enabled )
>>>>
>>>> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
>>>> not clear to me why you don't here, could you explain this to me? (I'm
>>>> therefore undecided whether this is merely a cosmetic [consistency] issue.)
>>>
>>> No, it's intended to use bar->enabled here rather than
>>> header->bars_mapped.
>>>
>>> It's possible to have header->bars_mapped == true, but bar->enabled ==
>>> false if memory decoding is enabled, but this BAR specifically has
>>> failed to be mapped in the guest p2m, which means dom0 is safe to move
>>> it for what Xen cares (ie: it won't mess with p2m mappings because
>>> there are none for the BAR).
>>>
>>> We could be more strict and use header->bars_mapped, but I don't think
>>> there's a need for it.
>>>
>>> What about adding a comment with:
>>>
>>> /*
>>>  * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
>>>  * writes as long as the BAR is not mapped into the p2m.
>>>  */
>>>
>>> Otherwise I can switch to using header->bars_mapped if you think
>>> that's clearer.
>>
>> It's not so much a matter of being clearer, but a matter of consistency:
>> Why does the same consideration not apply in rom_write()? In fact both
>> uses there are (already before the change) combined with further
>> conditions (checking header->rom_enabled and new_enabled). If the
>> inconsistency is on purpose (and perhaps necessary), I'd like to first
>> understand why that is before deciding what to do about it. A comment
>> like you suggest it _may_ be the route to go.
> 
> ROM register is more complex to handle, because the same register
> that's used to store the address also contains the bit that can
> trigger whether it's mapped into the guest p2m or not
> (PCI_ROM_ADDRESS_ENABLE).  So ROM BAR writes with the ROM BAR mapped
> and the PCI_ROM_ADDRESS_ENABLE bit also set in the to be written value
> will be rejected, because we only allow to first disable the ROM and
> then change the address (which is likely to not play well with OSes,
> but so far I haven't been able to test ROM BAR register usage on PVH).
> 
> I do think for consistency it would be better to use rom->enabled in
> the first conditional of rom_write() check, so it would be:
> 
>     if ( rom->enabled && new_enabled )
>     {
>         gprintk(XENLOG_WARNING,
>                 "%pp: ignored ROM BAR write while mapped\n",
>                 &pdev->sbdf);
>         return;
>     }
> 
> So that we also allow changing the address of ROM BARs even with
> memory decoding and PCI_ROM_ADDRESS_ENABLE as long as the ROM BAR is
> not mapped in the p2m.
> 
> Would you be fine with the comment in the previous email added and
> rom_write() adjusted as suggested above?

Yes, that would look better to me. The comment then probably also wants
duplicating (or pointing at from) here.

Jan

Re: [PATCH for-4.17 v2 1/5] vpci: don't assume that vpci per-device data exists unconditionally

[Jan Beulich]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -37,7 +37,7 @@ extern vpci_register_init_t *const __end_vpci_array[];
>  
>  void vpci_remove_device(struct pci_dev *pdev)
>  {
> -    if ( !has_vpci(pdev->domain) )
> +    if ( !has_vpci(pdev->domain) || !pdev->vpci )
>          return;

Btw (noticing while backporting) - is the left side of the || still
needed then?

Jan