|
From: | Alexey Kardashevskiy |
Subject: | Re: [Qemu-devel] [PATCH qemu v5 04/12] spapr_pci_vfio: Enable multiple groups per container |
Date: | Thu, 09 Apr 2015 17:13:34 +1000 |
User-agent: | Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
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 <address@hidden>[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
[Prev in Thread] | Current Thread | [Next in Thread] |