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

[Alexey Kardashevskiy]

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 <address@hidden>
>>>>>> ---
>>>>>> 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) {



>>  
>>>>>> +
>>>>>> +    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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
Alexey