On 8/14/25 12:03, Roger Pau Monne wrote: > 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. >
Nit: I think you want: Resolves: https://gitlab.com/xen-project/xen/-/issues/98 > Signed-off-by: Roger Pau Monné <roger....@citrix.com> > --- > xen/drivers/vpci/header.c | 42 +++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 1035dcca242d..1a501a0ba47e 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -163,8 +163,6 @@ static void modify_decoding(const struct pci_dev *pdev, > uint16_t cmd, > 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); rom_pos and val are unused drivers/vpci/header.c: In function ‘modify_decoding’: drivers/vpci/header.c:160:22: error: unused variable ‘val’ [-Werror=unused-variable] 160 | uint32_t val = bar->addr | | ^~~ drivers/vpci/header.c:158:26: error: unused variable ‘rom_pos’ [-Werror=unused-variable] 158 | unsigned int rom_pos = (i == PCI_HEADER_NORMAL_NR_BARS) | ^~~~~~~ Other than that the patch looks good to me > return; > } > > @@ -174,14 +172,6 @@ static void modify_decoding(const struct pci_dev *pdev, > uint16_t cmd, > _mfn(PFN_DOWN(bar->addr + bar->size - 1))) ) > bar->enabled = map; > } > - > - 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) > @@ -547,16 +537,23 @@ 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 != !!(cmd & PCI_COMMAND_MEMORY) && > + 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; > + > + /* > + * 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 = cmd & PCI_COMMAND_MEMORY; > + pci_conf_write16(pdev->sbdf, reg, cmd); > } > > static uint32_t cf_check guest_cmd_read( > @@ -712,17 +709,11 @@ 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 ( 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 > @@ -733,6 +724,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; > @@ -1057,6 +1050,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:
