Re: [Qemu-devel] [PATCH v4 04/12] spapr vfio: add vfio_container_spapr_get_info()

[Alex Williamson]

On Fri, 2013-09-13 at 20:11 +1000, Alexey Kardashevskiy wrote:
> On 09/11/2013 08:11 AM, Alex Williamson wrote:
> > On Tue, 2013-09-10 at 18:36 +1000, Alexey Kardashevskiy wrote:
> >> On 09/06/2013 05:01 AM, Alex Williamson wrote:
> >>> On Fri, 2013-08-30 at 20:15 +1000, Alexey Kardashevskiy wrote:
> >>>> As sPAPR platform supports DMA windows on a PCI bus, the information
> >>>> about their location and size should be passed into the guest via
> >>>> the device tree.
> >>>>
> >>>> The patch adds a helper to read this info from the container fd.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> >>>> ---
> >>>> Changes:
> >>>> v4:
> >>>> * fixed possible leaks on error paths
> >>>> ---
> >>>>  hw/misc/vfio.c         | 45 
> >>>> +++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/hw/misc/vfio.h | 11 +++++++++++
> >>>>  2 files changed, 56 insertions(+)
> >>>>  create mode 100644 include/hw/misc/vfio.h
> >>>>
> >>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >>>> index 53791fb..4210471 100644
> >>>> --- a/hw/misc/vfio.c
> >>>> +++ b/hw/misc/vfio.c
> >>>> @@ -39,6 +39,7 @@
> >>>>  #include "qemu/range.h"
> >>>>  #include "sysemu/kvm.h"
> >>>>  #include "sysemu/sysemu.h"
> >>>> +#include "hw/misc/vfio.h"
> >>>>  
> >>>>  /* #define DEBUG_VFIO */
> >>>>  #ifdef DEBUG_VFIO
> >>>> @@ -3490,3 +3491,47 @@ static void register_vfio_pci_dev_type(void)
> >>>>  }
> >>>>  
> >>>>  type_init(register_vfio_pci_dev_type)
> >>>> +
> >>>> +int vfio_container_spapr_get_info(AddressSpace *as, int32_t groupid,
> >>>> +                                  struct vfio_iommu_spapr_tce_info 
> >>>> *info,
> >>>> +                                  int *group_fd)
> >>>> +{
> >>>> +    VFIOAddressSpace *space;
> >>>> +    VFIOGroup *group;
> >>>> +    VFIOContainer *container;
> >>>> +    int ret, fd;
> >>>> +
> >>>> +    space = vfio_get_address_space(as);
> >>>> +    if (!space) {
> >>>> +        return -1;
> >>>> +    }
> >>>> +    group = vfio_get_group(groupid, space);
> >>>> +    if (!group) {
> >>>> +        goto put_as_exit;
> >>>> +    }
> >>>> +    container = group->container;
> >>>> +    if (!group->container) {
> >>>> +        goto put_group_exit;
> >>>> +    }
> >>>> +    fd = container->fd;
> >>>> +    if (!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
> >>>> +        goto put_group_exit;
> >>>> +    }
> >>>> +    ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, info);
> >>>> +    if (ret) {
> >>>> +        error_report("vfio: failed to get iommu info for container: %s",
> >>>> +                     strerror(errno));
> >>>> +        goto put_group_exit;
> >>>> +    }
> >>>> +    *group_fd = group->fd;
> >>>
> >>> The above gets don't actually increment a reference count, so copying
> >>> the fd seems risky here.
> >>
> >>
> >> If fd is gone while I am carrying it to my "external VFIO user" to call
> >> kvmppc_vfio_group_get_external_user() on it, then the guest just shut
> >> itself in a foot, no?
> >> And I do not see how I would make it no risky, do you?
> > 
> > We've handled the case in the kernel where the IOMMU code has a
> > reference to the group so the group won't go away as long as that
> > reference is in place, but we don't have that in QEMU.  If you supported
> > hotplug, how would QEMU vfio notify spapr code to release the group?  I
> > think you'd be left with the spapr kernel code holding the group
> > reference and possibly a bogus file descriptor in QEMU if the group is
> > close()'d and you've cached it from the above code.  Perhaps it's
> > sufficient to note that you don't support hot remove, but do you
> > actually do anything to prevent it?  Thanks,
> 
> 
> I do not cache group_fd, I copy iе from VFIOGroup and immediately pass it
> to KVM which immediately calls fget() on it. This is really short distance
> and the only thing for protection here would be:
> 
> -    *group_fd = group->fd;
> +    *group_fd = dup(group->fd);
> 
> and then close(group_fd) after I passed it to KVM. I guess it has to be
> done anyway. But I suspect this is not what you are talking about...

Meanwhile each of the processors has executed several million
instructions during this sequence of "immediate" events.  Besides, this
just creates the interface, who uses it and how is outside of our
control after this is in place.  Rather than creating an interface where
you can ask for info, some of which may be closely tied to the lifecycle
of a specific device, why not make an interface where vfio-pci can
register and unregister information about a device as part of it's
lifecycle?  That at least gives you an end point after which you know
the data is no longer valid.  Thanks,

Alex

> >>>> +
> >>>> +    return 0;
> >>>> +
> >>>> +put_group_exit:
> >>>> +    vfio_put_group(group);
> >>>> +
> >>>> +put_as_exit:
> >>>> +    vfio_put_address_space(space);
> >>>
> >>> But put_group calls disconnect_container which calls
> >>> put_address_space... so it get's put twice.  The lack of symmetry
> >>> already bites us with a bug.
> >>
> >> True. This will be fixed by moving vfio_get_address_space() into
> >> vfio_get_group().
> 
>