Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2014-06-07 at 08:50 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 03:35 AM, Alex Williamson wrote:
> > On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
> >> On 06/07/2014 02:48 AM, Alex Williamson wrote:
> >>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> >>>> While most operations with VFIO IOMMU driver are generic and used inside
> >>>> vfio.c, there are still some operations which only specific VFIO IOMMU
> >>>> drivers implement. The first example of it will be reading a DMA window
> >>>> start from the host.
> >>>>
> >>>> This adds a helper which passes an ioctl request to the container's fd.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>> Changes:
> >>>> v8:
> >>>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
> >>>> generalized
> >>>>
> >>>> v7:
> >>>> * do not return a group fd from the helper
> >>>>
> >>>> v6:
> >>>> * added dup() to protect group_fd from accidental disposal
> >>>>
> >>>> v5:
> >>>> * reworked to reflect change in vfio_get_group() from one
> >>>> of previous patches change
> >>>>
> >>>> v4:
> >>>> * fixed possible leaks on error paths
> >>>> ---
> >>>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
> >>>>  include/hw/misc/vfio.h |  9 +++++++++
> >>>>  2 files changed, 37 insertions(+)
> >>>>  create mode 100644 include/hw/misc/vfio.h
> >>>>
> >>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >>>> index 7437c2e..bb77934 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
> >>>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
> >>>>  }
> >>>>  
> >>>>  type_init(register_vfio_pci_dev_type)
> >>>> +
> >>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >>>> +                         int req, void *param)
> >>>> +{
> >>>> +    VFIOGroup *group;
> >>>> +    VFIOContainer *container;
> >>>> +    int ret = -1;
> >>>> +
> >>>> +    group = vfio_get_group(groupid, as);
> >>>> +    if (!group) {
> >>>> +        error_report("vfio: group %d not registered", groupid);
> >>>> +        return ret;
> >>>> +    }
> 
> 
> >>>
> >>> Gavin's version of this walked existing groups and returned an error if
> >>> not found.  vfio_get_group() will go out and create a group and connect
> >>> it to a container.  I'm not sure we really want to expose that sort of
> >>> capability out to the reset of QEMU.  Thanks,
> 
> >>
> >>
> >> If it is not created here, then it can only be created from vfio_initfn()
> >> which is too late as I need group to get DMA window properties for PHB when
> >> PHB is being realized.
> >>
> >> Add another helper?
> > 
> > Urgh.  No, let's not make another helper.  Can we at least keep the
> > filtering Gavin had so we don't just allow any ioctl through here
> > though?  At least we can make vfio.c involved if someone what's to do
> > something crazy here.  Thanks,
> 
> 
> if (req != CHECK_EXTENSION) &&
>    (req != ENABLE) &&
>    (reg != EEH_OP)
> 	return -1;
> 
> Like that (roughly)?

A switch statement seems a lot cleaner.
>  
> >>>> +
> >>>> +    container = group->container;
> >>>> +    if (group->container) {
> >>>> +        ret = ioctl(container->fd, req, param);
> >>>> +        if (ret < 0) {
> >>>> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
> >>>> +                         ret, strerror(errno));
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    vfio_put_group(group);
> >>>> +
> >>>> +    return ret;
> >>>> +}
> >>>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> >>>> new file mode 100644
> >>>> index 0000000..0b26cd8
> >>>> --- /dev/null
> >>>> +++ b/include/hw/misc/vfio.h
> >>>> @@ -0,0 +1,9 @@
> >>>> +#ifndef VFIO_API_H
> >>>> +#define VFIO_API_H
> >>>> +
> >>>> +#include "qemu/typedefs.h"
> >>>> +
> >>>> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >>>> +                                int req, void *param);
> >>>> +
> >>>> +#endif
> >>>
> >>>
> >>>
> >>
> >>
> > 
> > 
> > 
> 
>