diff mbox series

[v3] vPCI: account for hidden devices

Message ID e1c6e297-0046-73f6-981d-af776b271f24@suse.com (mailing list archive)
State New, archived
Headers show
Series [v3] vPCI: account for hidden devices | expand

Commit Message

Jan Beulich May 30, 2023, 12:38 p.m. UTC
Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
console) are associated with DomXEN, not Dom0. This means that while
looking for overlapping BARs such devices cannot be found on Dom0's list
of devices; DomXEN's list also needs to be scanned.

Suppress vPCI init altogether for r/o devices (which constitute a subset
of hidden ones).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Also consider pdev being DomXEN's in modify_bars(). Also consult
    DomXEN in vpci_{read,write}(). Move vpci_write()'s check of the r/o
    map out of mainline code. Re-base over the standalone addition of
    the loop continuation in modify_bars(), and finally make the code
    change there well-formed.
v2: Extend existing comment. Relax assertion. Don't initialize vPCI for
    r/o devices.

Comments

Roger Pau Monné May 30, 2023, 1:36 p.m. UTC | #1
On Tue, May 30, 2023 at 02:38:56PM +0200, Jan Beulich wrote:
> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> console) are associated with DomXEN, not Dom0. This means that while
> looking for overlapping BARs such devices cannot be found on Dom0's list
> of devices; DomXEN's list also needs to be scanned.
> 
> Suppress vPCI init altogether for r/o devices (which constitute a subset
> of hidden ones).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one nit below.

> ---
> v3: Also consider pdev being DomXEN's in modify_bars(). Also consult
>     DomXEN in vpci_{read,write}(). Move vpci_write()'s check of the r/o
>     map out of mainline code. Re-base over the standalone addition of
>     the loop continuation in modify_bars(), and finally make the code
>     change there well-formed.
> v2: Extend existing comment. Relax assertion. Don't initialize vPCI for
>     r/o devices.
> 
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
>      struct vpci_header *header = &pdev->vpci->header;
>      struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>      struct pci_dev *tmp, *dev = NULL;
> +    const struct domain *d;
>      const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i;
>      int rc;
> @@ -285,58 +286,69 @@ static int modify_bars(const struct pci_
>  
>      /*
>       * Check for overlaps with other BARs. Note that only BARs that are
> -     * currently mapped (enabled) are checked for overlaps.
> +     * currently mapped (enabled) are checked for overlaps. Note also that
> +     * for hwdom we also need to include hidden, i.e. DomXEN's, devices.
>       */
> -    for_each_pdev ( pdev->domain, tmp )
> +    for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain; ; )
>      {
> -        if ( !tmp->vpci )
> -            /*
> -             * For the hardware domain it's possible to have devices assigned
> -             * to it that are not handled by vPCI, either because those are
> -             * read-only devices, or because vPCI setup has failed.
> -             */
> -            continue;
> -
> -        if ( tmp == pdev )
> +        for_each_pdev ( d, tmp )
>          {
> -            /*
> -             * Need to store the device so it's not constified and defer_map
> -             * can modify it in case of error.
> -             */
> -            dev = tmp;
> -            if ( !rom_only )
> +            if ( !tmp->vpci )
>                  /*
> -                 * If memory decoding is toggled avoid checking against the
> -                 * same device, or else all regions will be removed from the
> -                 * memory map in the unmap case.
> +                 * For the hardware domain it's possible to have devices
> +                 * assigned to it that are not handled by vPCI, either because
> +                 * those are read-only devices, or because vPCI setup has
> +                 * failed.
>                   */
>                  continue;
> -        }
>  
> -        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
> -        {
> -            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> -            unsigned long start = PFN_DOWN(bar->addr);
> -            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> -
> -            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) ||
> -                 /*
> -                  * If only the ROM enable bit is toggled check against other
> -                  * BARs in the same device for overlaps, but not against the
> -                  * same ROM BAR.
> -                  */
> -                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> -                continue;
> +            if ( tmp == pdev )
> +            {
> +                /*
> +                 * Need to store the device so it's not constified and defer_map
> +                 * can modify it in case of error.
> +                 */
> +                dev = tmp;
> +                if ( !rom_only )
> +                    /*
> +                     * If memory decoding is toggled avoid checking against the
> +                     * same device, or else all regions will be removed from the
> +                     * memory map in the unmap case.
> +                     */
> +                    continue;
> +            }
>  
> -            rc = rangeset_remove_range(mem, start, end);
> -            if ( rc )
> +            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>              {
> -                printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
> -                       start, end, rc);
> -                rangeset_destroy(mem);
> -                return rc;
> +                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> +                unsigned long start = PFN_DOWN(bar->addr);
> +                unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> +
> +                if ( !bar->enabled ||
> +                     !rangeset_overlaps_range(mem, start, end) ||
> +                     /*
> +                      * If only the ROM enable bit is toggled check against
> +                      * other BARs in the same device for overlaps, but not
> +                      * against the same ROM BAR.
> +                      */
> +                     (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> +                    continue;
> +
> +                rc = rangeset_remove_range(mem, start, end);
> +                if ( rc )
> +                {
> +                    printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
> +                           start, end, rc);
> +                    rangeset_destroy(mem);
> +                    return rc;
> +                }
>              }
>          }
> +
> +        if ( !is_hardware_domain(d) )
> +            break;
> +
> +        d = dom_xen;

Nit: don't you want to do this in the advancement to the next
iteration?

Thanks, Roger.
Jan Beulich May 30, 2023, 2:13 p.m. UTC | #2
On 30.05.2023 15:36, Roger Pau Monné wrote:
> On Tue, May 30, 2023 at 02:38:56PM +0200, Jan Beulich wrote:
>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>> console) are associated with DomXEN, not Dom0. This means that while
>> looking for overlapping BARs such devices cannot be found on Dom0's list
>> of devices; DomXEN's list also needs to be scanned.
>>
>> Suppress vPCI init altogether for r/o devices (which constitute a subset
>> of hidden ones).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> @@ -285,58 +286,69 @@ static int modify_bars(const struct pci_
>>  
>>      /*
>>       * Check for overlaps with other BARs. Note that only BARs that are
>> -     * currently mapped (enabled) are checked for overlaps.
>> +     * currently mapped (enabled) are checked for overlaps. Note also that
>> +     * for hwdom we also need to include hidden, i.e. DomXEN's, devices.
>>       */
>> -    for_each_pdev ( pdev->domain, tmp )
>> +    for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain; ; )
>>      {
>> -        if ( !tmp->vpci )
>> -            /*
>> -             * For the hardware domain it's possible to have devices assigned
>> -             * to it that are not handled by vPCI, either because those are
>> -             * read-only devices, or because vPCI setup has failed.
>> -             */
>> -            continue;
>> -
>> -        if ( tmp == pdev )
>> +        for_each_pdev ( d, tmp )
>>          {
>> -            /*
>> -             * Need to store the device so it's not constified and defer_map
>> -             * can modify it in case of error.
>> -             */
>> -            dev = tmp;
>> -            if ( !rom_only )
>> +            if ( !tmp->vpci )
>>                  /*
>> -                 * If memory decoding is toggled avoid checking against the
>> -                 * same device, or else all regions will be removed from the
>> -                 * memory map in the unmap case.
>> +                 * For the hardware domain it's possible to have devices
>> +                 * assigned to it that are not handled by vPCI, either because
>> +                 * those are read-only devices, or because vPCI setup has
>> +                 * failed.
>>                   */
>>                  continue;
>> -        }
>>  
>> -        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>> -        {
>> -            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
>> -            unsigned long start = PFN_DOWN(bar->addr);
>> -            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
>> -
>> -            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) ||
>> -                 /*
>> -                  * If only the ROM enable bit is toggled check against other
>> -                  * BARs in the same device for overlaps, but not against the
>> -                  * same ROM BAR.
>> -                  */
>> -                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
>> -                continue;
>> +            if ( tmp == pdev )
>> +            {
>> +                /*
>> +                 * Need to store the device so it's not constified and defer_map
>> +                 * can modify it in case of error.
>> +                 */
>> +                dev = tmp;
>> +                if ( !rom_only )
>> +                    /*
>> +                     * If memory decoding is toggled avoid checking against the
>> +                     * same device, or else all regions will be removed from the
>> +                     * memory map in the unmap case.
>> +                     */
>> +                    continue;
>> +            }
>>  
>> -            rc = rangeset_remove_range(mem, start, end);
>> -            if ( rc )
>> +            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>>              {
>> -                printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
>> -                       start, end, rc);
>> -                rangeset_destroy(mem);
>> -                return rc;
>> +                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
>> +                unsigned long start = PFN_DOWN(bar->addr);
>> +                unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
>> +
>> +                if ( !bar->enabled ||
>> +                     !rangeset_overlaps_range(mem, start, end) ||
>> +                     /*
>> +                      * If only the ROM enable bit is toggled check against
>> +                      * other BARs in the same device for overlaps, but not
>> +                      * against the same ROM BAR.
>> +                      */
>> +                     (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
>> +                    continue;
>> +
>> +                rc = rangeset_remove_range(mem, start, end);
>> +                if ( rc )
>> +                {
>> +                    printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
>> +                           start, end, rc);
>> +                    rangeset_destroy(mem);
>> +                    return rc;
>> +                }
>>              }
>>          }
>> +
>> +        if ( !is_hardware_domain(d) )
>> +            break;
>> +
>> +        d = dom_xen;
> 
> Nit: don't you want to do this in the advancement to the next
> iteration?

Well, I had it that way first, but I didn't like the need to wrap the
line there. Hence I moved it here, which is functionally identical as
long as no "continue" appears in this (now) outer loop.

Jan
Stefano Stabellini May 30, 2023, 11:05 p.m. UTC | #3
On Tue, 30 May 2023, Jan Beulich wrote:
> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> console) are associated with DomXEN, not Dom0. This means that while
> looking for overlapping BARs such devices cannot be found on Dom0's list
> of devices; DomXEN's list also needs to be scanned.
> 
> Suppress vPCI init altogether for r/o devices (which constitute a subset
> of hidden ones).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> v3: Also consider pdev being DomXEN's in modify_bars(). Also consult
>     DomXEN in vpci_{read,write}(). Move vpci_write()'s check of the r/o
>     map out of mainline code. Re-base over the standalone addition of
>     the loop continuation in modify_bars(), and finally make the code
>     change there well-formed.
> v2: Extend existing comment. Relax assertion. Don't initialize vPCI for
>     r/o devices.
> 
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
>      struct vpci_header *header = &pdev->vpci->header;
>      struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>      struct pci_dev *tmp, *dev = NULL;
> +    const struct domain *d;
>      const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i;
>      int rc;
> @@ -285,58 +286,69 @@ static int modify_bars(const struct pci_
>  
>      /*
>       * Check for overlaps with other BARs. Note that only BARs that are
> -     * currently mapped (enabled) are checked for overlaps.
> +     * currently mapped (enabled) are checked for overlaps. Note also that
> +     * for hwdom we also need to include hidden, i.e. DomXEN's, devices.
>       */
> -    for_each_pdev ( pdev->domain, tmp )
> +    for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain; ; )
>      {
> -        if ( !tmp->vpci )
> -            /*
> -             * For the hardware domain it's possible to have devices assigned
> -             * to it that are not handled by vPCI, either because those are
> -             * read-only devices, or because vPCI setup has failed.
> -             */
> -            continue;
> -
> -        if ( tmp == pdev )
> +        for_each_pdev ( d, tmp )
>          {
> -            /*
> -             * Need to store the device so it's not constified and defer_map
> -             * can modify it in case of error.
> -             */
> -            dev = tmp;
> -            if ( !rom_only )
> +            if ( !tmp->vpci )
>                  /*
> -                 * If memory decoding is toggled avoid checking against the
> -                 * same device, or else all regions will be removed from the
> -                 * memory map in the unmap case.
> +                 * For the hardware domain it's possible to have devices
> +                 * assigned to it that are not handled by vPCI, either because
> +                 * those are read-only devices, or because vPCI setup has
> +                 * failed.
>                   */
>                  continue;
> -        }
>  
> -        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
> -        {
> -            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> -            unsigned long start = PFN_DOWN(bar->addr);
> -            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> -
> -            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) ||
> -                 /*
> -                  * If only the ROM enable bit is toggled check against other
> -                  * BARs in the same device for overlaps, but not against the
> -                  * same ROM BAR.
> -                  */
> -                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> -                continue;
> +            if ( tmp == pdev )
> +            {
> +                /*
> +                 * Need to store the device so it's not constified and defer_map
> +                 * can modify it in case of error.
> +                 */
> +                dev = tmp;
> +                if ( !rom_only )
> +                    /*
> +                     * If memory decoding is toggled avoid checking against the
> +                     * same device, or else all regions will be removed from the
> +                     * memory map in the unmap case.
> +                     */
> +                    continue;
> +            }
>  
> -            rc = rangeset_remove_range(mem, start, end);
> -            if ( rc )
> +            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>              {
> -                printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
> -                       start, end, rc);
> -                rangeset_destroy(mem);
> -                return rc;
> +                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> +                unsigned long start = PFN_DOWN(bar->addr);
> +                unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> +
> +                if ( !bar->enabled ||
> +                     !rangeset_overlaps_range(mem, start, end) ||
> +                     /*
> +                      * If only the ROM enable bit is toggled check against
> +                      * other BARs in the same device for overlaps, but not
> +                      * against the same ROM BAR.
> +                      */
> +                     (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> +                    continue;
> +
> +                rc = rangeset_remove_range(mem, start, end);
> +                if ( rc )
> +                {
> +                    printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
> +                           start, end, rc);
> +                    rangeset_destroy(mem);
> +                    return rc;
> +                }
>              }
>          }
> +
> +        if ( !is_hardware_domain(d) )
> +            break;
> +
> +        d = dom_xen;
>      }
>  
>      ASSERT(dev);
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -70,6 +70,7 @@ void vpci_remove_device(struct pci_dev *
>  int vpci_add_handlers(struct pci_dev *pdev)
>  {
>      unsigned int i;
> +    const unsigned long *ro_map;
>      int rc = 0;
>  
>      if ( !has_vpci(pdev->domain) )
> @@ -78,6 +79,11 @@ int vpci_add_handlers(struct pci_dev *pd
>      /* We should not get here twice for the same device. */
>      ASSERT(!pdev->vpci);
>  
> +    /* No vPCI for r/o devices. */
> +    ro_map = pci_get_ro_map(pdev->sbdf.seg);
> +    if ( ro_map && test_bit(pdev->sbdf.bdf, ro_map) )
> +        return 0;
> +
>      pdev->vpci = xzalloc(struct vpci);
>      if ( !pdev->vpci )
>          return -ENOMEM;
> @@ -332,8 +338,13 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsi
>          return data;
>      }
>  
> -    /* Find the PCI dev matching the address. */
> +    /*
> +     * Find the PCI dev matching the address, which for hwdom also requires
> +     * consulting DomXEN.  Passthrough everything that's not trapped.
> +     */
>      pdev = pci_get_pdev(d, sbdf);
> +    if ( !pdev && is_hardware_domain(d) )
> +        pdev = pci_get_pdev(dom_xen, sbdf);
>      if ( !pdev || !pdev->vpci )
>          return vpci_read_hw(sbdf, reg, size);
>  
> @@ -427,7 +438,6 @@ void vpci_write(pci_sbdf_t sbdf, unsigne
>      const struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
> -    const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
>  
>      if ( !size )
>      {
> @@ -435,18 +445,20 @@ void vpci_write(pci_sbdf_t sbdf, unsigne
>          return;
>      }
>  
> -    if ( ro_map && test_bit(sbdf.bdf, ro_map) )
> -        /* Ignore writes to read-only devices. */
> -        return;
> -
>      /*
> -     * Find the PCI dev matching the address.
> -     * Passthrough everything that's not trapped.
> +     * Find the PCI dev matching the address, which for hwdom also requires
> +     * consulting DomXEN.  Passthrough everything that's not trapped.
>       */
>      pdev = pci_get_pdev(d, sbdf);
> +    if ( !pdev && is_hardware_domain(d) )
> +        pdev = pci_get_pdev(dom_xen, sbdf);
>      if ( !pdev || !pdev->vpci )
>      {
> -        vpci_write_hw(sbdf, reg, size, data);
> +        /* Ignore writes to read-only devices, which have no ->vpci. */
> +        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
> +
> +        if ( !ro_map || !test_bit(sbdf.bdf, ro_map) )
> +            vpci_write_hw(sbdf, reg, size, data);
>          return;
>      }
>  
>
diff mbox series

Patch

--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -218,6 +218,7 @@  static int modify_bars(const struct pci_
     struct vpci_header *header = &pdev->vpci->header;
     struct rangeset *mem = rangeset_new(NULL, NULL, 0);
     struct pci_dev *tmp, *dev = NULL;
+    const struct domain *d;
     const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i;
     int rc;
@@ -285,58 +286,69 @@  static int modify_bars(const struct pci_
 
     /*
      * Check for overlaps with other BARs. Note that only BARs that are
-     * currently mapped (enabled) are checked for overlaps.
+     * currently mapped (enabled) are checked for overlaps. Note also that
+     * for hwdom we also need to include hidden, i.e. DomXEN's, devices.
      */
-    for_each_pdev ( pdev->domain, tmp )
+    for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain; ; )
     {
-        if ( !tmp->vpci )
-            /*
-             * For the hardware domain it's possible to have devices assigned
-             * to it that are not handled by vPCI, either because those are
-             * read-only devices, or because vPCI setup has failed.
-             */
-            continue;
-
-        if ( tmp == pdev )
+        for_each_pdev ( d, tmp )
         {
-            /*
-             * Need to store the device so it's not constified and defer_map
-             * can modify it in case of error.
-             */
-            dev = tmp;
-            if ( !rom_only )
+            if ( !tmp->vpci )
                 /*
-                 * If memory decoding is toggled avoid checking against the
-                 * same device, or else all regions will be removed from the
-                 * memory map in the unmap case.
+                 * For the hardware domain it's possible to have devices
+                 * assigned to it that are not handled by vPCI, either because
+                 * those are read-only devices, or because vPCI setup has
+                 * failed.
                  */
                 continue;
-        }
 
-        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
-        {
-            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
-            unsigned long start = PFN_DOWN(bar->addr);
-            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
-
-            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) ||
-                 /*
-                  * If only the ROM enable bit is toggled check against other
-                  * BARs in the same device for overlaps, but not against the
-                  * same ROM BAR.
-                  */
-                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
-                continue;
+            if ( tmp == pdev )
+            {
+                /*
+                 * Need to store the device so it's not constified and defer_map
+                 * can modify it in case of error.
+                 */
+                dev = tmp;
+                if ( !rom_only )
+                    /*
+                     * If memory decoding is toggled avoid checking against the
+                     * same device, or else all regions will be removed from the
+                     * memory map in the unmap case.
+                     */
+                    continue;
+            }
 
-            rc = rangeset_remove_range(mem, start, end);
-            if ( rc )
+            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
             {
-                printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
-                       start, end, rc);
-                rangeset_destroy(mem);
-                return rc;
+                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
+                unsigned long start = PFN_DOWN(bar->addr);
+                unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
+
+                if ( !bar->enabled ||
+                     !rangeset_overlaps_range(mem, start, end) ||
+                     /*
+                      * If only the ROM enable bit is toggled check against
+                      * other BARs in the same device for overlaps, but not
+                      * against the same ROM BAR.
+                      */
+                     (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
+                    continue;
+
+                rc = rangeset_remove_range(mem, start, end);
+                if ( rc )
+                {
+                    printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
+                           start, end, rc);
+                    rangeset_destroy(mem);
+                    return rc;
+                }
             }
         }
+
+        if ( !is_hardware_domain(d) )
+            break;
+
+        d = dom_xen;
     }
 
     ASSERT(dev);
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -70,6 +70,7 @@  void vpci_remove_device(struct pci_dev *
 int vpci_add_handlers(struct pci_dev *pdev)
 {
     unsigned int i;
+    const unsigned long *ro_map;
     int rc = 0;
 
     if ( !has_vpci(pdev->domain) )
@@ -78,6 +79,11 @@  int vpci_add_handlers(struct pci_dev *pd
     /* We should not get here twice for the same device. */
     ASSERT(!pdev->vpci);
 
+    /* No vPCI for r/o devices. */
+    ro_map = pci_get_ro_map(pdev->sbdf.seg);
+    if ( ro_map && test_bit(pdev->sbdf.bdf, ro_map) )
+        return 0;
+
     pdev->vpci = xzalloc(struct vpci);
     if ( !pdev->vpci )
         return -ENOMEM;
@@ -332,8 +338,13 @@  uint32_t vpci_read(pci_sbdf_t sbdf, unsi
         return data;
     }
 
-    /* Find the PCI dev matching the address. */
+    /*
+     * Find the PCI dev matching the address, which for hwdom also requires
+     * consulting DomXEN.  Passthrough everything that's not trapped.
+     */
     pdev = pci_get_pdev(d, sbdf);
+    if ( !pdev && is_hardware_domain(d) )
+        pdev = pci_get_pdev(dom_xen, sbdf);
     if ( !pdev || !pdev->vpci )
         return vpci_read_hw(sbdf, reg, size);
 
@@ -427,7 +438,6 @@  void vpci_write(pci_sbdf_t sbdf, unsigne
     const struct pci_dev *pdev;
     const struct vpci_register *r;
     unsigned int data_offset = 0;
-    const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
 
     if ( !size )
     {
@@ -435,18 +445,20 @@  void vpci_write(pci_sbdf_t sbdf, unsigne
         return;
     }
 
-    if ( ro_map && test_bit(sbdf.bdf, ro_map) )
-        /* Ignore writes to read-only devices. */
-        return;
-
     /*
-     * Find the PCI dev matching the address.
-     * Passthrough everything that's not trapped.
+     * Find the PCI dev matching the address, which for hwdom also requires
+     * consulting DomXEN.  Passthrough everything that's not trapped.
      */
     pdev = pci_get_pdev(d, sbdf);
+    if ( !pdev && is_hardware_domain(d) )
+        pdev = pci_get_pdev(dom_xen, sbdf);
     if ( !pdev || !pdev->vpci )
     {
-        vpci_write_hw(sbdf, reg, size, data);
+        /* Ignore writes to read-only devices, which have no ->vpci. */
+        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
+
+        if ( !ro_map || !test_bit(sbdf.bdf, ro_map) )
+            vpci_write_hw(sbdf, reg, size, data);
         return;
     }