Currently, Xen vPCI refuses BAR writes if the BAR is mapped in p2m. If
firmware initializes a 32-bit BAR to a bad address, Linux may try to
write a new address to the 32-bit BAR without disabling memory decoding.
Since Xen refuses such writes, the BAR (and thus PCI device) will be
non-functional.
Allow the hardware domain to issue 32-bit BAR writes with memory
decoding enabled. This increases the compatibility of PVH dom0 with more
hardware.
Note that Linux aims at disabling memory decoding before writing 64-bit
BARs. Continue to refuse 64-bit BAR writes in Xen while those BARs are
mapped for now to avoid mapping half-updated BARs in p2m.
Resolves: https://gitlab.com/xen-project/xen/-/issues/197
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v3->v4:
* rebase on dynamically allocated map queue
v2->v3:
* minor tweaks for fixed number of map/unmap slots
v1->v2:
* rework on top of queued BAR map/unmap operation machinery
RFC->v1:
* keep memory decoding enabled in hardware
* allow write while memory decoding is enabled for 32-bit BARs only
* rework BAR mapping machinery to support unmap-then-map operation
---
xen/drivers/vpci/header.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 20fe380552f4..dc4f585b4e40 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -670,6 +670,7 @@ static void cf_check bar_write(
{
struct vpci_bar *bar = data;
bool hi = false;
+ uint16_t cmd = 0;
ASSERT(is_hardware_domain(pdev->domain));
@@ -683,19 +684,29 @@ static void cf_check bar_write(
val &= PCI_BASE_ADDRESS_MEM_MASK;
/*
- * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
- * writes as long as the BAR is not mapped into the p2m.
+ * Allow 64-bit BAR writes only when the BAR is not mapped in p2m. Always
+ * allow 32-bit BAR writes.
*/
if ( bar->enabled )
{
- /* If the value written is the current one avoid printing a warning. */
- if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
- gprintk(XENLOG_WARNING,
- "%pp: ignored BAR %zu write while mapped\n",
- &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
- return;
- }
+ if ( bar->type == VPCI_BAR_MEM32 )
+ {
+ if ( val == bar->addr )
+ return;
+ cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
+ modify_bars(pdev, cmd, false, false);
+ }
+ else
+ {
+ /* If the value written is the same avoid printing a warning. */
+ if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
+ gprintk(XENLOG_WARNING,
+ "%pp: ignored BAR %zu write while mapped\n",
+ &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
+ return;
+ }
+ }
/*
* Update the cached address, so that when memory decoding is enabled
@@ -715,6 +726,9 @@ static void cf_check bar_write(
}
pci_conf_write32(pdev->sbdf, reg, val);
+
+ if ( bar->enabled )
+ modify_bars(pdev, cmd, false, true);
}
static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,
--
2.53.0On Mon, Apr 06, 2026 at 03:11:58PM -0400, Stewart Hildebrand wrote:
> Currently, Xen vPCI refuses BAR writes if the BAR is mapped in p2m. If
> firmware initializes a 32-bit BAR to a bad address, Linux may try to
> write a new address to the 32-bit BAR without disabling memory decoding.
> Since Xen refuses such writes, the BAR (and thus PCI device) will be
> non-functional.
>
> Allow the hardware domain to issue 32-bit BAR writes with memory
> decoding enabled. This increases the compatibility of PVH dom0 with more
> hardware.
>
> Note that Linux aims at disabling memory decoding before writing 64-bit
> BARs. Continue to refuse 64-bit BAR writes in Xen while those BARs are
> mapped for now to avoid mapping half-updated BARs in p2m.
While Linux does the disabling for 64bit BARs, I wonder if at some we
will need to handle 64bit BAR writes anyway for other OSes, specially
with domU support.
> Resolves: https://gitlab.com/xen-project/xen/-/issues/197
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v3->v4:
> * rebase on dynamically allocated map queue
>
> v2->v3:
> * minor tweaks for fixed number of map/unmap slots
>
> v1->v2:
> * rework on top of queued BAR map/unmap operation machinery
>
> RFC->v1:
> * keep memory decoding enabled in hardware
> * allow write while memory decoding is enabled for 32-bit BARs only
> * rework BAR mapping machinery to support unmap-then-map operation
> ---
> xen/drivers/vpci/header.c | 32 +++++++++++++++++++++++---------
> 1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 20fe380552f4..dc4f585b4e40 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -670,6 +670,7 @@ static void cf_check bar_write(
> {
> struct vpci_bar *bar = data;
> bool hi = false;
> + uint16_t cmd = 0;
>
> ASSERT(is_hardware_domain(pdev->domain));
>
> @@ -683,19 +684,29 @@ static void cf_check bar_write(
> val &= PCI_BASE_ADDRESS_MEM_MASK;
>
> /*
> - * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
> - * writes as long as the BAR is not mapped into the p2m.
> + * Allow 64-bit BAR writes only when the BAR is not mapped in p2m. Always
> + * allow 32-bit BAR writes.
> */
> if ( bar->enabled )
> {
> - /* If the value written is the current one avoid printing a warning. */
> - if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> - gprintk(XENLOG_WARNING,
> - "%pp: ignored BAR %zu write while mapped\n",
> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> - return;
> - }
> + if ( bar->type == VPCI_BAR_MEM32 )
> + {
> + if ( val == bar->addr )
> + return;
>
> + cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> + modify_bars(pdev, cmd, false, false);
> + }
> + else
> + {
> + /* If the value written is the same avoid printing a warning. */
> + if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> + gprintk(XENLOG_WARNING,
> + "%pp: ignored BAR %zu write while mapped\n",
> + &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> + return;
> + }
> + }
>
> /*
> * Update the cached address, so that when memory decoding is enabled
> @@ -715,6 +726,9 @@ static void cf_check bar_write(
> }
>
> pci_conf_write32(pdev->sbdf, reg, val);
I don't think it matters a lot, but here we are changing the position
of the BAR in the host memory map while the mappings are still active.
> +
> + if ( bar->enabled )
> + modify_bars(pdev, cmd, false, true);
> }
>
> static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,
As it's in context here - we possibly want to do something similar
with guest writes now that we can?
While we might not have a user-case for domUs ATM, I think it's vest
if we also fix that use case at the same time that dom0 is fixed
(unless there's something that prevents it).
Thanks, Roger.
On 24.04.2026 10:50, Roger Pau Monné wrote:
> On Mon, Apr 06, 2026 at 03:11:58PM -0400, Stewart Hildebrand wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -670,6 +670,7 @@ static void cf_check bar_write(
>> {
>> struct vpci_bar *bar = data;
>> bool hi = false;
>> + uint16_t cmd = 0;
>>
>> ASSERT(is_hardware_domain(pdev->domain));
>>
>> @@ -683,19 +684,29 @@ static void cf_check bar_write(
>> val &= PCI_BASE_ADDRESS_MEM_MASK;
>>
>> /*
>> - * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
>> - * writes as long as the BAR is not mapped into the p2m.
>> + * Allow 64-bit BAR writes only when the BAR is not mapped in p2m. Always
>> + * allow 32-bit BAR writes.
>> */
>> if ( bar->enabled )
>> {
>> - /* If the value written is the current one avoid printing a warning. */
>> - if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>> - gprintk(XENLOG_WARNING,
>> - "%pp: ignored BAR %zu write while mapped\n",
>> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>> - return;
>> - }
>> + if ( bar->type == VPCI_BAR_MEM32 )
>> + {
>> + if ( val == bar->addr )
>> + return;
>>
>> + cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>> + modify_bars(pdev, cmd, false, false);
>> + }
>> + else
>> + {
>> + /* If the value written is the same avoid printing a warning. */
>> + if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>> + gprintk(XENLOG_WARNING,
>> + "%pp: ignored BAR %zu write while mapped\n",
>> + &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>> + return;
>> + }
>> + }
>>
>> /*
>> * Update the cached address, so that when memory decoding is enabled
>> @@ -715,6 +726,9 @@ static void cf_check bar_write(
>> }
>>
>> pci_conf_write32(pdev->sbdf, reg, val);
>
> I don't think it matters a lot, but here we are changing the position
> of the BAR in the host memory map while the mappings are still active.
It would matter if the original address space could be re-used for another
purpose while those mappings are still there?
Jan
On 06.04.2026 21:11, Stewart Hildebrand wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -670,6 +670,7 @@ static void cf_check bar_write(
> {
> struct vpci_bar *bar = data;
> bool hi = false;
> + uint16_t cmd = 0;
>
> ASSERT(is_hardware_domain(pdev->domain));
>
> @@ -683,19 +684,29 @@ static void cf_check bar_write(
> val &= PCI_BASE_ADDRESS_MEM_MASK;
>
> /*
> - * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
> - * writes as long as the BAR is not mapped into the p2m.
> + * Allow 64-bit BAR writes only when the BAR is not mapped in p2m. Always
> + * allow 32-bit BAR writes.
> */
> if ( bar->enabled )
> {
> - /* If the value written is the current one avoid printing a warning. */
> - if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> - gprintk(XENLOG_WARNING,
> - "%pp: ignored BAR %zu write while mapped\n",
> - &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> - return;
> - }
> + if ( bar->type == VPCI_BAR_MEM32 )
> + {
> + if ( val == bar->addr )
> + return;
>
> + cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> + modify_bars(pdev, cmd, false, false);
> + }
> + else
> + {
> + /* If the value written is the same avoid printing a warning. */
> + if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> + gprintk(XENLOG_WARNING,
> + "%pp: ignored BAR %zu write while mapped\n",
> + &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> + return;
> + }
> + }
>
> /*
> * Update the cached address, so that when memory decoding is enabled
> @@ -715,6 +726,9 @@ static void cf_check bar_write(
> }
>
> pci_conf_write32(pdev->sbdf, reg, val);
> +
> + if ( bar->enabled )
> + modify_bars(pdev, cmd, false, true);
> }
>
> static void cf_check guest_mem_bar_write(const struct pci_dev *pdev,
While this all looks plausible, isn't something similar needed in rom_write()
then as well?
Jan
© 2016 - 2026 Red Hat, Inc.