From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55185) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yg6eq-00022U-Vx for qemu-devel@nongnu.org; Thu, 09 Apr 2015 03:13:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yg6el-0005uO-Vb for qemu-devel@nongnu.org; Thu, 09 Apr 2015 03:13:48 -0400 Received: from mail-pd0-f171.google.com ([209.85.192.171]:36543) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yg6el-0005u7-NA for qemu-devel@nongnu.org; Thu, 09 Apr 2015 03:13:43 -0400 Received: by pdea3 with SMTP id a3so143339229pde.3 for ; Thu, 09 Apr 2015 00:13:42 -0700 (PDT) Message-ID: <5526269E.8080408@ozlabs.ru> Date: Thu, 09 Apr 2015 17:13:34 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1427779727-13353-1-git-send-email-aik@ozlabs.ru> <1427779727-13353-5-git-send-email-aik@ozlabs.ru> <20150408020121.GB28909@voom.redhat.com> <5524A44F.2010905@ozlabs.ru> <20150409064359.GM28909@voom.redhat.com> In-Reply-To: <20150409064359.GM28909@voom.redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v5 04/12] spapr_pci_vfio: Enable multiple groups per container List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf On 04/09/2015 04:43 PM, David Gibson wrote: > On Wed, Apr 08, 2015 at 01:45:19PM +1000, Alexey Kardashevskiy wrote: >> On 04/08/2015 12:01 PM, David Gibson wrote: >>> On Tue, Mar 31, 2015 at 04:28:39PM +1100, 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. >>>> >>>> This adds a sanity check that there is just one VFIO container per >>>> PHB address space. >>>> >>>> Signed-off-by: Alexey Kardashevskiy >>> >>> [snip] >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index b012620..99e1900 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -915,21 +915,23 @@ 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; >>>> + int ret; >>>> + VFIOAddressSpace *space; >>>> >>>> - group = vfio_get_group(groupid, as); >>>> - if (!group) { >>>> - error_report("vfio: group %d not registered", groupid); >>>> - return ret; >>>> - } >>>> + space = vfio_get_address_space(as); >>>> + container = QLIST_FIRST(&space->containers); >>> >>> So getting the container handle from the address space, rather than >>> the group id certainly makes more sense to me. >>> >>>> - container = group->container; >>>> - if (group->container) { >>>> + if (!container) { >>>> + error_report("vfio: container is not set"); >>>> + return -1; >>>> + } else if (QLIST_NEXT(container, next)) { >>>> + error_report("vfio: multiple containers per PHB are not supported"); >>>> + return -1; >>> >>> But if only one PHB per address space is possible, why is the >>> containers field a list in the first place? >> >> >> Historically the list was added in 3df3e0a5872 (the patch of yours >> :) ). > > Heh. > >> In theory we could implement spapr-pci-bridge (derived from pci-bridge) with >> isolation capability (i.e. its own LIOBN/DMA window), in this case there >> could be multiple containers per PHB address space. Other archs could want >> multiple containers for some other reason. It would help me a lot if you >> remembered why you kept the list at the first place :) > > Ok, I've looked over the patch and it has jogged my memory a bit. So > the dumb answer is that it's because the per address-space list was > replacing a global list of containers > > The more useful answer is that I think it was because I was > anticipating the possibility of working around the > one-group-per-container limit by allowing a single VFIOAddressSpace in > qemu to be backed by several containers, whose mappings would be kept > in sync from the userspace side by duplicating all mappings. > > Anyway, I think that means the right way to implement this is by > duplicating the ioctl() across all the attached containers, rather > than picking just one. Right. I will do that. >> For now I guess I'll move the next patch ("vfio: spapr: Move SPAPR-related >> code to a separate file") before this one, do s/vfio_container_do_ioctl/ >> vfio_spapr_container_do_ioctl/ and move it to hw/vfio/spapr.c. Makes >> sense? > > That sounds fine, though I don't see that it really addresses the > question here. You are right, it does not. I won't do it in this patchset then. Thanks. > > >> >> >>>> + } else { >>>> ret = ioctl(container->fd, req, param); >>>> if (ret < 0) { >>>> error_report("vfio: failed to ioctl %d to container: ret=%d, %s", >>>> @@ -937,12 +939,10 @@ static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, >>>> } >>>> } >>>> >>>> - 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 */ >>>> @@ -957,5 +957,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