On Tue, 2013-09-10 at 18:09 +1000, Alexey Kardashevskiy wrote:
> On 09/06/2013 04:24 AM, Alex Williamson wrote:
> > On Fri, 2013-08-30 at 20:15 +1000, Alexey Kardashevskiy wrote:
> >> From: David Gibson <da...@gibson.dropbear.id.au>
> >>
> >> So far, VFIO has a notion of different logical DMA address spaces, but
> >> only ever uses one (system memory). This patch extends this, creating
> >> new VFIOAddressSpace objects as necessary, according to the AddressSpace
> >> reported by the PCI subsystem for this device's DMAs.
> >>
> >> This isn't enough yet to support guest side IOMMUs with VFIO, but it does
> >> mean we could now support VFIO devices on, for example, a guest side PCI
> >> host bridge which maps system memory at somewhere other than 0 in PCI
> >> space.
> >>
> >> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> >> ---
> >> hw/misc/vfio.c | 43 +++++++++++++++++++++++++++++++++++--------
> >> 1 file changed, 35 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 93a316e..c16f41b 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -133,9 +133,10 @@ enum {
> >> typedef struct VFIOAddressSpace {
> >> AddressSpace *as;
> >> QLIST_HEAD(, VFIOContainer) containers;
> >> + QLIST_ENTRY(VFIOAddressSpace) list;
> >> } VFIOAddressSpace;
> >>
> >> -static VFIOAddressSpace vfio_address_space_memory;
> >> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
> >>
> >> struct VFIOGroup;
> >>
> >> @@ -2611,10 +2612,34 @@ static int vfio_load_rom(VFIODevice *vdev)
> >> return 0;
> >> }
> >>
> >> -static void vfio_address_space_init(VFIOAddressSpace *space, AddressSpace
> >> *as)
> >> +static VFIOAddressSpace *vfio_get_address_space(AddressSpace *as)
> >> {
> >> + VFIOAddressSpace *space;
> >> +
> >> + QLIST_FOREACH(space, &vfio_address_spaces, list) {
> >> + if (space->as == as) {
> >> + return space;
> >> + }
> >> + }
> >> +
> >> + /* No suitable VFIOAddressSpace, create a new one */
> >> + space = g_malloc0(sizeof(*space));
> >> space->as = as;
> >> QLIST_INIT(&space->containers);
> >> +
> >> + QLIST_INSERT_HEAD(&vfio_address_spaces, space, list);
> >> +
> >> + return space;
> >> +}
> >> +
> >> +static void vfio_put_address_space(VFIOAddressSpace *space)
> >> +{
> >> + if (!QLIST_EMPTY(&space->containers)) {
> >> + return;
> >> + }
> >> +
> >> + QLIST_REMOVE(space, list);
> >> + g_free(space);
> >> }
> >>
> >> static int vfio_connect_container(VFIOGroup *group, VFIOAddressSpace
> >> *space)
> >> @@ -2699,6 +2724,8 @@ static void vfio_disconnect_container(VFIOGroup
> >> *group)
> >> group->container = NULL;
> >>
> >> if (QLIST_EMPTY(&container->group_list)) {
> >> + VFIOAddressSpace *space = container->space;
> >> +
> >> if (container->iommu_data.release) {
> >> container->iommu_data.release(container);
> >> }
> >> @@ -2706,6 +2733,8 @@ static void vfio_disconnect_container(VFIOGroup
> >> *group)
> >> DPRINTF("vfio_disconnect_container: close container->fd\n");
> >> close(container->fd);
> >> g_free(container);
> >> +
> >> + vfio_put_address_space(space);
> >> }
> >> }
> >>
> >> @@ -3076,6 +3105,7 @@ static int vfio_initfn(PCIDevice *pdev)
> >> {
> >> VFIODevice *pvdev, *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
> >> VFIOGroup *group;
> >> + VFIOAddressSpace *space;
> >> char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
> >> ssize_t len;
> >> struct stat st;
> >> @@ -3111,14 +3141,12 @@ static int vfio_initfn(PCIDevice *pdev)
> >> DPRINTF("%s(%04x:%02x:%02x.%x) group %d\n", __func__,
> >> vdev->host.domain,
> >> vdev->host.bus, vdev->host.slot, vdev->host.function,
> >> groupid);
> >>
> >> - if (pci_device_iommu_address_space(pdev) != &address_space_memory) {
> >> - error_report("vfio: DMA address space must be system memory");
> >> - return -EINVAL;
> >> - }
> >> + space = vfio_get_address_space(pci_device_iommu_address_space(pdev));
> >>
> >> - group = vfio_get_group(groupid, &vfio_address_space_memory);
> >> + group = vfio_get_group(groupid, space);
> >> if (!group) {
> >> error_report("vfio: failed to get group %d", groupid);
> >> + vfio_put_address_space(space);
> >> return -ENOENT;
> >> }
> >>
> >
> > Kind of a code flow issue here, on teardown we have:
> >
> > vfio_put_group
> > vfio_disconnect_container
> > vfio_put_address_space
> >
> > On setup we do:
> >
> > vfio_get_address_space
> > vfio_get_group
> > vfio_connect_container
> >
> > We could easily move vfio_get_address_space into vfio_get_group to make
> > things a little more balanced. It doesn't seem like too much additional
> > to pass the address space through vfio_get_group into
> > vfio_connect_container so that we could have a completely symmetric flow
> > though.
>
> I can do that. I will just need to call vfio_put_address_space() on every
> branch which returns NULL. Or rework a bit more. So I ended up with this:
>
> (not a patch, just cut-n-paste).
> ===
>
> -static VFIOGroup *vfio_get_group(int groupid, VFIOAddressSpace *space)
> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
> {
> + VFIOAddressSpace *space;
> VFIOGroup *group;
> char path[32];
> struct vfio_group_status status = { .argsz = sizeof(status) };
>
> + space = vfio_get_address_space(as);
> +
You don't even need this here, you could test
group->container->space->as == as
below, then pass "as" to vfio_connect_container() instead of "space",
then the space would be created and destroyed by the container setup
code. I think that would cleanup your exit paths too, not that we
couldn't stand to cleanup some of the duplicate exit code w/ a goto
anyway. Thanks,
Alex
> QLIST_FOREACH(group, &group_list, next) {
> if (group->groupid == groupid) {
> /* Found it. Now is it already in the right context? */
> @@ -2723,7 +2755,7 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
> } else {
> error_report("vfio: group %d used in multiple address
> spaces",
> group->groupid);
> - return NULL;
> + goto error_exit;
> }
> }
> }
> @@ -2734,24 +2766,19 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
> group->fd = qemu_open(path, O_RDWR);
> if (group->fd < 0) {
> error_report("vfio: error opening %s: %m", path);
> - g_free(group);
> - return NULL;
> + goto free_group_exit;
> }
>
> if (ioctl(group->fd, VFIO_GROUP_GET_STATUS, &status)) {
> error_report("vfio: error getting group status: %m");
> - close(group->fd);
> - g_free(group);
> - return NULL;
> + goto close_fd_exit;
> }
>
> if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
> error_report("vfio: error, group %d is not viable, please ensure "
> "all devices within the iommu_group are bound to their "
> "vfio bus driver.", groupid);
> - close(group->fd);
> - g_free(group);
> - return NULL;
> + goto close_fd_exit;
> }
>
> group->groupid = groupid;
> @@ -2759,14 +2786,23 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
>
> if (vfio_connect_container(group, space)) {
> error_report("vfio: failed to setup container for group %d",
> groupid);
> - close(group->fd);
> - g_free(group);
> - return NULL;
> + goto close_fd_exit;
> }
>
> QLIST_INSERT_HEAD(&group_list, group, next);
>
> return group;
> +
> +close_fd_exit:
> + close(group->fd);
> +
> +free_group_exit:
> + g_free(group);
> +
> +error_exit:
> + vfio_put_address_space(space);
> +
> + return NULL;
> }
>
> ===
>
> Is that ok? Split it into 2 patches for easier review?
> >
> >
> >> @@ -3339,7 +3367,6 @@ static const TypeInfo vfio_pci_dev_info = {
> >>
> >> static void register_vfio_pci_dev_type(void)
> >> {
> >> - vfio_address_space_init(&vfio_address_space_memory,
> >> &address_space_memory);
> >> type_register_static(&vfio_pci_dev_info);
> >> }
> >>
> >
> >
> >
>
>
