* [PATCH v5 1/9] vpci: move BAR mapping permissions checks
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-23 14:44 ` Jan Beulich
2026-07-08 21:02 ` [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain Stewart Hildebrand
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Stewart Hildebrand, Roger Pau Monné, Daniel P. Smith
There's no need to defer the permissions checks. Perform them right away
in modify_bars().
As a result of the move, the permissions checks will now cover the whole
BAR before any holes are removed.
Carry over the domain_crash() in the error path for domUs.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v5: new patch
---
xen/drivers/vpci/header.c | 40 +++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index d1c92cf77f8a..5ac525036672 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -58,24 +58,6 @@ static int cf_check map_range(
* offset of the current address from the BAR start.
*/
unsigned long map_mfn = start_mfn + s - start_gfn;
- unsigned long m_end = map_mfn + size - 1;
-
- if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
- {
- printk(XENLOG_G_WARNING
- "%pd denied access to MMIO range [%#lx, %#lx]\n",
- map->d, map_mfn, m_end);
- return -EPERM;
- }
-
- rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
- if ( rc )
- {
- printk(XENLOG_G_WARNING
- "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
- map->d, map_mfn, m_end, rc);
- return rc;
- }
/*
* ARM TODOs:
@@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
return -EINVAL;
}
+ if ( !iomem_access_permitted(pdev->domain, start, end) )
+ {
+ printk(XENLOG_G_WARNING
+ "%pd denied access to MMIO range [%#lx, %#lx]\n",
+ pdev->domain, start, end);
+ if ( !is_hardware_domain(pdev->domain) )
+ domain_crash(pdev->domain);
+ return -EPERM;
+ }
+
+ rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
+ !!(cmd & PCI_COMMAND_MEMORY));
+ if ( rc )
+ {
+ printk(XENLOG_G_WARNING
+ "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
+ pdev->domain, start, end, rc);
+ if ( !is_hardware_domain(pdev->domain) )
+ domain_crash(pdev->domain);
+ return rc;
+ }
+
rc = rangeset_add_range(bar->mem, start_guest, end_guest);
if ( rc )
{
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks
2026-07-08 21:02 ` [PATCH v5 1/9] vpci: move BAR mapping permissions checks Stewart Hildebrand
@ 2026-07-23 14:44 ` Jan Beulich
2026-07-23 14:57 ` Roger Pau Monné
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2026-07-23 14:44 UTC (permalink / raw)
To: Stewart Hildebrand; +Cc: Roger Pau Monné, Daniel P. Smith, xen-devel
On 08.07.2026 23:02, Stewart Hildebrand wrote:
> There's no need to defer the permissions checks. Perform them right away
> in modify_bars().
>
> As a result of the move, the permissions checks will now cover the whole
> BAR before any holes are removed.
Is this a good idea though? A really good toolstack would avoid granting
access to e.g. the MSI-X table.
> Carry over the domain_crash() in the error path for domUs.
I don't think this is warranted.
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -58,24 +58,6 @@ static int cf_check map_range(
> * offset of the current address from the BAR start.
> */
> unsigned long map_mfn = start_mfn + s - start_gfn;
> - unsigned long m_end = map_mfn + size - 1;
> -
> - if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> - {
> - printk(XENLOG_G_WARNING
> - "%pd denied access to MMIO range [%#lx, %#lx]\n",
> - map->d, map_mfn, m_end);
> - return -EPERM;
> - }
> -
> - rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> - if ( rc )
> - {
> - printk(XENLOG_G_WARNING
> - "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> - map->d, map_mfn, m_end, rc);
I realize you literally only take what's here, but ...
> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> return -EINVAL;
> }
>
> + if ( !iomem_access_permitted(pdev->domain, start, end) )
> + {
> + printk(XENLOG_G_WARNING
> + "%pd denied access to MMIO range [%#lx, %#lx]\n",
> + pdev->domain, start, end);
> + if ( !is_hardware_domain(pdev->domain) )
> + domain_crash(pdev->domain);
> + return -EPERM;
> + }
> +
> + rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> + !!(cmd & PCI_COMMAND_MEMORY));
> + if ( rc )
> + {
> + printk(XENLOG_G_WARNING
> + "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> + pdev->domain, start, end, rc);
... is the logging of rc actually useful? Afaict it's only ever going to be
-EPERM.
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks
2026-07-23 14:44 ` Jan Beulich
@ 2026-07-23 14:57 ` Roger Pau Monné
2026-07-23 15:06 ` Jan Beulich
2026-07-23 20:09 ` Daniel P. Smith
0 siblings, 2 replies; 17+ messages in thread
From: Roger Pau Monné @ 2026-07-23 14:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Stewart Hildebrand, Daniel P. Smith, xen-devel
On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
> On 08.07.2026 23:02, Stewart Hildebrand wrote:
> > There's no need to defer the permissions checks. Perform them right away
> > in modify_bars().
> >
> > As a result of the move, the permissions checks will now cover the whole
> > BAR before any holes are removed.
>
> Is this a good idea though? A really good toolstack would avoid granting
> access to e.g. the MSI-X table.
>
> > Carry over the domain_crash() in the error path for domUs.
>
> I don't think this is warranted.
>
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -58,24 +58,6 @@ static int cf_check map_range(
> > * offset of the current address from the BAR start.
> > */
> > unsigned long map_mfn = start_mfn + s - start_gfn;
> > - unsigned long m_end = map_mfn + size - 1;
> > -
> > - if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> > - {
> > - printk(XENLOG_G_WARNING
> > - "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > - map->d, map_mfn, m_end);
> > - return -EPERM;
> > - }
> > -
> > - rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> > - if ( rc )
> > - {
> > - printk(XENLOG_G_WARNING
> > - "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > - map->d, map_mfn, m_end, rc);
>
> I realize you literally only take what's here, but ...
>
> > @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> > return -EINVAL;
> > }
> >
> > + if ( !iomem_access_permitted(pdev->domain, start, end) )
> > + {
> > + printk(XENLOG_G_WARNING
> > + "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > + pdev->domain, start, end);
> > + if ( !is_hardware_domain(pdev->domain) )
> > + domain_crash(pdev->domain);
> > + return -EPERM;
> > + }
> > +
> > + rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> > + !!(cmd & PCI_COMMAND_MEMORY));
> > + if ( rc )
> > + {
> > + printk(XENLOG_G_WARNING
> > + "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > + pdev->domain, start, end, rc);
>
> ... is the logging of rc actually useful? Afaict it's only ever going to be
> -EPERM.
I think Flask can also return -EACCES, not that it's very relevant I'm
afraid. It's likely too late now, but those XSM checks should instead
return a bool to avoid this kind of confusion with the returned
error? (or whether diverse return codes are indeed expected).
Thanks, Roger.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks
2026-07-23 14:57 ` Roger Pau Monné
@ 2026-07-23 15:06 ` Jan Beulich
2026-07-23 20:09 ` Daniel P. Smith
1 sibling, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2026-07-23 15:06 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Stewart Hildebrand, Daniel P. Smith, xen-devel
On 23.07.2026 16:57, Roger Pau Monné wrote:
> On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
>> On 08.07.2026 23:02, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -58,24 +58,6 @@ static int cf_check map_range(
>>> * offset of the current address from the BAR start.
>>> */
>>> unsigned long map_mfn = start_mfn + s - start_gfn;
>>> - unsigned long m_end = map_mfn + size - 1;
>>> -
>>> - if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
>>> - {
>>> - printk(XENLOG_G_WARNING
>>> - "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> - map->d, map_mfn, m_end);
>>> - return -EPERM;
>>> - }
>>> -
>>> - rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
>>> - if ( rc )
>>> - {
>>> - printk(XENLOG_G_WARNING
>>> - "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> - map->d, map_mfn, m_end, rc);
>>
>> I realize you literally only take what's here, but ...
>>
>>> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>> return -EINVAL;
>>> }
>>>
>>> + if ( !iomem_access_permitted(pdev->domain, start, end) )
>>> + {
>>> + printk(XENLOG_G_WARNING
>>> + "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> + pdev->domain, start, end);
>>> + if ( !is_hardware_domain(pdev->domain) )
>>> + domain_crash(pdev->domain);
>>> + return -EPERM;
>>> + }
>>> +
>>> + rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
>>> + !!(cmd & PCI_COMMAND_MEMORY));
>>> + if ( rc )
>>> + {
>>> + printk(XENLOG_G_WARNING
>>> + "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> + pdev->domain, start, end, rc);
>>
>> ... is the logging of rc actually useful? Afaict it's only ever going to be
>> -EPERM.
>
> I think Flask can also return -EACCES, not that it's very relevant I'm
> afraid.
Hmm, looks like this can indeed happen. Please disregard this comment of
mine then.
> It's likely too late now, but those XSM checks should instead
> return a bool to avoid this kind of confusion with the returned
> error? (or whether diverse return codes are indeed expected).
Depends on whether having distinct error codes is wanted. Daniel?
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks
2026-07-23 14:57 ` Roger Pau Monné
2026-07-23 15:06 ` Jan Beulich
@ 2026-07-23 20:09 ` Daniel P. Smith
1 sibling, 0 replies; 17+ messages in thread
From: Daniel P. Smith @ 2026-07-23 20:09 UTC (permalink / raw)
To: Roger Pau Monné, Jan Beulich; +Cc: Stewart Hildebrand, xen-devel
On 7/23/26 10:57 AM, Roger Pau Monné wrote:
> On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
>> On 08.07.2026 23:02, Stewart Hildebrand wrote:
>>> There's no need to defer the permissions checks. Perform them right away
>>> in modify_bars().
>>>
>>> As a result of the move, the permissions checks will now cover the whole
>>> BAR before any holes are removed.
>>
>> Is this a good idea though? A really good toolstack would avoid granting
>> access to e.g. the MSI-X table.
>>
>>> Carry over the domain_crash() in the error path for domUs.
>>
>> I don't think this is warranted.
>>
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -58,24 +58,6 @@ static int cf_check map_range(
>>> * offset of the current address from the BAR start.
>>> */
>>> unsigned long map_mfn = start_mfn + s - start_gfn;
>>> - unsigned long m_end = map_mfn + size - 1;
>>> -
>>> - if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
>>> - {
>>> - printk(XENLOG_G_WARNING
>>> - "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> - map->d, map_mfn, m_end);
>>> - return -EPERM;
>>> - }
>>> -
>>> - rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
>>> - if ( rc )
>>> - {
>>> - printk(XENLOG_G_WARNING
>>> - "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> - map->d, map_mfn, m_end, rc);
>>
>> I realize you literally only take what's here, but ...
>>
>>> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>> return -EINVAL;
>>> }
>>>
>>> + if ( !iomem_access_permitted(pdev->domain, start, end) )
>>> + {
>>> + printk(XENLOG_G_WARNING
>>> + "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> + pdev->domain, start, end);
>>> + if ( !is_hardware_domain(pdev->domain) )
>>> + domain_crash(pdev->domain);
>>> + return -EPERM;
>>> + }
>>> +
>>> + rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
>>> + !!(cmd & PCI_COMMAND_MEMORY));
>>> + if ( rc )
>>> + {
>>> + printk(XENLOG_G_WARNING
>>> + "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> + pdev->domain, start, end, rc);
>>
>> ... is the logging of rc actually useful? Afaict it's only ever going to be
>> -EPERM.
>
> I think Flask can also return -EACCES, not that it's very relevant I'm
> afraid. It's likely too late now, but those XSM checks should instead
> return a bool to avoid this kind of confusion with the returned
> error? (or whether diverse return codes are indeed expected).
An XSM access check may return -EACCES as one of any number of error
codes depending on which XSM module is loaded. I haven't traced this
specific path but a quick grep for the direct FLASK hooks shows EPERM,
EINVAL, ENOMEM, and ENOENT directly in the hooks themselves. This does
not even include the error codes that could come from the AVC. This is a
consist interface provided by all XSM calls and fairly standard behavior
across any number of respectable kernels such as Linux.
v/r,
dps
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 1/9] vpci: move BAR mapping permissions checks Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-23 14:55 ` Jan Beulich
2026-07-08 21:02 ` [PATCH v5 3/9] vpci: only check BAR validity once Stewart Hildebrand
` (6 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Stewart Hildebrand
From: Roger Pau Monne <roger.pau@citrix.com>
The logic in map_range() will bubble up failures to the upper layer, which
will result in any remaining regions being skip, and for the non-hardware
domain case the owner domain of the device would be destroyed. However for
the hardware domain the intent is to continue execution, hoping the
failure to modify the p2m could be worked around by the hardware domain.
To accomplish that in a better way, ignore failures and skip the range in
that case, possibly continuing to map further ranges.
Since the error path in vpci_process_pending() should only be used by domUs
now, and it will unconditionally end up calling domain_crash(), simplify
it: there's no need to cleanup if the domain will be destroyed.
Memory decoding may be left enabled in case of mapping error for devices
assigned to domUs.
No functional change for domUs intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Although not strictly a pre-requisite for queued p2m mapping, I've
included this patch from Roger's vPCI fixes series since it simplifies
the error path in vpci_process_pending().
v1->v5:
* move "*c += size;" outside is_hardware_domain condition
* add note to commit message about not disabling memory decoding on failure
---
xen/drivers/vpci/header.c | 45 +++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 5ac525036672..9f2d0c8a5452 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -70,17 +70,26 @@ static int cf_check map_range(
rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn))
: unmap_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn));
- if ( rc == 0 )
- {
- *c += size;
- break;
- }
if ( rc < 0 )
{
printk(XENLOG_G_WARNING
"Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
map->map ? "" : "un", s, e, map_mfn,
map_mfn + size, map->d, rc);
+ goto done;
+ }
+ if ( rc == 0 )
+ {
+ done:
+ if ( is_hardware_domain(map->d) )
+ /*
+ * Ignore failures for the hardware domain and skip the range.
+ * Do it as a best effort workaround to attempt to get the
+ * hardware domain to boot.
+ */
+ rc = 0;
+
+ *c += size;
break;
}
ASSERT(rc < size);
@@ -196,28 +205,22 @@ bool vpci_process_pending(struct vcpu *v)
return true;
}
- if ( rc )
+ if ( rc && !is_hardware_domain(v->domain) )
{
- spin_lock(&pdev->vpci->lock);
- /* Disable memory decoding unconditionally on failure. */
- modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
- false);
- spin_unlock(&pdev->vpci->lock);
-
- /* Clean all the rangesets */
- for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
- if ( !rangeset_is_empty(header->bars[i].mem) )
- rangeset_purge(header->bars[i].mem);
-
- v->vpci.pdev = NULL;
-
read_unlock(&v->domain->pci_lock);
- if ( !is_hardware_domain(v->domain) )
- domain_crash(v->domain);
+ domain_crash(v->domain);
return false;
}
+ ASSERT(!rc);
+ /*
+ * Purge rangeset to deal with the hardware domain having triggered an
+ * error. It shouldn't be possible, as map_range() will always swallow
+ * errors for hardware domain owned devices, and
+ * rangeset_consume_ranges() itself doesn't generate any errors.
+ */
+ rangeset_purge(bar->mem);
}
v->vpci.pdev = NULL;
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain
2026-07-08 21:02 ` [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain Stewart Hildebrand
@ 2026-07-23 14:55 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2026-07-23 14:55 UTC (permalink / raw)
To: Stewart Hildebrand; +Cc: Roger Pau Monné, xen-devel
On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> The logic in map_range() will bubble up failures to the upper layer, which
> will result in any remaining regions being skip, and for the non-hardware
> domain case the owner domain of the device would be destroyed. However for
> the hardware domain the intent is to continue execution, hoping the
> failure to modify the p2m could be worked around by the hardware domain.
>
> To accomplish that in a better way, ignore failures and skip the range in
> that case, possibly continuing to map further ranges.
>
> Since the error path in vpci_process_pending() should only be used by domUs
> now, and it will unconditionally end up calling domain_crash(), simplify
> it: there's no need to cleanup if the domain will be destroyed.
Aren't we increasing the number of places adjustments will need making if
that (not really nice) crashing of the domain was sorted at some point?
> Memory decoding may be left enabled in case of mapping error for devices
> assigned to domUs.
I'm similarly concerned of this. I also can't spot why this would be for
DomU-s only. The original code did it for all domains.
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -70,17 +70,26 @@ static int cf_check map_range(
>
> rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn))
> : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn));
> - if ( rc == 0 )
> - {
> - *c += size;
> - break;
> - }
> if ( rc < 0 )
> {
> printk(XENLOG_G_WARNING
> "Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
> map->map ? "" : "un", s, e, map_mfn,
> map_mfn + size, map->d, rc);
> + goto done;
> + }
> + if ( rc == 0 )
> + {
> + done:
> + if ( is_hardware_domain(map->d) )
> + /*
> + * Ignore failures for the hardware domain and skip the range.
> + * Do it as a best effort workaround to attempt to get the
> + * hardware domain to boot.
> + */
> + rc = 0;
Why would this not move ahead of the "goto done"? For the if() here it's
a no-op. Then (really: independently) ...
> + *c += size;
> break;
> }
... the remaining piece could be in "if ( rc <= 0 )".
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 3/9] vpci: only check BAR validity once
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 1/9] vpci: move BAR mapping permissions checks Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-23 15:46 ` Jan Beulich
2026-07-08 21:02 ` [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable Stewart Hildebrand
` (5 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Stewart Hildebrand, Andrew Cooper
From: Roger Pau Monne <roger.pau@citrix.com>
The BAR validity checks in modify_decoding() are redundant since they
are already done in modify_bars().
Avoid multiple calls to pci_check_bar() for the same memory decoding
related operation, as each call can possibly print a warning message
about a BAR being in an invalid position.
Further, there's no need to defer setting bar->enabled until p2m
operations are finished. It can safely be set in modify_bars() itself.
This slightly changes the meaning of the bar->enabled flag: when true,
it means a BAR is either mapped or queued to be mapped. Store whether
the BAR is validly positioned in modify_bars(), and use the cached value
for setting bar->enabled.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Fixes: 4acab25a9300 ('x86/vpci: fix handling of BAR overlaps with non-hole regions')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v5:
* rebase
* don't defer setting bar->enabled
* use local variable for bar_valid flag
Original report [1]:
[1] https://lore.kernel.org/xen-devel/dbc003a2-9202-46ec-bf87-2829d8a63d53@citrix.com/
---
xen/drivers/vpci/header.c | 18 +++++++++---------
xen/include/xen/vpci.h | 2 +-
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 9f2d0c8a5452..1fd4832033e9 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -128,19 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
uint32_t val = bar->addr |
(map ? PCI_ROM_ADDRESS_ENABLE : 0);
- if ( pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
- _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
- bar->enabled = map;
header->rom_enabled = map;
pci_conf_write32(pdev->sbdf, rom_pos, val);
return;
}
-
- if ( !rom_only &&
- (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;
}
/*
@@ -296,6 +287,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
struct pci_dev *tmp;
const struct domain *d;
const struct vpci_msix *msix = pdev->vpci->msix;
+ bool bar_valid[ARRAY_SIZE(header->bars)] = { false };
unsigned int i, j;
int rc;
@@ -341,6 +333,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
ASSERT(rangeset_is_empty(bar->mem));
+ bar_valid[i] = true;
+
/*
* Make sure that the guest set address has the same page offset
* as the physical address on the host or otherwise things won't work as
@@ -506,6 +500,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
d = dom_xen;
}
+ for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ {
+ if ( bar_valid[i] )
+ header->bars[i].enabled = cmd & PCI_COMMAND_MEMORY;
+ }
+
if ( system_state < SYS_STATE_active )
{
/*
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 877aa391d178..ab94eb60d65f 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -64,7 +64,7 @@ struct vpci {
VPCI_BAR_ROM,
} type;
bool prefetchable : 1;
- /* Store whether the BAR is mapped into guest p2m. */
+ /* Whether the BAR is mapped or queued for mapping in guest p2m. */
bool enabled : 1;
} bars[PCI_HEADER_NORMAL_NR_BARS + 1];
/* At most 6 BARS + 1 expansion ROM BAR. */
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v5 3/9] vpci: only check BAR validity once
2026-07-08 21:02 ` [PATCH v5 3/9] vpci: only check BAR validity once Stewart Hildebrand
@ 2026-07-23 15:46 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2026-07-23 15:46 UTC (permalink / raw)
To: Stewart Hildebrand; +Cc: Roger Pau Monné, Andrew Cooper, xen-devel
On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> The BAR validity checks in modify_decoding() are redundant since they
> are already done in modify_bars().
>
> Avoid multiple calls to pci_check_bar() for the same memory decoding
> related operation, as each call can possibly print a warning message
> about a BAR being in an invalid position.
>
> Further, there's no need to defer setting bar->enabled until p2m
> operations are finished. It can safely be set in modify_bars() itself.
> This slightly changes the meaning of the bar->enabled flag: when true,
> it means a BAR is either mapped or queued to be mapped.
But are all consumers of the flag okay with this semantic change? What
would clear the flag if the mapping failed? It looks to me as if e.g.
msix_find()'s behavior would change then.
> @@ -296,6 +287,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> struct pci_dev *tmp;
> const struct domain *d;
> const struct vpci_msix *msix = pdev->vpci->msix;
> + bool bar_valid[ARRAY_SIZE(header->bars)] = { false };
Just {} ?
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (2 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 3/9] vpci: only check BAR validity once Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-23 15:59 ` Jan Beulich
2026-07-08 21:02 ` [PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes Stewart Hildebrand
` (4 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Stewart Hildebrand
From: Roger Pau Monne <roger.pau@citrix.com>
Deferring the p2m unpopulate of the MSI-X table region to after the BARs are
mapped in the p2m is not needed. The aim of vpci_make_msix_hole() is to
cope with the BAR containing the MSI-X table being positioned over a
reserved e820 region, by making sure the MSI-X table region is
unpopulated on the p2m. Note that reserved e820 regions are identity mapped
in the hardware domain p2m by default.
Doing it before or after the BAR p2m changes are done is irrelevant, hence
do it ahead of the p2m changes, as that's simpler.
Note that vpci_make_msix_hole() should be invoked strictly after setting
bar->enabled. The movement of vpci_make_msix_hole() is OK since
bar->enabled is set in modify_bars().
Take the opportunity to store the memory decoding enabled bit in a local
variable.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Note that this should not be committed until after the bar->enabled
setting movement in ("xen/vpci: only check BAR validity once")
I tested this with hot-plug as described in 7c24d58a6e64 ("vpci/msix:
check for BARs enabled in vpci_make_msix_hole").
The return at the end of the function will gain a purpose again after a
later code movement patch in the series.
The new_enabled variable will gain 2 additional uses later in the series.
v1->v5:
* order after ("xen/vpci: only check BAR validity once")
* preserve behavior of only invoking vpci_make_msix_hole() when memory
decoding toggles from 0 to 1
* add note about vpci_make_msix_hole() invocation after setting bar->enabled
* use local variable
* Jan offered an R-b tag in [1], though I hesitated to pick it up due to
changes, and it was offered prior to 7c24d58a6e64 going upstream
[1] https://lore.kernel.org/xen-devel/e2c4944d-d089-490c-879d-b6baec5e4cae@suse.com/
---
xen/drivers/vpci/header.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 1fd4832033e9..cf9d0bac8876 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -134,19 +134,6 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
}
}
- /*
- * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
- * can be trapped (and emulated) by Xen when the memory decoding bit is
- * enabled.
- *
- * FIXME: punching holes after the p2m has been set up might be racy for
- * DomU usage, needs to be revisited.
- */
-#ifdef CONFIG_HAS_PCI_MSI
- if ( map && !rom_only && vpci_make_msix_hole(pdev) )
- return;
-#endif
-
if ( !rom_only )
{
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
@@ -529,6 +516,7 @@ static void cf_check cmd_write(
const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
{
struct vpci_header *header = data;
+ bool new_enabled = cmd & PCI_COMMAND_MEMORY;
if ( !is_hardware_domain(pdev->domain) )
{
@@ -546,7 +534,7 @@ 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 != !!(cmd & PCI_COMMAND_MEMORY) )
+ if ( header->bars_mapped != new_enabled )
/*
* 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
@@ -556,6 +544,12 @@ static void cf_check cmd_write(
modify_bars(pdev, cmd, false);
else
pci_conf_write16(pdev->sbdf, reg, cmd);
+
+#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
}
static uint32_t cf_check guest_cmd_read(
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable
2026-07-08 21:02 ` [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable Stewart Hildebrand
@ 2026-07-23 15:59 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2026-07-23 15:59 UTC (permalink / raw)
To: Stewart Hildebrand; +Cc: Roger Pau Monné, xen-devel
On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> Deferring the p2m unpopulate of the MSI-X table region to after the BARs are
> mapped in the p2m is not needed. The aim of vpci_make_msix_hole() is to
> cope with the BAR containing the MSI-X table being positioned over a
> reserved e820 region, by making sure the MSI-X table region is
> unpopulated on the p2m. Note that reserved e820 regions are identity mapped
> in the hardware domain p2m by default.
>
> Doing it before or after the BAR p2m changes are done is irrelevant, hence
> do it ahead of the p2m changes, as that's simpler.
>
> Note that vpci_make_msix_hole() should be invoked strictly after setting
> bar->enabled. The movement of vpci_make_msix_hole() is OK since
> bar->enabled is set in modify_bars().
>
> Take the opportunity to store the memory decoding enabled bit in a local
> variable.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> Note that this should not be committed until after the bar->enabled
> setting movement in ("xen/vpci: only check BAR validity once")
>
> I tested this with hot-plug as described in 7c24d58a6e64 ("vpci/msix:
> check for BARs enabled in vpci_make_msix_hole").
>
> The return at the end of the function will gain a purpose again after a
> later code movement patch in the series.
>
> The new_enabled variable will gain 2 additional uses later in the series.
>
> v1->v5:
> * order after ("xen/vpci: only check BAR validity once")
> * preserve behavior of only invoking vpci_make_msix_hole() when memory
> decoding toggles from 0 to 1
> * add note about vpci_make_msix_hole() invocation after setting bar->enabled
> * use local variable
> * Jan offered an R-b tag in [1], though I hesitated to pick it up due to
> changes, and it was offered prior to 7c24d58a6e64 going upstream
> [1] https://lore.kernel.org/xen-devel/e2c4944d-d089-490c-879d-b6baec5e4cae@suse.com/
If there wasn't ...
> @@ -556,6 +544,12 @@ static void cf_check cmd_write(
> modify_bars(pdev, cmd, false);
> else
> pci_conf_write16(pdev->sbdf, reg, cmd);
> +
> +#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
... the new first part of the conditional, I'd re-offer R-b right away. With
that check added, aren't we at risk of leaving the range mapped when it
shouldn't be?
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (3 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 6/9] vpci: create mem local variables Stewart Hildebrand
` (3 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Stewart Hildebrand
From: Roger Pau Monne <roger.pau@citrix.com>
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@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v5 6/9] vpci: create mem local variables
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (4 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 7/9] vpci: split vpci_process_pending() Stewart Hildebrand
` (2 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Stewart Hildebrand, Roger Pau Monné, Mykyta Poturai
Use local variables to reference struct rangeset *mem in preparation of
moving it out of struct vpci. No functional change.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v4->v5:
* Patch adapted from [1] ("vpci: Use pervcpu ranges for BAR mapping").
I kept Mykyta's S-o-b tag since many of the changes are similar.
[1] https://lore.kernel.org/xen-devel/20260406191203.97662-2-stewart.hildebrand@amd.com/
---
xen/drivers/vpci/header.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 908adf0b3f4c..28e051f2779c 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -124,6 +124,7 @@ bool vpci_process_pending(struct vcpu *v)
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
struct vpci_bar *bar = &header->bars[i];
+ struct rangeset *mem = bar->mem;
struct map_data data = {
.d = v->domain,
.map = v->vpci.cmd & PCI_COMMAND_MEMORY,
@@ -131,10 +132,10 @@ bool vpci_process_pending(struct vcpu *v)
};
int rc;
- if ( rangeset_is_empty(bar->mem) )
+ if ( rangeset_is_empty(mem) )
continue;
- rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+ rc = rangeset_consume_ranges(mem, map_range, &data);
if ( rc == -ERESTART )
{
@@ -178,13 +179,14 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
struct vpci_bar *bar = &header->bars[i];
+ struct rangeset *mem = bar->mem;
struct map_data data = { .d = d, .map = true, .bar = bar };
- if ( rangeset_is_empty(bar->mem) )
+ if ( rangeset_is_empty(mem) )
continue;
- while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
- &data)) == -ERESTART )
+ while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) ==
+ -ERESTART )
{
/*
* It's safe to drop and reacquire the lock in this context
@@ -248,12 +250,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
struct vpci_bar *bar = &header->bars[i];
+ struct rangeset *mem = bar->mem;
unsigned long start = PFN_DOWN(bar->addr);
unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
unsigned long start_guest = PFN_DOWN(bar->guest_addr);
unsigned long end_guest = PFN_DOWN(bar->guest_addr + bar->size - 1);
- if ( !bar->mem )
+ if ( !mem )
continue;
if ( !MAPPABLE_BAR(bar) ||
@@ -271,7 +274,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
continue;
}
- ASSERT(rangeset_is_empty(bar->mem));
+ ASSERT(rangeset_is_empty(mem));
bar_valid[i] = true;
@@ -310,7 +313,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
return rc;
}
- rc = rangeset_add_range(bar->mem, start_guest, end_guest);
+ rc = rangeset_add_range(mem, start_guest, end_guest);
if ( rc )
{
printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",
@@ -321,12 +324,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
/* Check for overlap with the already setup BAR ranges. */
for ( j = 0; j < i; j++ )
{
- struct vpci_bar *prev_bar = &header->bars[j];
+ struct rangeset *prev_mem = header->bars[j].mem;
- if ( rangeset_is_empty(prev_bar->mem) )
+ if ( rangeset_is_empty(prev_mem) )
continue;
- rc = rangeset_remove_range(prev_bar->mem, start_guest, end_guest);
+ rc = rangeset_remove_range(prev_mem, start_guest, end_guest);
if ( rc )
{
gprintk(XENLOG_WARNING,
@@ -336,7 +339,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
}
}
- rc = pci_sanitize_bar_memory(bar->mem);
+ rc = pci_sanitize_bar_memory(mem);
if ( rc )
{
gprintk(XENLOG_WARNING,
@@ -355,12 +358,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
{
- const struct vpci_bar *bar = &header->bars[j];
+ struct rangeset *mem = header->bars[j].mem;
- if ( rangeset_is_empty(bar->mem) )
+ if ( rangeset_is_empty(mem) )
continue;
- rc = rangeset_remove_range(bar->mem, start, end);
+ rc = rangeset_remove_range(mem, start, end);
if ( rc )
{
gprintk(XENLOG_WARNING,
@@ -410,8 +413,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( j = 0; j < ARRAY_SIZE(header->bars); j++)
{
const struct vpci_bar *bar = &header->bars[j];
+ struct rangeset *mem = bar->mem;
- if ( !rangeset_overlaps_range(bar->mem, start, end) ||
+ if ( !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
@@ -422,7 +426,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
bar->type == VPCI_BAR_ROM) )
continue;
- rc = rangeset_remove_range(bar->mem, start, end);
+ rc = rangeset_remove_range(mem, start, end);
if ( rc )
{
gprintk(XENLOG_WARNING,
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v5 7/9] vpci: split vpci_process_pending()
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (5 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 6/9] vpci: create mem local variables Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 8/9] vpci: allow queueing of mapping operations Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 9/9] vpci: don't pass command value to modify_bars() Stewart Hildebrand
8 siblings, 0 replies; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Stewart Hildebrand, Roger Pau Monné
Create a new function to separate the mapping logic from the pdev
validation and lock-acquiring logic. Slightly consolidate exit paths for
hardware_domain and !hardware_domain.
No functional change.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* new patch
---
xen/drivers/vpci/header.c | 65 +++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 28e051f2779c..f7eaf9c6ddf2 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -102,23 +102,12 @@ static int cf_check map_range(
return rc;
}
-bool vpci_process_pending(struct vcpu *v)
+static int process_map_task(const struct pci_dev *pdev, bool map)
{
- const struct pci_dev *pdev = v->vpci.pdev;
struct vpci_header *header = NULL;
unsigned int i;
- if ( !pdev )
- return false;
-
- read_lock(&v->domain->pci_lock);
-
- if ( !pdev->vpci || (v->domain != pdev->domain) )
- {
- v->vpci.pdev = NULL;
- read_unlock(&v->domain->pci_lock);
- return false;
- }
+ ASSERT(rw_is_locked(&pdev->domain->pci_lock));
header = &pdev->vpci->header;
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
@@ -126,8 +115,8 @@ bool vpci_process_pending(struct vcpu *v)
struct vpci_bar *bar = &header->bars[i];
struct rangeset *mem = bar->mem;
struct map_data data = {
- .d = v->domain,
- .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
+ .d = pdev->domain,
+ .map = map,
.bar = bar,
};
int rc;
@@ -136,21 +125,12 @@ bool vpci_process_pending(struct vcpu *v)
continue;
rc = rangeset_consume_ranges(mem, map_range, &data);
-
if ( rc == -ERESTART )
- {
- read_unlock(&v->domain->pci_lock);
- return true;
- }
-
- if ( rc && !is_hardware_domain(v->domain) )
- {
- read_unlock(&v->domain->pci_lock);
+ return rc;
- domain_crash(v->domain);
+ if ( rc && !is_hardware_domain(pdev->domain) )
+ return rc;
- return false;
- }
ASSERT(!rc);
/*
* Purge rangeset to deal with the hardware domain having triggered an
@@ -160,10 +140,41 @@ bool vpci_process_pending(struct vcpu *v)
*/
rangeset_purge(bar->mem);
}
+
+ return 0;
+}
+
+bool vpci_process_pending(struct vcpu *v)
+{
+ const struct pci_dev *pdev = v->vpci.pdev;
+ int rc;
+
+ if ( !pdev )
+ return false;
+
+ read_lock(&v->domain->pci_lock);
+
+ if ( !pdev->vpci || (v->domain != pdev->domain) )
+ {
+ v->vpci.pdev = NULL;
+ read_unlock(&v->domain->pci_lock);
+ return false;
+ }
+
+ rc = process_map_task(pdev, v->vpci.cmd & PCI_COMMAND_MEMORY);
+ if ( rc == -ERESTART )
+ {
+ read_unlock(&v->domain->pci_lock);
+ return true;
+ }
+
v->vpci.pdev = NULL;
read_unlock(&v->domain->pci_lock);
+ if ( rc && !is_hardware_domain(v->domain) )
+ domain_crash(v->domain);
+
return false;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v5 8/9] vpci: allow queueing of mapping operations
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (6 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 7/9] vpci: split vpci_process_pending() Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
2026-07-08 21:02 ` [PATCH v5 9/9] vpci: don't pass command value to modify_bars() Stewart Hildebrand
8 siblings, 0 replies; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Mykyta Poturai
Introduce vPCI BAR mapping task queue. Store information needed to
map/unmap BARs in struct map_task. Allow queueing of BAR map/unmap
operations in a queue, thus making it possible to perform multiple p2m
operations associated with single PCI config space register access.
This is preparatory work for further changes that need to perform
multiple unmap/map operations before returning to guest.
While queueing multiple operations is valid, at the moment, only a
single operation will be queued.
Removed rangeset names, since RANGESETF_no_print was set anyway.
Amends: 622bdd962822 ("vpci/header: handle p2m range sets per BAR")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
As a side effect of this, the ASSERT(rangeset_is_empty(mem)) in
modify_bars() is no longer bogus (see [2]).
v4->v5:
* split non-functional pre-requisites out into separate patches
* move pdev into struct map_task in anticipation of mapping different pdevs
corresponding to different VFs
* move call to vpci_vcpu_destroy() from vcpu_destroy() to vcpu_teardown()
v3->v4:
* switch back to dynamically allocated queue elements
v2->v3:
* rebase on ("vpci: Use pervcpu ranges for BAR mapping") from [1]
* rework with fixed array of map/unmap slots
[1] https://lore.kernel.org/xen-devel/cover.1772806036.git.mykyta_poturai@epam.com/T/#t
[2] https://lore.kernel.org/xen-devel/20250814160358.95543-2-roger.pau@citrix.com/
v1->v2:
* new patch
---
xen/common/domain.c | 5 +
xen/drivers/vpci/header.c | 222 +++++++++++++++++++++++++------------
xen/drivers/vpci/private.h | 1 +
xen/drivers/vpci/vpci.c | 17 ++-
xen/include/xen/vpci.h | 13 ++-
5 files changed, 180 insertions(+), 78 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 53ac1d6c4034..d576c2a40fdf 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -36,6 +36,7 @@
#include <xen/argo.h>
#include <xen/llc-coloring.h>
#include <xen/xvmalloc.h>
+#include <xen/vpci.h>
#include <asm/p2m.h>
#include <asm/processor.h>
#include <public/sched.h>
@@ -439,6 +440,8 @@ static int vmtrace_alloc_buffer(struct vcpu *v)
*/
static int vcpu_teardown(struct vcpu *v)
{
+ vpci_vcpu_destroy(v);
+
vmtrace_free_buffer(v);
return 0;
@@ -508,6 +511,8 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
if ( arch_vcpu_create(v) != 0 )
goto fail_sched;
+ vpci_vcpu_init(v);
+
d->vcpu[vcpu_id] = v;
if ( vcpu_id != 0 )
{
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index f7eaf9c6ddf2..99ce67053068 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -23,6 +23,7 @@
#include <xen/lib.h>
#include <xen/sched.h>
#include <xen/softirq.h>
+#include <xen/xvmalloc.h>
#include <xsm/xsm.h>
@@ -33,9 +34,21 @@
((x)->type == VPCI_BAR_MEM32 || (x)->type == VPCI_BAR_MEM64_LO || \
(x)->type == VPCI_BAR_ROM)
+struct map_task {
+ /* Structure to store state while {un}mapping PCI BARs. */
+ struct list_head next; /* List element in v->vpci.task_queue */
+ const struct pci_dev *pdev;
+ struct bar_map {
+ uint64_t addr;
+ uint64_t guest_addr;
+ struct rangeset *mem;
+ } bars[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];
+ bool map : 1;
+};
+
struct map_data {
struct domain *d;
- const struct vpci_bar *bar;
+ const struct bar_map *bar;
bool map;
};
@@ -102,21 +115,82 @@ static int cf_check map_range(
return rc;
}
-static int process_map_task(const struct pci_dev *pdev, bool map)
+static void destroy_map_task(struct map_task *task)
+{
+ unsigned int i;
+
+ for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+ rangeset_destroy(task->bars[i].mem);
+
+ xvfree(task);
+}
+
+void vpci_clear_map_queue(struct vcpu *v, const struct pci_dev *pdev)
+{
+ struct map_task *task, *tmp;
+
+ list_for_each_entry_safe ( task, tmp, &v->vpci.task_queue, next )
+ {
+ if ( !pdev || task->pdev == pdev )
+ {
+ list_del(&task->next);
+ destroy_map_task(task);
+ }
+ }
+}
+
+static struct map_task *alloc_map_task(const struct pci_dev *pdev, bool map,
+ bool rom_only)
+{
+ struct map_task *task;
+ unsigned int i;
+
+ task = xvzalloc(struct map_task);
+ if ( !task )
+ return NULL;
+
+ task->pdev = pdev;
+
+ for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+ {
+ if ( !MAPPABLE_BAR(&pdev->vpci->header.bars[i]) )
+ continue;
+
+ if ( rom_only && pdev->vpci->header.bars[i].type != VPCI_BAR_ROM )
+ continue;
+
+ task->bars[i].mem = rangeset_new(pdev->domain, NULL,
+ RANGESETF_no_print);
+
+ if ( !task->bars[i].mem )
+ {
+ destroy_map_task(task);
+ return NULL;
+ }
+
+ task->bars[i].addr = pdev->vpci->header.bars[i].addr;
+ task->bars[i].guest_addr = pdev->vpci->header.bars[i].guest_addr;
+ }
+
+ task->map = map;
+
+ return task;
+}
+
+static int process_map_task(struct map_task *task)
{
- struct vpci_header *header = NULL;
+ const struct pci_dev *pdev = task->pdev;
unsigned int i;
ASSERT(rw_is_locked(&pdev->domain->pci_lock));
- header = &pdev->vpci->header;
- for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
{
- struct vpci_bar *bar = &header->bars[i];
+ struct bar_map *bar = &task->bars[i];
struct rangeset *mem = bar->mem;
struct map_data data = {
.d = pdev->domain,
- .map = map,
+ .map = task->map,
.bar = bar,
};
int rc;
@@ -146,29 +220,47 @@ static int process_map_task(const struct pci_dev *pdev, bool map)
bool vpci_process_pending(struct vcpu *v)
{
- const struct pci_dev *pdev = v->vpci.pdev;
- int rc;
-
- if ( !pdev )
- return false;
+ struct map_task *task;
+ int rc = 0;
read_lock(&v->domain->pci_lock);
- if ( !pdev->vpci || (v->domain != pdev->domain) )
+ while ( (task = list_first_entry_or_null(&v->vpci.task_queue,
+ struct map_task, next)) != NULL )
{
- v->vpci.pdev = NULL;
- read_unlock(&v->domain->pci_lock);
- return false;
- }
+ const struct pci_dev *pdev = task->pdev;
- rc = process_map_task(pdev, v->vpci.cmd & PCI_COMMAND_MEMORY);
- if ( rc == -ERESTART )
- {
- read_unlock(&v->domain->pci_lock);
- return true;
- }
+ if ( !pdev )
+ {
+ ASSERT_UNREACHABLE();
+ vpci_clear_map_queue(v, NULL);
+ rc = -EILSEQ;
+ break;
+ }
- v->vpci.pdev = NULL;
+ if ( !pdev->vpci || (v->domain != pdev->domain) )
+ {
+ list_del(&task->next);
+ destroy_map_task(task);
+ continue;
+ }
+
+ rc = process_map_task(task);
+ if ( rc == -ERESTART )
+ {
+ read_unlock(&v->domain->pci_lock);
+ return true;
+ }
+
+ list_del(&task->next);
+ destroy_map_task(task);
+
+ if ( rc )
+ {
+ vpci_clear_map_queue(v, pdev);
+ break;
+ }
+ }
read_unlock(&v->domain->pci_lock);
@@ -178,18 +270,17 @@ bool vpci_process_pending(struct vcpu *v)
return false;
}
-static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
- uint16_t cmd)
+static int __init apply_map(struct map_task *task)
{
- struct vpci_header *header = &pdev->vpci->header;
+ struct domain *d = task->pdev->domain;
int rc = 0;
unsigned int i;
ASSERT(rw_is_write_locked(&d->pci_lock));
- for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
{
- struct vpci_bar *bar = &header->bars[i];
+ struct bar_map *bar = &task->bars[i];
struct rangeset *mem = bar->mem;
struct map_data data = { .d = d, .map = true, .bar = bar };
@@ -213,7 +304,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
return rc;
}
-static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static void defer_map(struct map_task *task)
{
struct vcpu *curr = current;
@@ -223,9 +314,8 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
* is mapped. This can lead to parallel mapping operations being
* started for the same device if the domain is not well-behaved.
*/
- curr->vpci.pdev = pdev;
- curr->vpci.cmd = cmd;
- curr->vpci.rom_only = rom_only;
+ list_add_tail(&task->next, &curr->vpci.task_queue);
+
/*
* Raise a scheduler softirq in order to prevent the guest from resuming
* execution with pending mapping operations, to trigger the invocation
@@ -240,12 +330,17 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
struct pci_dev *tmp;
const struct domain *d;
const struct vpci_msix *msix = pdev->vpci->msix;
+ struct map_task *task;
bool bar_valid[ARRAY_SIZE(header->bars)] = { false };
unsigned int i, j;
int rc;
ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
+ task = alloc_map_task(pdev, cmd & PCI_COMMAND_MEMORY, rom_only);
+ if ( !task )
+ return -ENOMEM;
+
/*
* Create a rangeset per BAR that represents the current device memory
* region and compare it against all the currently active BAR memory
@@ -261,7 +356,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
struct vpci_bar *bar = &header->bars[i];
- struct rangeset *mem = bar->mem;
+ struct rangeset *mem = task->bars[i].mem;
unsigned long start = PFN_DOWN(bar->addr);
unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
unsigned long start_guest = PFN_DOWN(bar->guest_addr);
@@ -299,7 +394,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
gprintk(XENLOG_G_WARNING,
"%pp: can't map BAR%u - offset mismatch: %#lx vs %#lx\n",
&pdev->sbdf, i, bar->guest_addr, bar->addr);
- return -EINVAL;
+ rc = -EINVAL;
+ goto fail;
}
if ( !iomem_access_permitted(pdev->domain, start, end) )
@@ -309,7 +405,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
pdev->domain, start, end);
if ( !is_hardware_domain(pdev->domain) )
domain_crash(pdev->domain);
- return -EPERM;
+ rc = -EPERM;
+ goto fail;
}
rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
@@ -321,7 +418,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
pdev->domain, start, end, rc);
if ( !is_hardware_domain(pdev->domain) )
domain_crash(pdev->domain);
- return rc;
+ goto fail;
}
rc = rangeset_add_range(mem, start_guest, end_guest);
@@ -329,13 +426,13 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
{
printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",
start_guest, end_guest, rc);
- return rc;
+ goto fail;
}
/* Check for overlap with the already setup BAR ranges. */
for ( j = 0; j < i; j++ )
{
- struct rangeset *prev_mem = header->bars[j].mem;
+ struct rangeset *prev_mem = task->bars[j].mem;
if ( rangeset_is_empty(prev_mem) )
continue;
@@ -346,7 +443,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
gprintk(XENLOG_WARNING,
"%pp: failed to remove overlapping range [%lx, %lx]: %d\n",
&pdev->sbdf, start_guest, end_guest, rc);
- return rc;
+ goto fail;
}
}
@@ -356,7 +453,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
gprintk(XENLOG_WARNING,
"%pp: failed to sanitize BAR#%u memory: %d\n",
&pdev->sbdf, i, rc);
- return rc;
+ goto fail;
}
}
@@ -367,9 +464,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
vmsix_table_size(pdev->vpci, i) - 1);
- for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+ for ( j = 0; j < ARRAY_SIZE(task->bars); j++ )
{
- struct rangeset *mem = header->bars[j].mem;
+ struct rangeset *mem = task->bars[j].mem;
if ( rangeset_is_empty(mem) )
continue;
@@ -380,7 +477,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
gprintk(XENLOG_WARNING,
"%pp: failed to remove MSIX table [%lx, %lx]: %d\n",
&pdev->sbdf, start, end, rc);
- return rc;
+ goto fail;
}
}
}
@@ -424,7 +521,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( j = 0; j < ARRAY_SIZE(header->bars); j++)
{
const struct vpci_bar *bar = &header->bars[j];
- struct rangeset *mem = bar->mem;
+ struct rangeset *mem = task->bars[j].mem;
if ( !rangeset_overlaps_range(mem, start, end) ||
/*
@@ -443,7 +540,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
gprintk(XENLOG_WARNING,
"%pp: failed to remove [%lx, %lx]: %d\n",
&pdev->sbdf, start, end, rc);
- return rc;
+ goto fail;
}
}
}
@@ -472,12 +569,19 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
* will always be to establish mappings and process all the BARs.
*/
ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
- return apply_map(pdev->domain, pdev, cmd);
+ rc = apply_map(task);
+ destroy_map_task(task);
+ return rc;
}
- defer_map(pdev, cmd, rom_only);
+ defer_map(task);
return 0;
+
+ fail:
+ destroy_map_task(task);
+
+ return rc;
}
static void cf_check cmd_write(
@@ -705,18 +809,6 @@ static void cf_check rom_write(
}
}
-static int bar_add_rangeset(const struct pci_dev *pdev, struct vpci_bar *bar,
- unsigned int i)
-{
- char str[32];
-
- snprintf(str, sizeof(str), "%pp:BAR%u", &pdev->sbdf, i);
-
- bar->mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);
-
- return !bar->mem ? -ENOMEM : 0;
-}
-
int vpci_init_header(struct pci_dev *pdev)
{
uint16_t cmd;
@@ -826,10 +918,6 @@ int vpci_init_header(struct pci_dev *pdev)
else
bars[i].type = VPCI_BAR_MEM32;
- rc = bar_add_rangeset(pdev, &bars[i], i);
- if ( rc )
- goto fail;
-
rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
(i == num_bars - 1) ? PCI_BAR_LAST : 0);
if ( rc < 0 )
@@ -882,12 +970,6 @@ int vpci_init_header(struct pci_dev *pdev)
4, rom);
if ( rc )
rom->type = VPCI_BAR_EMPTY;
- else
- {
- rc = bar_add_rangeset(pdev, rom, num_bars);
- if ( rc )
- goto fail;
- }
}
else if ( !is_hwdom )
{
diff --git a/xen/drivers/vpci/private.h b/xen/drivers/vpci/private.h
index 2907f6b40fce..52f26394745b 100644
--- a/xen/drivers/vpci/private.h
+++ b/xen/drivers/vpci/private.h
@@ -44,6 +44,7 @@ typedef struct {
#define REGISTER_VPCI_EXTCAP(name, finit, fclean) \
REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##name, name, finit, fclean, true)
+void vpci_clear_map_queue(struct vcpu *v, const struct pci_dev *pdev);
int __must_check vpci_init_header(struct pci_dev *pdev);
int vpci_init_capabilities(struct pci_dev *pdev, bool ext_only);
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 0ac9ec8b0475..226c7870dd6c 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -24,6 +24,17 @@
#ifdef __XEN__
+void vpci_vcpu_destroy(struct vcpu *v)
+{
+ if ( v->vpci.task_queue.next )
+ vpci_clear_map_queue(v, NULL);
+}
+
+void vpci_vcpu_init(struct vcpu *v)
+{
+ INIT_LIST_HEAD(&v->vpci.task_queue);
+}
+
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
static int assign_virtual_sbdf(struct pci_dev *pdev)
{
@@ -89,7 +100,7 @@ struct vpci_register *vpci_get_register(const struct vpci *vpci,
void vpci_deassign_device(struct pci_dev *pdev)
{
- unsigned int i;
+ struct vcpu *v;
ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
@@ -116,8 +127,8 @@ void vpci_deassign_device(struct pci_dev *pdev)
}
spin_unlock(&pdev->vpci->lock);
- for ( i = 0; i < ARRAY_SIZE(pdev->vpci->header.bars); i++ )
- rangeset_destroy(pdev->vpci->header.bars[i].mem);
+ for_each_vcpu ( pdev->domain, v )
+ vpci_clear_map_queue(v, pdev);
xfree(pdev->vpci);
pdev->vpci = NULL;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index ab94eb60d65f..6bb09afd4eb0 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -19,6 +19,9 @@
*/
#define VPCI_MAX_VIRT_DEV (PCI_SLOT(~0) + 1)
+void vpci_vcpu_destroy(struct vcpu *v);
+void vpci_vcpu_init(struct vcpu *v);
+
/* Assign vPCI to device by adding handlers. */
int __must_check vpci_assign_device(struct pci_dev *pdev);
@@ -54,7 +57,6 @@ struct vpci {
uint64_t guest_addr;
uint64_t size;
uint64_t resizable_sizes;
- struct rangeset *mem;
enum {
VPCI_BAR_EMPTY,
VPCI_BAR_IO,
@@ -153,10 +155,8 @@ struct vpci {
};
struct vpci_vcpu {
- /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
- const struct pci_dev *pdev;
- uint16_t cmd;
- bool rom_only : 1;
+ /* List head with struct map_task elements */
+ struct list_head task_queue;
};
#ifdef __XEN__
@@ -204,6 +204,9 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
#else /* !CONFIG_HAS_VPCI */
struct vpci_vcpu {};
+static inline void vpci_vcpu_destroy(struct vcpu *v) { }
+static inline void vpci_vcpu_init(struct vcpu *v) { }
+
static inline int vpci_reinit_ext_capabilities(struct pci_dev *pdev)
{
return 0;
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v5 9/9] vpci: don't pass command value to modify_bars()
2026-07-08 21:02 [PATCH v5 0/9] vpci: allow queueing of mapping operations Stewart Hildebrand
` (7 preceding siblings ...)
2026-07-08 21:02 ` [PATCH v5 8/9] vpci: allow queueing of mapping operations Stewart Hildebrand
@ 2026-07-08 21:02 ` Stewart Hildebrand
8 siblings, 0 replies; 17+ messages in thread
From: Stewart Hildebrand @ 2026-07-08 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: Stewart Hildebrand, Roger Pau Monné, Daniel P. Smith
Now that writing the command register is no longer deferred, there's no
need to pass the cmd value to modify_bars(). Replace it with a bool to
indicate map/unmap.
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* adapted from: ("vpci: allow BAR map/unmap without affecting memory
decoding bit")
* significant rework
v3->v4:
* rebase on dynamically allocated map queue
v2->v3:
* use bool
* switch to task->map in more places
v1->v2:
* new patch
---
xen/drivers/vpci/header.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 99ce67053068..187de3a12abb 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -324,7 +324,7 @@ static void defer_map(struct map_task *task)
raise_softirq(SCHEDULE_SOFTIRQ);
}
-static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
{
struct vpci_header *header = &pdev->vpci->header;
struct pci_dev *tmp;
@@ -337,7 +337,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
- task = alloc_map_task(pdev, cmd & PCI_COMMAND_MEMORY, rom_only);
+ task = alloc_map_task(pdev, map, rom_only);
if ( !task )
return -ENOMEM;
@@ -369,7 +369,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
(rom_only ? bar->type != VPCI_BAR_ROM
: (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
/* Skip BARs already in the requested state. */
- bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
+ bar->enabled == map )
continue;
if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
@@ -409,8 +409,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
goto fail;
}
- rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
- !!(cmd & PCI_COMMAND_MEMORY));
+ rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end, map);
if ( rc )
{
printk(XENLOG_G_WARNING
@@ -555,7 +554,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
if ( bar_valid[i] )
- header->bars[i].enabled = cmd & PCI_COMMAND_MEMORY;
+ header->bars[i].enabled = map;
}
if ( system_state < SYS_STATE_active )
@@ -568,7 +567,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, 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((cmd & PCI_COMMAND_MEMORY) && !rom_only);
+ ASSERT(map && !rom_only);
rc = apply_map(task);
destroy_map_task(task);
return rc;
@@ -607,7 +606,7 @@ static void cf_check cmd_write(
* handled above and by the rsvdp_mask.
*/
if ( header->bars_mapped != new_enabled &&
- modify_bars(pdev, cmd, false) )
+ modify_bars(pdev, new_enabled, 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
@@ -784,12 +783,8 @@ static void cf_check rom_write(
rom->guest_addr = rom->addr;
}
- /*
- * 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.
- */
if ( header->bars_mapped && rom->enabled != new_enabled &&
- modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
+ modify_bars(pdev, new_enabled, 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
@@ -988,7 +983,7 @@ int vpci_init_header(struct pci_dev *pdev)
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
}
- return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
+ return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
fail:
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
--
2.54.0
^ permalink raw reply related [flat|nested] 17+ messages in thread