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

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

On Sun, 2014-06-08 at 09:48 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 08:55 AM, Alex Williamson wrote:
> > 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.
> 
> 
> Like that (#2 adds empty switch{}, #4 enables two ioctls, below is #4)?
> 
>  int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>                           int req, void *param)
>  {
>      VFIOGroup *group;
>      VFIOContainer *container;
>      int ret = -1;
> 
>      switch (req) {
> +    case VFIO_CHECK_EXTENSION:
> +    case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
> +        break;
>      default:
>          /* Return an error on unknown requests */
>          return ret;
>      }
>      group = vfio_get_group(groupid, as);
>      if (!group) {
> 

Sure, I might have chosen to move the body to a separate static
function, making this a simple wrapper to avoid awkward code flow, but
it's up to you.  Thanks,

Alex