Re: [PATCH RFC RESEND v2 1/6] hw/pci/pci_host: Add iommu property for pci host

[Wang Xingang]

Hi Eric,

On 2021/4/13 1:36, Auger Eric wrote:
> Hi Wang,
>
> On 3/25/21 8:22 AM, Wang Xingang wrote:
> > From: Xingang Wang <wangxingang5@huawei.com>
> >
> > The pci host iommu property is useful to check whether
> > the iommu is enabled on the pci root bus.
> >
> > Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> > Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> > ---
> >   hw/pci/pci.c              | 18 +++++++++++++++++-
> >   hw/pci/pci_host.c         |  2 ++
> >   include/hw/pci/pci.h      |  1 +
> >   include/hw/pci/pci_host.h |  1 +
> >   4 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index ac9a24889c..e17aa9075f 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -417,6 +417,22 @@ const char *pci_root_bus_path(PCIDevice *dev)
> >       return rootbus->qbus.name;
> >   }
> >   
> > +bool pci_root_bus_has_iommu(PCIBus *bus)
> "has_iommu" is misleading as it does not mean an IOMMU is actually
> instantiated but rather that if there is any, it will translate
> transactions coming from this primary bus
>
> I would rather inverse the logic and have a
>
> "bypass_iommu" property defaulting to false
>
> and this function dubbed something like pci_root_bus_bypass_iommu
> > +{
> > +    PCIBus *rootbus = bus;
> > +    PCIHostState *host_bridge;
> > +
> > +    if (!pci_bus_is_root(bus)) {
> > +        rootbus = pci_device_root_bus(bus->parent_dev);
> > +    }
> > +
> > +    host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
> > +
> > +    assert(host_bridge->bus == rootbus);
> > +
> > +    return host_bridge->iommu;
> > +}
> > +

Thanks for your advice, it is misleading, i will replace this.

> >   static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
> >                                 MemoryRegion *address_space_mem,
> >                                 MemoryRegion *address_space_io,
> > @@ -2716,7 +2732,7 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice 
> > *dev)
> >   
> >           iommu_bus = parent_bus;
> >       }
> > -    if (iommu_bus && iommu_bus->iommu_fn) {
> > +    if (pci_root_bus_has_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) {
> >           return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
> >       }
> >       return &address_space_memory;
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 8ca5fadcbd..92ce213b18 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -222,6 +222,8 @@ const VMStateDescription vmstate_pcihost = {
> >   static Property pci_host_properties_common[] = {
> >       DEFINE_PROP_BOOL("x-config-reg-migration-enabled", PCIHostState,
> >                        mig_enabled, true),
> > +    DEFINE_PROP_BOOL("pci-host-iommu-enabled", PCIHostState,
> > +                     iommu, true),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 6be4e0c460..718b5a454a 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -480,6 +480,7 @@ void pci_for_each_bus(PCIBus *bus,
> >   
> >   PCIBus *pci_device_root_bus(const PCIDevice *d);
> >   const char *pci_root_bus_path(PCIDevice *dev);
> > +bool pci_root_bus_has_iommu(PCIBus *bus);
> >   PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> >   int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> >   void pci_bus_get_w64_range(PCIBus *bus, Range *range);
> > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> > index 52e038c019..64128e3a19 100644
> > --- a/include/hw/pci/pci_host.h
> > +++ b/include/hw/pci/pci_host.h
> > @@ -43,6 +43,7 @@ struct PCIHostState {
> >       uint32_t config_reg;
> >       bool mig_enabled;
> >       PCIBus *bus;
> > +    bool iommu;
> >   
> >       QLIST_ENTRY(PCIHostState) next;
> >   };
> Thanks
>
> Eric
>
> .

Thanks

Xingang

.