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 <a...@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