From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 77923C43331 for ; Thu, 2 Apr 2020 12:41:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 345BD20757 for ; Thu, 2 Apr 2020 12:41:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="OTgi+lBK" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388229AbgDBMll (ORCPT ); Thu, 2 Apr 2020 08:41:41 -0400 Received: from us-smtp-1.mimecast.com ([205.139.110.61]:36633 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2388210AbgDBMll (ORCPT ); Thu, 2 Apr 2020 08:41:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585831299; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FPDHpRUs5ebERKZHb1N/xx88smeNoPwiC2fNKSzO0Gc=; b=OTgi+lBKY5mWKIMe7LSJahvSHwfR7cXkr5BNZN4eq+keMaoRWJ+0NVC8OYy4r2d0AgJEt2 Ua50UjyrSikh8905exlAh/J8qJhW3J2G+vnvGeYpFnyiDSzK2vtQxfilfMZy4NKZGtSxuQ zqtfxntX4k6y6hPM0sLDfTsw0O9lxKo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-222-y2Dx8px_MiuptR2RnVzcXQ-1; Thu, 02 Apr 2020 08:41:36 -0400 X-MC-Unique: y2Dx8px_MiuptR2RnVzcXQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1037F8017F3; Thu, 2 Apr 2020 12:41:34 +0000 (UTC) Received: from [10.36.112.58] (ovpn-112-58.ams2.redhat.com [10.36.112.58]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D4AB238D; Thu, 2 Apr 2020 12:41:24 +0000 (UTC) Subject: Re: [PATCH v2 05/22] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps To: "Liu, Yi L" , "qemu-devel@nongnu.org" , "alex.williamson@redhat.com" , "peterx@redhat.com" Cc: "jean-philippe@linaro.org" , "Tian, Kevin" , Jacob Pan , Yi Sun , "kvm@vger.kernel.org" , "mst@redhat.com" , "Tian, Jun J" , "Sun, Yi Y" , "pbonzini@redhat.com" , "Wu, Hao" , "david@gibson.dropbear.id.au" References: <1585542301-84087-1-git-send-email-yi.l.liu@intel.com> <1585542301-84087-6-git-send-email-yi.l.liu@intel.com> From: Auger Eric Message-ID: <532069f1-123e-1ba0-cbd0-c849c2e0c5aa@redhat.com> Date: Thu, 2 Apr 2020 14:41:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Yi, On 4/2/20 10:52 AM, Liu, Yi L wrote: >> From: Auger Eric < eric.auger@redhat.com> >> Sent: Monday, March 30, 2020 7:02 PM >> To: Liu, Yi L ; qemu-devel@nongnu.org; >> Subject: Re: [PATCH v2 05/22] hw/pci: modify pci_setup_iommu() to set >> PCIIOMMUOps >> >> >> >> On 3/30/20 6:24 AM, Liu Yi L wrote: >>> This patch modifies pci_setup_iommu() to set PCIIOMMUOps instead of >>> setting PCIIOMMUFunc. PCIIOMMUFunc is used to get an address space for >>> a PCI device in vendor specific way. The PCIIOMMUOps still offers this >>> functionality. But using PCIIOMMUOps leaves space to add more iommu >>> related vendor specific operations. >>> >>> Cc: Kevin Tian >>> Cc: Jacob Pan >>> Cc: Peter Xu >>> Cc: Eric Auger >>> Cc: Yi Sun >>> Cc: David Gibson >>> Cc: Michael S. Tsirkin >>> Reviewed-by: David Gibson >>> Reviewed-by: Peter Xu >>> Signed-off-by: Liu Yi L >>> --- >>> hw/alpha/typhoon.c | 6 +++++- >>> hw/arm/smmu-common.c | 6 +++++- >>> hw/hppa/dino.c | 6 +++++- >>> hw/i386/amd_iommu.c | 6 +++++- >>> hw/i386/intel_iommu.c | 6 +++++- >>> hw/pci-host/designware.c | 6 +++++- >>> hw/pci-host/pnv_phb3.c | 6 +++++- >>> hw/pci-host/pnv_phb4.c | 6 +++++- >>> hw/pci-host/ppce500.c | 6 +++++- >>> hw/pci-host/prep.c | 6 +++++- >>> hw/pci-host/sabre.c | 6 +++++- >>> hw/pci/pci.c | 12 +++++++----- >>> hw/ppc/ppc440_pcix.c | 6 +++++- >>> hw/ppc/spapr_pci.c | 6 +++++- >>> hw/s390x/s390-pci-bus.c | 8 ++++++-- hw/virtio/virtio-iommu.c | 6 >>> +++++- >>> include/hw/pci/pci.h | 8 ++++++-- >>> include/hw/pci/pci_bus.h | 2 +- >>> 18 files changed, 90 insertions(+), 24 deletions(-) >>> >>> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c index >>> 1795e2f..f271de1 100644 >>> --- a/hw/alpha/typhoon.c >>> +++ b/hw/alpha/typhoon.c >>> @@ -740,6 +740,10 @@ static AddressSpace *typhoon_pci_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &s->pchip.iommu_as; >>> } >>> >>> +static const PCIIOMMUOps typhoon_iommu_ops = { >>> + .get_address_space = typhoon_pci_dma_iommu, }; >>> + >>> static void typhoon_set_irq(void *opaque, int irq, int level) { >>> TyphoonState *s = opaque; >>> @@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, ISABus >> **isa_bus, qemu_irq *p_rtc_irq, >>> "iommu-typhoon", UINT64_MAX); >>> address_space_init(&s->pchip.iommu_as, MEMORY_REGION(&s- >>> pchip.iommu), >>> "pchip0-pci"); >>> - pci_setup_iommu(b, typhoon_pci_dma_iommu, s); >>> + pci_setup_iommu(b, &typhoon_iommu_ops, s); >>> >>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */ >>> memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), >>> &alpha_pci_iack_ops, diff --git a/hw/arm/smmu-common.c >>> b/hw/arm/smmu-common.c index e13a5f4..447146e 100644 >>> --- a/hw/arm/smmu-common.c >>> +++ b/hw/arm/smmu-common.c >>> @@ -343,6 +343,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, >> void *opaque, int devfn) >>> return &sdev->as; >>> } >>> >>> +static const PCIIOMMUOps smmu_ops = { >>> + .get_address_space = smmu_find_add_as, }; >>> + >>> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) { >>> uint8_t bus_n, devfn; >>> @@ -437,7 +441,7 @@ static void smmu_base_realize(DeviceState *dev, Error >> **errp) >>> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL); >>> >>> if (s->primary_bus) { >>> - pci_setup_iommu(s->primary_bus, smmu_find_add_as, s); >>> + pci_setup_iommu(s->primary_bus, &smmu_ops, s); >>> } else { >>> error_setg(errp, "SMMU is not attached to any PCI bus!"); >>> } >>> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c index 2b1b38c..3da4f84 >>> 100644 >>> --- a/hw/hppa/dino.c >>> +++ b/hw/hppa/dino.c >>> @@ -459,6 +459,10 @@ static AddressSpace *dino_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps dino_iommu_ops = { >>> + .get_address_space = dino_pcihost_set_iommu, }; >>> + >>> /* >>> * Dino interrupts are connected as shown on Page 78, Table 23 >>> * (Little-endian bit numbers) >>> @@ -580,7 +584,7 @@ PCIBus *dino_init(MemoryRegion *addr_space, >>> memory_region_add_subregion(&s->bm, 0xfff00000, >>> &s->bm_cpu_alias); >>> address_space_init(&s->bm_as, &s->bm, "pci-bm"); >>> - pci_setup_iommu(b, dino_pcihost_set_iommu, s); >>> + pci_setup_iommu(b, &dino_iommu_ops, s); >>> >>> *p_rtc_irq = qemu_allocate_irq(dino_set_timer_irq, s, 0); >>> *p_ser_irq = qemu_allocate_irq(dino_set_serial_irq, s, 0); diff >>> --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index >>> b1175e5..5fec30e 100644 >>> --- a/hw/i386/amd_iommu.c >>> +++ b/hw/i386/amd_iommu.c >>> @@ -1451,6 +1451,10 @@ static AddressSpace >> *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >>> return &iommu_as[devfn]->as; >>> } >>> >>> +static const PCIIOMMUOps amdvi_iommu_ops = { >>> + .get_address_space = amdvi_host_dma_iommu, }; >>> + >>> static const MemoryRegionOps mmio_mem_ops = { >>> .read = amdvi_mmio_read, >>> .write = amdvi_mmio_write, >>> @@ -1577,7 +1581,7 @@ static void amdvi_realize(DeviceState *dev, >>> Error **errp) >>> >>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio); >>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR); >>> - pci_setup_iommu(bus, amdvi_host_dma_iommu, s); >>> + pci_setup_iommu(bus, &amdvi_iommu_ops, s); >>> s->devid = object_property_get_int(OBJECT(&s->pci), "addr", errp); >>> msi_init(&s->pci.dev, 0, 1, true, false, errp); >>> amdvi_init(s); >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index >>> df7ad25..4b22910 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3729,6 +3729,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &vtd_as->as; >>> } >>> >>> +static PCIIOMMUOps vtd_iommu_ops = { >> static const > > got it. > >>> + .get_address_space = vtd_host_dma_iommu, }; >>> + >>> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) { >>> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); @@ -3840,7 >>> +3844,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) >>> g_free, g_free); >>> vtd_init(s); >>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, >> Q35_HOST_BRIDGE_IOMMU_ADDR); >>> - pci_setup_iommu(bus, vtd_host_dma_iommu, dev); >>> + pci_setup_iommu(bus, &vtd_iommu_ops, dev); >>> /* Pseudo address space under root PCI bus. */ >>> x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, >> Q35_PSEUDO_DEVFN_IOAPIC); >>> qemu_add_machine_init_done_notifier(&vtd_machine_done_notify); >>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c index >>> dd24551..4c6338a 100644 >>> --- a/hw/pci-host/designware.c >>> +++ b/hw/pci-host/designware.c >>> @@ -645,6 +645,10 @@ static AddressSpace >> *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque, >>> return &s->pci.address_space; >>> } >>> >>> +static const PCIIOMMUOps designware_iommu_ops = { >>> + .get_address_space = designware_pcie_host_set_iommu, }; >>> + >>> static void designware_pcie_host_realize(DeviceState *dev, Error >>> **errp) { >>> PCIHostState *pci = PCI_HOST_BRIDGE(dev); @@ -686,7 +690,7 @@ >>> static void designware_pcie_host_realize(DeviceState *dev, Error **errp) >>> address_space_init(&s->pci.address_space, >>> &s->pci.address_space_root, >>> "pcie-bus-address-space"); >>> - pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s); >>> + pci_setup_iommu(pci->bus, &designware_iommu_ops, s); >>> >>> qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus)); >>> qdev_init_nofail(DEVICE(&s->root)); >>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index >>> 74618fa..ecfe627 100644 >>> --- a/hw/pci-host/pnv_phb3.c >>> +++ b/hw/pci-host/pnv_phb3.c >>> @@ -961,6 +961,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &ds->dma_as; >>> } >>> >>> +static PCIIOMMUOps pnv_phb3_iommu_ops = { >> static const > got it. :-) > >>> + .get_address_space = pnv_phb3_dma_iommu, }; >>> + >>> static void pnv_phb3_instance_init(Object *obj) { >>> PnvPHB3 *phb = PNV_PHB3(obj); >>> @@ -1059,7 +1063,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error >> **errp) >>> &phb->pci_mmio, &phb->pci_io, >>> 0, 4, TYPE_PNV_PHB3_ROOT_BUS); >>> >>> - pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb); >>> + pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb); >>> >>> /* Add a single Root port */ >>> qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id); >>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index >>> 23cf093..04e95e3 100644 >>> --- a/hw/pci-host/pnv_phb4.c >>> +++ b/hw/pci-host/pnv_phb4.c >>> @@ -1148,6 +1148,10 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &ds->dma_as; >>> } >>> >>> +static PCIIOMMUOps pnv_phb4_iommu_ops = { >> idem > will add const. > >>> + .get_address_space = pnv_phb4_dma_iommu, }; >>> + >>> static void pnv_phb4_instance_init(Object *obj) { >>> PnvPHB4 *phb = PNV_PHB4(obj); >>> @@ -1205,7 +1209,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error >> **errp) >>> pnv_phb4_set_irq, pnv_phb4_map_irq, phb, >>> &phb->pci_mmio, &phb->pci_io, >>> 0, 4, TYPE_PNV_PHB4_ROOT_BUS); >>> - pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb); >>> + pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb); >>> >>> /* Add a single Root port */ >>> qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id); >>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index >>> d710727..5baf5db 100644 >>> --- a/hw/pci-host/ppce500.c >>> +++ b/hw/pci-host/ppce500.c >>> @@ -439,6 +439,10 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps ppce500_iommu_ops = { >>> + .get_address_space = e500_pcihost_set_iommu, }; >>> + >>> static void e500_pcihost_realize(DeviceState *dev, Error **errp) { >>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); @@ -473,7 +477,7 @@ >>> static void e500_pcihost_realize(DeviceState *dev, Error **errp) >>> memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX); >>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem); >>> address_space_init(&s->bm_as, &s->bm, "pci-bm"); >>> - pci_setup_iommu(b, e500_pcihost_set_iommu, s); >>> + pci_setup_iommu(b, &ppce500_iommu_ops, s); >>> >>> pci_create_simple(b, 0, "e500-host-bridge"); >>> >>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c index >>> 1a02e9a..7c57311 100644 >>> --- a/hw/pci-host/prep.c >>> +++ b/hw/pci-host/prep.c >>> @@ -213,6 +213,10 @@ static AddressSpace *raven_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps raven_iommu_ops = { >>> + .get_address_space = raven_pcihost_set_iommu, }; >>> + >>> static void raven_change_gpio(void *opaque, int n, int level) { >>> PREPPCIState *s = opaque; >>> @@ -303,7 +307,7 @@ static void raven_pcihost_initfn(Object *obj) >>> memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias); >>> memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias); >>> address_space_init(&s->bm_as, &s->bm, "raven-bm"); >>> - pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s); >>> + pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s); >>> >>> h->bus = &s->pci_bus; >>> >>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c index >>> 2b8503b..251549b 100644 >>> --- a/hw/pci-host/sabre.c >>> +++ b/hw/pci-host/sabre.c >>> @@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &is->iommu_as; >>> } >>> >>> +static const PCIIOMMUOps sabre_iommu_ops = { >>> + .get_address_space = sabre_pci_dma_iommu, }; >>> + >>> static void sabre_config_write(void *opaque, hwaddr addr, >>> uint64_t val, unsigned size) { @@ >>> -402,7 +406,7 @@ static void sabre_realize(DeviceState *dev, Error **errp) >>> /* IOMMU */ >>> memory_region_add_subregion_overlap(&s->sabre_config, 0x200, >>> sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1); >>> - pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu); >>> + pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu); >>> >>> /* APB secondary busses */ >>> pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), >>> true, diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e1ed667..aa9025c >>> 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -2644,7 +2644,7 @@ AddressSpace >> *pci_device_iommu_address_space(PCIDevice *dev) >>> PCIBus *iommu_bus = bus; >>> uint8_t devfn = dev->devfn; >>> >>> - while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) >> { >>> + while (iommu_bus && !iommu_bus->iommu_ops && >>> + iommu_bus->parent_dev) { >> Depending on future usage, this is not strictly identical to the original >> code. You exit >> the loop as soon as a iommu_bus->iommu_ops is set whatever the presence of >> get_address_space(). > > To be identical with original code, may adding the get_address_space() > presence check. Then the loop exits when the iommu_bus->iommu_ops is > set and meanwhile iommu_bus->iommu_ops->get_address_space() is set. > But is it possible that there is an intermediate iommu_bus which has > iommu_ops set but the get_address_space() is clear. I guess not as > iommu_ops is set by vIOMMU and vIOMMU won't differentiate buses? I don't know. That depends on how the ops are going to be used in the future. Can't you enforce the fact that get_address_space() is a mandatory ops? Thanks Eric > > Also the get_address_space() presence will be checked when trying to > use it. right? > > Regards, > Yi Liu > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2383C43331 for ; Thu, 2 Apr 2020 12:42:58 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 86E3C20757 for ; Thu, 2 Apr 2020 12:42:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="OTgi+lBK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 86E3C20757 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:38022 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jJzBR-00051E-Lq for qemu-devel@archiver.kernel.org; Thu, 02 Apr 2020 08:42:57 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33041) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jJzAE-00041J-Ie for qemu-devel@nongnu.org; Thu, 02 Apr 2020 08:41:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jJzAC-0007Ic-3u for qemu-devel@nongnu.org; Thu, 02 Apr 2020 08:41:42 -0400 Received: from us-smtp-2.mimecast.com ([205.139.110.61]:34752 helo=us-smtp-delivery-1.mimecast.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1jJzAB-0007IK-VA for qemu-devel@nongnu.org; Thu, 02 Apr 2020 08:41:40 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585831299; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FPDHpRUs5ebERKZHb1N/xx88smeNoPwiC2fNKSzO0Gc=; b=OTgi+lBKY5mWKIMe7LSJahvSHwfR7cXkr5BNZN4eq+keMaoRWJ+0NVC8OYy4r2d0AgJEt2 Ua50UjyrSikh8905exlAh/J8qJhW3J2G+vnvGeYpFnyiDSzK2vtQxfilfMZy4NKZGtSxuQ zqtfxntX4k6y6hPM0sLDfTsw0O9lxKo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-222-y2Dx8px_MiuptR2RnVzcXQ-1; Thu, 02 Apr 2020 08:41:36 -0400 X-MC-Unique: y2Dx8px_MiuptR2RnVzcXQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1037F8017F3; Thu, 2 Apr 2020 12:41:34 +0000 (UTC) Received: from [10.36.112.58] (ovpn-112-58.ams2.redhat.com [10.36.112.58]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D4AB238D; Thu, 2 Apr 2020 12:41:24 +0000 (UTC) Subject: Re: [PATCH v2 05/22] hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps To: "Liu, Yi L" , "qemu-devel@nongnu.org" , "alex.williamson@redhat.com" , "peterx@redhat.com" References: <1585542301-84087-1-git-send-email-yi.l.liu@intel.com> <1585542301-84087-6-git-send-email-yi.l.liu@intel.com> From: Auger Eric Message-ID: <532069f1-123e-1ba0-cbd0-c849c2e0c5aa@redhat.com> Date: Thu, 2 Apr 2020 14:41:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 205.139.110.61 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "jean-philippe@linaro.org" , "Tian, Kevin" , Jacob Pan , Yi Sun , "kvm@vger.kernel.org" , "mst@redhat.com" , "Tian, Jun J" , "Sun, Yi Y" , "pbonzini@redhat.com" , "david@gibson.dropbear.id.au" , "Wu, Hao" Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Hi Yi, On 4/2/20 10:52 AM, Liu, Yi L wrote: >> From: Auger Eric < eric.auger@redhat.com> >> Sent: Monday, March 30, 2020 7:02 PM >> To: Liu, Yi L ; qemu-devel@nongnu.org; >> Subject: Re: [PATCH v2 05/22] hw/pci: modify pci_setup_iommu() to set >> PCIIOMMUOps >> >> >> >> On 3/30/20 6:24 AM, Liu Yi L wrote: >>> This patch modifies pci_setup_iommu() to set PCIIOMMUOps instead of >>> setting PCIIOMMUFunc. PCIIOMMUFunc is used to get an address space for >>> a PCI device in vendor specific way. The PCIIOMMUOps still offers this >>> functionality. But using PCIIOMMUOps leaves space to add more iommu >>> related vendor specific operations. >>> >>> Cc: Kevin Tian >>> Cc: Jacob Pan >>> Cc: Peter Xu >>> Cc: Eric Auger >>> Cc: Yi Sun >>> Cc: David Gibson >>> Cc: Michael S. Tsirkin >>> Reviewed-by: David Gibson >>> Reviewed-by: Peter Xu >>> Signed-off-by: Liu Yi L >>> --- >>> hw/alpha/typhoon.c | 6 +++++- >>> hw/arm/smmu-common.c | 6 +++++- >>> hw/hppa/dino.c | 6 +++++- >>> hw/i386/amd_iommu.c | 6 +++++- >>> hw/i386/intel_iommu.c | 6 +++++- >>> hw/pci-host/designware.c | 6 +++++- >>> hw/pci-host/pnv_phb3.c | 6 +++++- >>> hw/pci-host/pnv_phb4.c | 6 +++++- >>> hw/pci-host/ppce500.c | 6 +++++- >>> hw/pci-host/prep.c | 6 +++++- >>> hw/pci-host/sabre.c | 6 +++++- >>> hw/pci/pci.c | 12 +++++++----- >>> hw/ppc/ppc440_pcix.c | 6 +++++- >>> hw/ppc/spapr_pci.c | 6 +++++- >>> hw/s390x/s390-pci-bus.c | 8 ++++++-- hw/virtio/virtio-iommu.c | 6 >>> +++++- >>> include/hw/pci/pci.h | 8 ++++++-- >>> include/hw/pci/pci_bus.h | 2 +- >>> 18 files changed, 90 insertions(+), 24 deletions(-) >>> >>> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c index >>> 1795e2f..f271de1 100644 >>> --- a/hw/alpha/typhoon.c >>> +++ b/hw/alpha/typhoon.c >>> @@ -740,6 +740,10 @@ static AddressSpace *typhoon_pci_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &s->pchip.iommu_as; >>> } >>> >>> +static const PCIIOMMUOps typhoon_iommu_ops = { >>> + .get_address_space = typhoon_pci_dma_iommu, }; >>> + >>> static void typhoon_set_irq(void *opaque, int irq, int level) { >>> TyphoonState *s = opaque; >>> @@ -897,7 +901,7 @@ PCIBus *typhoon_init(MemoryRegion *ram, ISABus >> **isa_bus, qemu_irq *p_rtc_irq, >>> "iommu-typhoon", UINT64_MAX); >>> address_space_init(&s->pchip.iommu_as, MEMORY_REGION(&s- >>> pchip.iommu), >>> "pchip0-pci"); >>> - pci_setup_iommu(b, typhoon_pci_dma_iommu, s); >>> + pci_setup_iommu(b, &typhoon_iommu_ops, s); >>> >>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */ >>> memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), >>> &alpha_pci_iack_ops, diff --git a/hw/arm/smmu-common.c >>> b/hw/arm/smmu-common.c index e13a5f4..447146e 100644 >>> --- a/hw/arm/smmu-common.c >>> +++ b/hw/arm/smmu-common.c >>> @@ -343,6 +343,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, >> void *opaque, int devfn) >>> return &sdev->as; >>> } >>> >>> +static const PCIIOMMUOps smmu_ops = { >>> + .get_address_space = smmu_find_add_as, }; >>> + >>> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) { >>> uint8_t bus_n, devfn; >>> @@ -437,7 +441,7 @@ static void smmu_base_realize(DeviceState *dev, Error >> **errp) >>> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL); >>> >>> if (s->primary_bus) { >>> - pci_setup_iommu(s->primary_bus, smmu_find_add_as, s); >>> + pci_setup_iommu(s->primary_bus, &smmu_ops, s); >>> } else { >>> error_setg(errp, "SMMU is not attached to any PCI bus!"); >>> } >>> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c index 2b1b38c..3da4f84 >>> 100644 >>> --- a/hw/hppa/dino.c >>> +++ b/hw/hppa/dino.c >>> @@ -459,6 +459,10 @@ static AddressSpace *dino_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps dino_iommu_ops = { >>> + .get_address_space = dino_pcihost_set_iommu, }; >>> + >>> /* >>> * Dino interrupts are connected as shown on Page 78, Table 23 >>> * (Little-endian bit numbers) >>> @@ -580,7 +584,7 @@ PCIBus *dino_init(MemoryRegion *addr_space, >>> memory_region_add_subregion(&s->bm, 0xfff00000, >>> &s->bm_cpu_alias); >>> address_space_init(&s->bm_as, &s->bm, "pci-bm"); >>> - pci_setup_iommu(b, dino_pcihost_set_iommu, s); >>> + pci_setup_iommu(b, &dino_iommu_ops, s); >>> >>> *p_rtc_irq = qemu_allocate_irq(dino_set_timer_irq, s, 0); >>> *p_ser_irq = qemu_allocate_irq(dino_set_serial_irq, s, 0); diff >>> --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index >>> b1175e5..5fec30e 100644 >>> --- a/hw/i386/amd_iommu.c >>> +++ b/hw/i386/amd_iommu.c >>> @@ -1451,6 +1451,10 @@ static AddressSpace >> *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) >>> return &iommu_as[devfn]->as; >>> } >>> >>> +static const PCIIOMMUOps amdvi_iommu_ops = { >>> + .get_address_space = amdvi_host_dma_iommu, }; >>> + >>> static const MemoryRegionOps mmio_mem_ops = { >>> .read = amdvi_mmio_read, >>> .write = amdvi_mmio_write, >>> @@ -1577,7 +1581,7 @@ static void amdvi_realize(DeviceState *dev, >>> Error **errp) >>> >>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio); >>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR); >>> - pci_setup_iommu(bus, amdvi_host_dma_iommu, s); >>> + pci_setup_iommu(bus, &amdvi_iommu_ops, s); >>> s->devid = object_property_get_int(OBJECT(&s->pci), "addr", errp); >>> msi_init(&s->pci.dev, 0, 1, true, false, errp); >>> amdvi_init(s); >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index >>> df7ad25..4b22910 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3729,6 +3729,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &vtd_as->as; >>> } >>> >>> +static PCIIOMMUOps vtd_iommu_ops = { >> static const > > got it. > >>> + .get_address_space = vtd_host_dma_iommu, }; >>> + >>> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) { >>> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); @@ -3840,7 >>> +3844,7 @@ static void vtd_realize(DeviceState *dev, Error **errp) >>> g_free, g_free); >>> vtd_init(s); >>> sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, >> Q35_HOST_BRIDGE_IOMMU_ADDR); >>> - pci_setup_iommu(bus, vtd_host_dma_iommu, dev); >>> + pci_setup_iommu(bus, &vtd_iommu_ops, dev); >>> /* Pseudo address space under root PCI bus. */ >>> x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, >> Q35_PSEUDO_DEVFN_IOAPIC); >>> qemu_add_machine_init_done_notifier(&vtd_machine_done_notify); >>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c index >>> dd24551..4c6338a 100644 >>> --- a/hw/pci-host/designware.c >>> +++ b/hw/pci-host/designware.c >>> @@ -645,6 +645,10 @@ static AddressSpace >> *designware_pcie_host_set_iommu(PCIBus *bus, void *opaque, >>> return &s->pci.address_space; >>> } >>> >>> +static const PCIIOMMUOps designware_iommu_ops = { >>> + .get_address_space = designware_pcie_host_set_iommu, }; >>> + >>> static void designware_pcie_host_realize(DeviceState *dev, Error >>> **errp) { >>> PCIHostState *pci = PCI_HOST_BRIDGE(dev); @@ -686,7 +690,7 @@ >>> static void designware_pcie_host_realize(DeviceState *dev, Error **errp) >>> address_space_init(&s->pci.address_space, >>> &s->pci.address_space_root, >>> "pcie-bus-address-space"); >>> - pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s); >>> + pci_setup_iommu(pci->bus, &designware_iommu_ops, s); >>> >>> qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus)); >>> qdev_init_nofail(DEVICE(&s->root)); >>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index >>> 74618fa..ecfe627 100644 >>> --- a/hw/pci-host/pnv_phb3.c >>> +++ b/hw/pci-host/pnv_phb3.c >>> @@ -961,6 +961,10 @@ static AddressSpace *pnv_phb3_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &ds->dma_as; >>> } >>> >>> +static PCIIOMMUOps pnv_phb3_iommu_ops = { >> static const > got it. :-) > >>> + .get_address_space = pnv_phb3_dma_iommu, }; >>> + >>> static void pnv_phb3_instance_init(Object *obj) { >>> PnvPHB3 *phb = PNV_PHB3(obj); >>> @@ -1059,7 +1063,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error >> **errp) >>> &phb->pci_mmio, &phb->pci_io, >>> 0, 4, TYPE_PNV_PHB3_ROOT_BUS); >>> >>> - pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb); >>> + pci_setup_iommu(pci->bus, &pnv_phb3_iommu_ops, phb); >>> >>> /* Add a single Root port */ >>> qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id); >>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index >>> 23cf093..04e95e3 100644 >>> --- a/hw/pci-host/pnv_phb4.c >>> +++ b/hw/pci-host/pnv_phb4.c >>> @@ -1148,6 +1148,10 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &ds->dma_as; >>> } >>> >>> +static PCIIOMMUOps pnv_phb4_iommu_ops = { >> idem > will add const. > >>> + .get_address_space = pnv_phb4_dma_iommu, }; >>> + >>> static void pnv_phb4_instance_init(Object *obj) { >>> PnvPHB4 *phb = PNV_PHB4(obj); >>> @@ -1205,7 +1209,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error >> **errp) >>> pnv_phb4_set_irq, pnv_phb4_map_irq, phb, >>> &phb->pci_mmio, &phb->pci_io, >>> 0, 4, TYPE_PNV_PHB4_ROOT_BUS); >>> - pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb); >>> + pci_setup_iommu(pci->bus, &pnv_phb4_iommu_ops, phb); >>> >>> /* Add a single Root port */ >>> qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id); >>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index >>> d710727..5baf5db 100644 >>> --- a/hw/pci-host/ppce500.c >>> +++ b/hw/pci-host/ppce500.c >>> @@ -439,6 +439,10 @@ static AddressSpace *e500_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps ppce500_iommu_ops = { >>> + .get_address_space = e500_pcihost_set_iommu, }; >>> + >>> static void e500_pcihost_realize(DeviceState *dev, Error **errp) { >>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); @@ -473,7 +477,7 @@ >>> static void e500_pcihost_realize(DeviceState *dev, Error **errp) >>> memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX); >>> memory_region_add_subregion(&s->bm, 0x0, &s->busmem); >>> address_space_init(&s->bm_as, &s->bm, "pci-bm"); >>> - pci_setup_iommu(b, e500_pcihost_set_iommu, s); >>> + pci_setup_iommu(b, &ppce500_iommu_ops, s); >>> >>> pci_create_simple(b, 0, "e500-host-bridge"); >>> >>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c index >>> 1a02e9a..7c57311 100644 >>> --- a/hw/pci-host/prep.c >>> +++ b/hw/pci-host/prep.c >>> @@ -213,6 +213,10 @@ static AddressSpace *raven_pcihost_set_iommu(PCIBus >> *bus, void *opaque, >>> return &s->bm_as; >>> } >>> >>> +static const PCIIOMMUOps raven_iommu_ops = { >>> + .get_address_space = raven_pcihost_set_iommu, }; >>> + >>> static void raven_change_gpio(void *opaque, int n, int level) { >>> PREPPCIState *s = opaque; >>> @@ -303,7 +307,7 @@ static void raven_pcihost_initfn(Object *obj) >>> memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias); >>> memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias); >>> address_space_init(&s->bm_as, &s->bm, "raven-bm"); >>> - pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s); >>> + pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s); >>> >>> h->bus = &s->pci_bus; >>> >>> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c index >>> 2b8503b..251549b 100644 >>> --- a/hw/pci-host/sabre.c >>> +++ b/hw/pci-host/sabre.c >>> @@ -112,6 +112,10 @@ static AddressSpace *sabre_pci_dma_iommu(PCIBus >> *bus, void *opaque, int devfn) >>> return &is->iommu_as; >>> } >>> >>> +static const PCIIOMMUOps sabre_iommu_ops = { >>> + .get_address_space = sabre_pci_dma_iommu, }; >>> + >>> static void sabre_config_write(void *opaque, hwaddr addr, >>> uint64_t val, unsigned size) { @@ >>> -402,7 +406,7 @@ static void sabre_realize(DeviceState *dev, Error **errp) >>> /* IOMMU */ >>> memory_region_add_subregion_overlap(&s->sabre_config, 0x200, >>> sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1); >>> - pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu); >>> + pci_setup_iommu(phb->bus, &sabre_iommu_ops, s->iommu); >>> >>> /* APB secondary busses */ >>> pci_dev = pci_create_multifunction(phb->bus, PCI_DEVFN(1, 0), >>> true, diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e1ed667..aa9025c >>> 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -2644,7 +2644,7 @@ AddressSpace >> *pci_device_iommu_address_space(PCIDevice *dev) >>> PCIBus *iommu_bus = bus; >>> uint8_t devfn = dev->devfn; >>> >>> - while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) >> { >>> + while (iommu_bus && !iommu_bus->iommu_ops && >>> + iommu_bus->parent_dev) { >> Depending on future usage, this is not strictly identical to the original >> code. You exit >> the loop as soon as a iommu_bus->iommu_ops is set whatever the presence of >> get_address_space(). > > To be identical with original code, may adding the get_address_space() > presence check. Then the loop exits when the iommu_bus->iommu_ops is > set and meanwhile iommu_bus->iommu_ops->get_address_space() is set. > But is it possible that there is an intermediate iommu_bus which has > iommu_ops set but the get_address_space() is clear. I guess not as > iommu_ops is set by vIOMMU and vIOMMU won't differentiate buses? I don't know. That depends on how the ops are going to be used in the future. Can't you enforce the fact that get_address_space() is a mandatory ops? Thanks Eric > > Also the get_address_space() presence will be checked when trying to > use it. right? > > Regards, > Yi Liu >