From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9mI7-0004l4-7d for qemu-devel@nongnu.org; Mon, 29 Jun 2015 23:33:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9mI3-0000hO-Sd for qemu-devel@nongnu.org; Mon, 29 Jun 2015 23:32:59 -0400 Received: from mail-pd0-f174.google.com ([209.85.192.174]:33096) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9mI3-0000gT-MC for qemu-devel@nongnu.org; Mon, 29 Jun 2015 23:32:55 -0400 Received: by pdjd13 with SMTP id d13so2956886pdj.0 for ; Mon, 29 Jun 2015 20:32:53 -0700 (PDT) References: <1434627456-13745-1-git-send-email-aik@ozlabs.ru> <1434627456-13745-4-git-send-email-aik@ozlabs.ru> <1435262398.3700.391.camel@redhat.com> From: Alexey Kardashevskiy Message-ID: <55920DDD.6030409@ozlabs.ru> Date: Tue, 30 Jun 2015 13:32:45 +1000 MIME-Version: 1.0 In-Reply-To: <1435262398.3700.391.camel@redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v8 03/14] spapr_pci_vfio: Enable multiple groups per container List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: David Gibson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Gavin Shan , Alexander Graf On 06/26/2015 05:59 AM, Alex Williamson wrote: > On Thu, 2015-06-18 at 21:37 +1000, Alexey Kardashevskiy wrote: >> This enables multiple IOMMU groups in one VFIO container which means >> that multiple devices from different groups can share the same IOMMU >> table (or tables if DDW). >> >> This removes a group id from vfio_container_ioctl(). The kernel support >> is required for this; if the host kernel does not have the support, >> it will allow only one group per container. The PHB's "iommuid" property >> is ignored. The ioctl is called for every container attached to >> the address space. At the moment there is just one container anyway. >> >> If there is no container attached to the address space, >> vfio_container_do_ioctl() returns -1. >> >> This removes casts to sPAPRPHBVFIOState as none of sPAPRPHBVFIOState >> members is accessed here. >> >> Signed-off-by: Alexey Kardashevskiy >> Reviewed-by: David Gibson >> --- >> hw/ppc/spapr_pci_vfio.c | 21 ++++++--------------- >> hw/vfio/common.c | 20 ++++++-------------- >> include/hw/vfio/vfio.h | 2 +- >> 3 files changed, 13 insertions(+), 30 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 99a1be5..e89cbff 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -35,12 +35,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) >> sPAPRTCETable *tcet; >> uint32_t liobn = svphb->phb.dma_liobn; >> >> - if (svphb->iommugroupid == -1) { >> - error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid); >> - return; >> - } >> - >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&svphb->phb.iommu_as, >> VFIO_CHECK_EXTENSION, >> (void *) VFIO_SPAPR_TCE_IOMMU); >> if (ret != 1) { >> @@ -49,7 +44,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) >> return; >> } >> >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&sphb->iommu_as, >> VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info); >> if (ret) { >> error_setg_errno(errp, -ret, >> @@ -79,7 +74,6 @@ static void spapr_phb_vfio_reset(DeviceState *qdev) >> static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, >> unsigned int addr, int option) >> { >> - sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; >> int ret; >> >> @@ -116,7 +110,7 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, >> return RTAS_OUT_PARAM_ERROR; >> } >> >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&sphb->iommu_as, >> VFIO_EEH_PE_OP, &op); >> if (ret < 0) { >> return RTAS_OUT_HW_ERROR; >> @@ -127,12 +121,11 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, >> >> static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state) >> { >> - sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; >> int ret; >> >> op.op = VFIO_EEH_PE_GET_STATE; >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&sphb->iommu_as, >> VFIO_EEH_PE_OP, &op); >> if (ret < 0) { >> return RTAS_OUT_PARAM_ERROR; >> @@ -144,7 +137,6 @@ static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state) >> >> static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option) >> { >> - sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; >> int ret; >> >> @@ -162,7 +154,7 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option) >> return RTAS_OUT_PARAM_ERROR; >> } >> >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&sphb->iommu_as, >> VFIO_EEH_PE_OP, &op); >> if (ret < 0) { >> return RTAS_OUT_HW_ERROR; >> @@ -173,12 +165,11 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option) >> >> static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb) >> { >> - sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; >> int ret; >> >> op.op = VFIO_EEH_PE_CONFIGURE; >> - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + ret = vfio_container_ioctl(&sphb->iommu_as, >> VFIO_EEH_PE_OP, &op); >> if (ret < 0) { >> return RTAS_OUT_PARAM_ERROR; >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 3e4c685..369e564 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -793,34 +793,26 @@ void vfio_put_base_device(VFIODevice *vbasedev) >> close(vbasedev->fd); >> } >> >> -static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, >> +static int vfio_container_do_ioctl(AddressSpace *as, >> int req, void *param) >> { >> - VFIOGroup *group; >> VFIOContainer *container; >> int ret = -1; >> + VFIOAddressSpace *space = vfio_get_address_space(as); >> >> - group = vfio_get_group(groupid, as); >> - if (!group) { >> - error_report("vfio: group %d not registered", groupid); >> - return ret; >> - } >> - >> - container = group->container; >> - if (group->container) { >> + QLIST_FOREACH(container, &space->containers, next) { >> ret = ioctl(container->fd, req, param); >> if (ret < 0) { >> error_report("vfio: failed to ioctl %d to container: ret=%d, %s", >> _IOC_NR(req) - VFIO_BASE, ret, strerror(errno)); >> + return -errno; >> } >> } > > > How does this work with VFIO_EEH_PE_OP? Suddenly we're potentially > calling it on multiple containers? Gavin says yes. > Should all of this container ioctl > code also be pulled out to spapr? Well, I could but I won't as I am dropping 2/14 at all. > There's clearly an approach here where the patches could be split into > separate ppc and vfio series rather than this horrible practice of > intermixing them. Thanks, Oh. Ok. I'll move 3/14 further. I just wanted to put 2/14 and 3/14 earlier as it allowed me to avoid changing same lines twice in the same patchset. > > Alex >> >> - vfio_put_group(group); >> - >> return ret; >> } >> >> -int vfio_container_ioctl(AddressSpace *as, int32_t groupid, >> +int vfio_container_ioctl(AddressSpace *as, >> int req, void *param) >> { >> /* We allow only certain ioctls to the container */ >> @@ -835,5 +827,5 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, >> return -1; >> } >> >> - return vfio_container_do_ioctl(as, groupid, req, param); >> + return vfio_container_do_ioctl(as, req, param); >> } >> diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h >> index 0b26cd8..76b5744 100644 >> --- a/include/hw/vfio/vfio.h >> +++ b/include/hw/vfio/vfio.h >> @@ -3,7 +3,7 @@ >> >> #include "qemu/typedefs.h" >> >> -extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid, >> +extern int vfio_container_ioctl(AddressSpace *as, >> int req, void *param); >> >> #endif > > > -- Alexey