2014-10-27 18:09:13

by Antonios Motakis

Subject: [PATCH v9 06/19] vfio/platform: return info for bound device

A VFIO userspace driver will start by opening the VFIO device
that corresponds to an IOMMU group, and will use the ioctl interface
to get the basic device info, such as number of memory regions and
interrupts, and their properties. This patch enables the
VFIO_DEVICE_GET_INFO ioctl call.

Signed-off-by: Antonios Motakis <[email protected]>
---
drivers/vfio/platform/vfio_platform_common.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e0fdbc8..cb20526 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -43,10 +43,27 @@ static int vfio_platform_open(void *device_data)
static long vfio_platform_ioctl(void *device_data,
unsigned int cmd, unsigned long arg)
{
- if (cmd == VFIO_DEVICE_GET_INFO)
- return -EINVAL;
+ struct vfio_platform_device *vdev = device_data;
+ unsigned long minsz;
+
+ if (cmd == VFIO_DEVICE_GET_INFO) {
+ struct vfio_device_info info;
+
+ minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ info.flags = vdev->flags;
+ info.num_regions = 0;
+ info.num_irqs = 0;
+
+ return copy_to_user((void __user *)arg, &info, minsz);

- else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
+ } else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
return -EINVAL;

else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
--
2.1.1

2014-11-12 16:36:29

by Alex Williamson

Subject: Re: [PATCH v9 06/19] vfio/platform: return info for bound device

On Wed, 2014-11-12 at 11:32 +0100, Eric Auger wrote:
> On 10/27/2014 07:07 PM, Antonios Motakis wrote:
> > A VFIO userspace driver will start by opening the VFIO device
> > that corresponds to an IOMMU group, and will use the ioctl interface
> > to get the basic device info, such as number of memory regions and
> > interrupts, and their properties. This patch enables the
> > VFIO_DEVICE_GET_INFO ioctl call.
> >
> > Signed-off-by: Antonios Motakis <[email protected]>
> > ---
> > drivers/vfio/platform/vfio_platform_common.c | 23 ++++++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> > index e0fdbc8..cb20526 100644
> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > @@ -43,10 +43,27 @@ static int vfio_platform_open(void *device_data)
> > static long vfio_platform_ioctl(void *device_data,
> > unsigned int cmd, unsigned long arg)
> > {
> > - if (cmd == VFIO_DEVICE_GET_INFO)
> > - return -EINVAL;
> > + struct vfio_platform_device *vdev = device_data;
> > + unsigned long minsz;
> > +
> > + if (cmd == VFIO_DEVICE_GET_INFO) {
> > + struct vfio_device_info info;
> > +
> > + minsz = offsetofend(struct vfio_device_info, num_irqs);
> > +
> > + if (copy_from_user(&info, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (info.argsz < minsz)
> > + return -EINVAL;
> > +
> > + info.flags = vdev->flags;
> > + info.num_regions = 0;
> > + info.num_irqs = 0;
> Seems a bit weird to me to enable the modality but returning zeroed
> values. Shouldn't we put that patch after VFIO_DEVICE_GET_REGION_INFO
> and VFIO_DEVICE_GET_IRQ_INFO ones?

I actually like how Antonios has started from a base framework, exposing
a device but none of the resources and then incrementally adds each
component. It's also a good showcase of the VFIO ABI that we can do
things like this. Thanks,

Alex

2014-11-20 14:11:14

by Antonios Motakis

Subject: Re: [PATCH v9 06/19] vfio/platform: return info for bound device

On Wed, Nov 12, 2014 at 5:36 PM, Alex Williamson
<[email protected]> wrote:
> On Wed, 2014-11-12 at 11:32 +0100, Eric Auger wrote:
>> On 10/27/2014 07:07 PM, Antonios Motakis wrote:
>> > A VFIO userspace driver will start by opening the VFIO device
>> > that corresponds to an IOMMU group, and will use the ioctl interface
>> > to get the basic device info, such as number of memory regions and
>> > interrupts, and their properties. This patch enables the
>> > VFIO_DEVICE_GET_INFO ioctl call.
>> >
>> > Signed-off-by: Antonios Motakis <[email protected]>
>> > ---
>> > drivers/vfio/platform/vfio_platform_common.c | 23 ++++++++++++++++++++---
>> > 1 file changed, 20 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> > index e0fdbc8..cb20526 100644
>> > --- a/drivers/vfio/platform/vfio_platform_common.c
>> > +++ b/drivers/vfio/platform/vfio_platform_common.c
>> > @@ -43,10 +43,27 @@ static int vfio_platform_open(void *device_data)
>> > static long vfio_platform_ioctl(void *device_data,
>> > unsigned int cmd, unsigned long arg)
>> > {
>> > - if (cmd == VFIO_DEVICE_GET_INFO)
>> > - return -EINVAL;
>> > + struct vfio_platform_device *vdev = device_data;
>> > + unsigned long minsz;
>> > +
>> > + if (cmd == VFIO_DEVICE_GET_INFO) {
>> > + struct vfio_device_info info;
>> > +
>> > + minsz = offsetofend(struct vfio_device_info, num_irqs);
>> > +
>> > + if (copy_from_user(&info, (void __user *)arg, minsz))
>> > + return -EFAULT;
>> > +
>> > + if (info.argsz < minsz)
>> > + return -EINVAL;
>> > +
>> > + info.flags = vdev->flags;
>> > + info.num_regions = 0;
>> > + info.num_irqs = 0;
>> Seems a bit weird to me to enable the modality but returning zeroed
>> values. Shouldn't we put that patch after VFIO_DEVICE_GET_REGION_INFO
>> and VFIO_DEVICE_GET_IRQ_INFO ones?
>
> I actually like how Antonios has started from a base framework, exposing
> a device but none of the resources and then incrementally adds each
> component. It's also a good showcase of the VFIO ABI that we can do
> things like this. Thanks,

I also agree with Alex with this. But of course I'm not married with
any particular splitting style, in case we decide to change this.

>
> Alex
>

2014-11-20 14:27:29

by Eric Auger

Subject: Re: [PATCH v9 06/19] vfio/platform: return info for bound device

On 11/20/2014 03:10 PM, Antonios Motakis wrote:
> On Wed, Nov 12, 2014 at 5:36 PM, Alex Williamson
> <[email protected]> wrote:
>> On Wed, 2014-11-12 at 11:32 +0100, Eric Auger wrote:
>>> On 10/27/2014 07:07 PM, Antonios Motakis wrote:
>>>> A VFIO userspace driver will start by opening the VFIO device
>>>> that corresponds to an IOMMU group, and will use the ioctl interface
>>>> to get the basic device info, such as number of memory regions and
>>>> interrupts, and their properties. This patch enables the
>>>> VFIO_DEVICE_GET_INFO ioctl call.
>>>>
>>>> Signed-off-by: Antonios Motakis <[email protected]>
>>>> ---
>>>> drivers/vfio/platform/vfio_platform_common.c | 23 ++++++++++++++++++++---
>>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index e0fdbc8..cb20526 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -43,10 +43,27 @@ static int vfio_platform_open(void *device_data)
>>>> static long vfio_platform_ioctl(void *device_data,
>>>> unsigned int cmd, unsigned long arg)
>>>> {
>>>> - if (cmd == VFIO_DEVICE_GET_INFO)
>>>> - return -EINVAL;
>>>> + struct vfio_platform_device *vdev = device_data;
>>>> + unsigned long minsz;
>>>> +
>>>> + if (cmd == VFIO_DEVICE_GET_INFO) {
>>>> + struct vfio_device_info info;
>>>> +
>>>> + minsz = offsetofend(struct vfio_device_info, num_irqs);
>>>> +
>>>> + if (copy_from_user(&info, (void __user *)arg, minsz))
>>>> + return -EFAULT;
>>>> +
>>>> + if (info.argsz < minsz)
>>>> + return -EINVAL;
>>>> +
>>>> + info.flags = vdev->flags;
>>>> + info.num_regions = 0;
>>>> + info.num_irqs = 0;
>>> Seems a bit weird to me to enable the modality but returning zeroed
>>> values. Shouldn't we put that patch after VFIO_DEVICE_GET_REGION_INFO
>>> and VFIO_DEVICE_GET_IRQ_INFO ones?
>>
>> I actually like how Antonios has started from a base framework, exposing
>> a device but none of the resources and then incrementally adds each
>> component. It's also a good showcase of the VFIO ABI that we can do
>> things like this. Thanks,
>
> I also agree with Alex with this. But of course I'm not married with
> any particular splitting style, in case we decide to change this.

Hi Antonios,
please keep as is. I also learn each day about splitting style ;-)
Best Regards
Eric
>
>>
>> Alex
>>