[v2] hw/pci-host: Re-use generic pci_host_data_le_ops MemoryRegionOps

[PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

Posted by Philippe Mathieu-Daudé 2 weeks, 4 days ago

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é <philmd@linaro.org>
---
 include/hw/pci/pci_host.h |  1 +
 hw/pci/pci_host.c         | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 954dd446fa4..c04a567ec57 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -43,6 +43,7 @@ struct PCIHostState {
     MemoryRegion data_mem;
     MemoryRegion mmcfg;
     uint32_t config_reg;
+    bool config_reg_check_high_bit;
     bool mig_enabled;
     PCIBus *bus;
     bool bypass_iommu;
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),
     DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
                      mig_enabled, true),
     DEFINE_PROP_BOOL(PCI_HOST_BYPASS_IOMMU, PCIHostState, bypass_iommu, false),
-- 
2.51.0

Re: [PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

Posted by Peter Maydell 2 weeks, 4 days ago

On Mon, 27 Oct 2025 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> 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é <philmd@linaro.org>

> 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.

thanks
-- PMM

Re: [PATCH v2 1/7] hw/pci/pci_host: Add 'config-reg-check-high-bit' property

Posted by Marc-André Lureau 2 weeks, 3 days ago

Hi

On Mon, Oct 27, 2025 at 9:31 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 27 Oct 2025 at 16:54, Philippe Mathieu-Daudé <philmd@linaro.org> 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é <philmd@linaro.org>
>
> > 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