Hi On Mon, Oct 27, 2025 at 9:31 PM Peter Maydell <[email protected]> wrote: > > On Mon, 27 Oct 2025 at 16:54, Philippe Mathieu-Daudé <[email protected]> > wrote: > > > > In order to have more PCI host bridges to re-use the > > generic pci_host_data_le_ops MemoryRegionOps, add the > > 'config-reg-check-high-bit' property (%true by default). > > > > Signed-off-by: Philippe Mathieu-Daudé <[email protected]> > > > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > > index b5c624e12e8..d6db365e327 100644 > > --- a/hw/pci/pci_host.c > > +++ b/hw/pci/pci_host.c > > @@ -184,8 +184,10 @@ static void pci_host_data_write(void *opaque, hwaddr > > addr, > > { > > PCIHostState *s = opaque; > > > > - if (s->config_reg & (1u << 31)) > > - pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); > > + if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) { > > + return; > > + } > > + pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); > > } > > > > static uint64_t pci_host_data_read(void *opaque, > > @@ -193,7 +195,7 @@ static uint64_t pci_host_data_read(void *opaque, > > { > > PCIHostState *s = opaque; > > > > - if (!(s->config_reg & (1U << 31))) { > > + if (s->config_reg_check_high_bit && !(s->config_reg & (1U << 31))) { > > return 0xffffffff; > > } > > return pci_data_read(s->bus, s->config_reg | (addr & 3), len); > > @@ -235,6 +237,8 @@ const VMStateDescription vmstate_pcihost = { > > }; > > > > static const Property pci_host_properties_common[] = { > > + DEFINE_PROP_BOOL("config-reg-check-high-bit", PCIHostState, > > + config_reg_check_high_bit, true), > > I think it might be useful to name and document this > property at a slightly higher level of abstraction. > > Specifically, this code is handling the behaviour of > the CONFIG_ADDRESS register which is part of the PCI > Configuration Access Method (CAM). For x86 the top bit of > CONFIG_ADDRESS is an Enable bit, which must be set to > cause accesses to CONFIG_DATA to actually do something. > For PCI controllers like Dino there is no Enable bit > defined in CONFIG_ADDRESS[*] and CONFIG_DATA accesses always > take effect. > > [*] http://ftp.parisc-linux.org/docs/chips/dino_ers.pdf page 49 > > So perhaps we could call this "config-address-reg-has-enable-bit" ? > > A documentation comment about its purpose and noting that > the expectation is that this is set by the subclass, > not by end-users, might also be helpful.
If this shouldn't be tweaked by users and is specific to a device kind, could it be a bool on the PCIHostBridgeClass ? -- Marc-André Lureau
