diff mbox

[3/7] Return info for device and its memory regions and interrupts

Message ID 1380554923-17818-4-git-send-email-a.motakis@virtualopensystems.com (mailing list archive)
State New, archived
Headers show

Commit Message

Antonios Motakis Sept. 30, 2013, 3:28 p.m. UTC
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 implements the IOCTLs:
 - VFIO_DEVICE_GET_INFO
 - VFIO_DEVICE_GET_REGION_INFO
 - VFIO_DEVICE_GET_IRQ_INFO

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/vfio_platform.c | 60 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 7 deletions(-)

Comments

Bharat Bhushan Sept. 30, 2013, 3:49 p.m. UTC | #1
> -----Original Message-----
> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> bounces@lists.linux-foundation.org] On Behalf Of Antonios Motakis
> Sent: Monday, September 30, 2013 8:59 PM
> To: kvmarm@lists.cs.columbia.edu; alex.williamson@redhat.com
> Cc: linux-samsung-soc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Yoder
> Stuart-B08248; iommu@lists.linux-foundation.org; Antonios Motakis;
> tech@virtualopensystems.com
> Subject: [PATCH 3/7] Return info for device and its memory regions and
> interrupts
> 
> 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 implements the IOCTLs:
>  - VFIO_DEVICE_GET_INFO
>  - VFIO_DEVICE_GET_REGION_INFO
>  - VFIO_DEVICE_GET_IRQ_INFO
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/vfio_platform.c | 60 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_platform.c b/drivers/vfio/vfio_platform.c index
> b9686b0..a0abcfa 100644
> --- a/drivers/vfio/vfio_platform.c
> +++ b/drivers/vfio/vfio_platform.c
> @@ -28,6 +28,10 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/vfio.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> 
>  #define DRIVER_VERSION  "0.1"
>  #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
> @@ -54,10 +58,13 @@ static long vfio_platform_ioctl(void *device_data,
>  			   unsigned int cmd, unsigned long arg)  {
>  	struct vfio_platform_device *vdev = device_data;
> +	struct device_node *of_node = vdev->pdev->dev.of_node;
>  	unsigned long minsz;
> 
>  	if (cmd == VFIO_DEVICE_GET_INFO) {
>  		struct vfio_device_info info;
> +		struct resource res;
> +		int cnt = 0;
> 
>  		minsz = offsetofend(struct vfio_device_info, num_irqs);
> 
> @@ -68,18 +75,57 @@ static long vfio_platform_ioctl(void *device_data,
>  			return -EINVAL;
> 
>  		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
> -		info.num_regions = 0;
> -		info.num_irqs = 0;
> +
> +		while (!of_address_to_resource(of_node, cnt, &res))
> +			cnt++;
> +
> +		info.num_regions = cnt;
> +
> +		info.num_irqs = of_irq_count(of_node);
> 
>  		return copy_to_user((void __user *)arg, &info, minsz);
> 
> -	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> -		return -EINVAL;
> +	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> +		struct vfio_region_info info;
> +		struct resource res;
> 
> -	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> -		return -EINVAL;
> +		minsz = offsetofend(struct vfio_region_info, offset);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		if(of_address_to_resource(of_node, info.index, &res))
> +			return -EINVAL;
> +
> +		info.offset = res.start;	/* map phys addr with offset */
> +		info.size = resource_size(&res);
> +		info.flags = 0;
> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
> +
> +	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
> +		struct vfio_irq_info info;
> +		struct resource res;
> +
> +		minsz = offsetofend(struct vfio_irq_info, count);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		of_irq_to_resource(of_node, info.index, &res);

Why are we calling the above function if not using res?

> +
> +		info.flags = 0;
> +		info.count = 1;

I believe count here is number of interrupts, and we can have devices with more than 1 interrupt.

-Bharat

> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
> 
> -	else if (cmd == VFIO_DEVICE_SET_IRQS)
> +	} else if (cmd == VFIO_DEVICE_SET_IRQS)
>  		return -EINVAL;
> 
>  	else if (cmd == VFIO_DEVICE_RESET)
> --
> 1.8.1.2
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson Sept. 30, 2013, 5:39 p.m. UTC | #2
On Mon, 2013-09-30 at 17:28 +0200, 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 implements the IOCTLs:
>  - VFIO_DEVICE_GET_INFO
>  - VFIO_DEVICE_GET_REGION_INFO
>  - VFIO_DEVICE_GET_IRQ_INFO
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/vfio_platform.c | 60 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_platform.c b/drivers/vfio/vfio_platform.c
> index b9686b0..a0abcfa 100644
> --- a/drivers/vfio/vfio_platform.c
> +++ b/drivers/vfio/vfio_platform.c
> @@ -28,6 +28,10 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/vfio.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
>  
>  #define DRIVER_VERSION  "0.1"
>  #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
> @@ -54,10 +58,13 @@ static long vfio_platform_ioctl(void *device_data,
>  			   unsigned int cmd, unsigned long arg)
>  {
>  	struct vfio_platform_device *vdev = device_data;
> +	struct device_node *of_node = vdev->pdev->dev.of_node;
>  	unsigned long minsz;
>  
>  	if (cmd == VFIO_DEVICE_GET_INFO) {
>  		struct vfio_device_info info;
> +		struct resource res;
> +		int cnt = 0;
>  
>  		minsz = offsetofend(struct vfio_device_info, num_irqs);
>  
> @@ -68,18 +75,57 @@ static long vfio_platform_ioctl(void *device_data,
>  			return -EINVAL;
>  
>  		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
> -		info.num_regions = 0;
> -		info.num_irqs = 0;
> +
> +		while (!of_address_to_resource(of_node, cnt, &res))
> +			cnt++;
> +
> +		info.num_regions = cnt;
> +
> +		info.num_irqs = of_irq_count(of_node);
>  
>  		return copy_to_user((void __user *)arg, &info, minsz);
>  
> -	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
> -		return -EINVAL;
> +	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> +		struct vfio_region_info info;
> +		struct resource res;
>  
> -	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
> -		return -EINVAL;
> +		minsz = offsetofend(struct vfio_region_info, offset);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		if(of_address_to_resource(of_node, info.index, &res))
> +			return -EINVAL;
> +
> +		info.offset = res.start;	/* map phys addr with offset */
> +		info.size = resource_size(&res);
> +		info.flags = 0;
> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
> +
> +	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
> +		struct vfio_irq_info info;
> +		struct resource res;
> +
> +		minsz = offsetofend(struct vfio_irq_info, count);
> +
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		of_irq_to_resource(of_node, info.index, &res);
> +
> +		info.flags = 0;
> +		info.count = 1;
> +
> +		return copy_to_user((void __user *)arg, &info, minsz);
>  
> -	else if (cmd == VFIO_DEVICE_SET_IRQS)
> +	} else if (cmd == VFIO_DEVICE_SET_IRQS)
>  		return -EINVAL;
>  
>  	else if (cmd == VFIO_DEVICE_RESET)

I notice all the open firmware calls here and I'm curious, will all
platform devices be making use of open firmware?  I don't know if this
is synonymous with device tree or not.  Thanks,

Alex 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yoder Stuart-B08248 Oct. 1, 2013, 7:32 p.m. UTC | #3
> Antonios Motakis wrote
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> > I notice all the open firmware calls here and I'm curious,
> > will all platform devices be making use of open firmware?
> > I don't know if this is synonymous with device tree or not.
> > Thanks,
>
> This VFIO driver will support only devices implemented
> on the device tree. While there can be platform devices
> outside the device tree, I don't think it makes sense
> to support them from the same driver. This is why I
> originally called the driver VFIO_DT, however I renamed
> it to VFIO_PLATFORM after feedback from the first
> RFC. However personally, I still think the VFIO_DT name
> is more appropriate since we don't support all platform
> devices, only those that use the device tree.

But there is no 'device tree' bus.  The bus type we're
dealing with is a platform bus.

vfio for platform devices should be independent of whether
the device was discovered in a device tree or not.
All you're doing is exposing mappable regions and IRQs
to user space and it does not matter where the info originated.

You should be using platform bus structs here not
reparsing device tree nodes.  The struct
platform_device already has resource info in the 
struct:

struct platform_device {
        const char      *name;
        u32             id;
        struct device   dev;
        u32             num_resources;
        struct resource *resource;
};

Stuart

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson Oct. 1, 2013, 7:41 p.m. UTC | #4
On Tue, 2013-10-01 at 19:32 +0000, Yoder Stuart-B08248 wrote:
> > Antonios Motakis wrote
> > > Alex Williamson <alex.williamson@redhat.com> wrote:
> > > I notice all the open firmware calls here and I'm curious,
> > > will all platform devices be making use of open firmware?
> > > I don't know if this is synonymous with device tree or not.
> > > Thanks,
> >
> > This VFIO driver will support only devices implemented
> > on the device tree. While there can be platform devices
> > outside the device tree, I don't think it makes sense
> > to support them from the same driver. This is why I
> > originally called the driver VFIO_DT, however I renamed
> > it to VFIO_PLATFORM after feedback from the first
> > RFC. However personally, I still think the VFIO_DT name
> > is more appropriate since we don't support all platform
> > devices, only those that use the device tree.
> 
> But there is no 'device tree' bus.  The bus type we're
> dealing with is a platform bus.
> 
> vfio for platform devices should be independent of whether
> the device was discovered in a device tree or not.
> All you're doing is exposing mappable regions and IRQs
> to user space and it does not matter where the info originated.
> 
> You should be using platform bus structs here not
> reparsing device tree nodes.  The struct
> platform_device already has resource info in the 
> struct:
> 
> struct platform_device {
>         const char      *name;
>         u32             id;
>         struct device   dev;
>         u32             num_resources;
>         struct resource *resource;
> };

It seems likely(?) that platform devices could be described via ACPI at
some point, so keeping this abstraction would be a plus.  Thanks,

Alex 


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonios Motakis Oct. 2, 2013, 11:21 a.m. UTC | #5
On Tue, Oct 1, 2013 at 9:32 PM, Yoder Stuart-B08248
<B08248@freescale.com> wrote:
>> Antonios Motakis wrote
>> > Alex Williamson <alex.williamson@redhat.com> wrote:
>> > I notice all the open firmware calls here and I'm curious,
>> > will all platform devices be making use of open firmware?
>> > I don't know if this is synonymous with device tree or not.
>> > Thanks,
>>
>> This VFIO driver will support only devices implemented
>> on the device tree. While there can be platform devices
>> outside the device tree, I don't think it makes sense
>> to support them from the same driver. This is why I
>> originally called the driver VFIO_DT, however I renamed
>> it to VFIO_PLATFORM after feedback from the first
>> RFC. However personally, I still think the VFIO_DT name
>> is more appropriate since we don't support all platform
>> devices, only those that use the device tree.
>
> But there is no 'device tree' bus.  The bus type we're
> dealing with is a platform bus.
>
> vfio for platform devices should be independent of whether
> the device was discovered in a device tree or not.
> All you're doing is exposing mappable regions and IRQs
> to user space and it does not matter where the info originated.
>
> You should be using platform bus structs here not
> reparsing device tree nodes.  The struct
> platform_device already has resource info in the
> struct:
>
> struct platform_device {
>         const char      *name;
>         u32             id;
>         struct device   dev;
>         u32             num_resources;
>         struct resource *resource;
> };
>
> Stuart
>

I will look into this. However, can we rely to have access to all
device resources through platform abstractions, for every type of
platform device? It seems to me that platform devices that are not
backed by a specific description mechanism (such as device tree) may
include a lot of hard coded values etc in their drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf Oct. 2, 2013, 12:12 p.m. UTC | #6
On 02.10.2013, at 13:21, Antonios Motakis wrote:

> On Tue, Oct 1, 2013 at 9:32 PM, Yoder Stuart-B08248
> <B08248@freescale.com> wrote:
>>> Antonios Motakis wrote
>>>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>>> I notice all the open firmware calls here and I'm curious,
>>>> will all platform devices be making use of open firmware?
>>>> I don't know if this is synonymous with device tree or not.
>>>> Thanks,
>>> 
>>> This VFIO driver will support only devices implemented
>>> on the device tree. While there can be platform devices
>>> outside the device tree, I don't think it makes sense
>>> to support them from the same driver. This is why I
>>> originally called the driver VFIO_DT, however I renamed
>>> it to VFIO_PLATFORM after feedback from the first
>>> RFC. However personally, I still think the VFIO_DT name
>>> is more appropriate since we don't support all platform
>>> devices, only those that use the device tree.
>> 
>> But there is no 'device tree' bus.  The bus type we're
>> dealing with is a platform bus.
>> 
>> vfio for platform devices should be independent of whether
>> the device was discovered in a device tree or not.
>> All you're doing is exposing mappable regions and IRQs
>> to user space and it does not matter where the info originated.
>> 
>> You should be using platform bus structs here not
>> reparsing device tree nodes.  The struct
>> platform_device already has resource info in the
>> struct:
>> 
>> struct platform_device {
>>        const char      *name;
>>        u32             id;
>>        struct device   dev;
>>        u32             num_resources;
>>        struct resource *resource;
>> };
>> 
>> Stuart
>> 
> 
> I will look into this. However, can we rely to have access to all
> device resources through platform abstractions, for every type of
> platform device? It seems to me that platform devices that are not
> backed by a specific description mechanism (such as device tree) may
> include a lot of hard coded values etc in their drivers.

The same can be true for device tree based drivers. We should tell user space which device tree node this is based off of so it can go and find properties itself, similar to how a kernel driver can still read dt properties for a platform device. But this is optional - we could just as well give user space a hint on an acpi interface.

What exactly would you consider not abstracted information that the kernel driver would need to know about? All cases I can think of (reset logic, power constraints) are very device specific and would require a new vfio-dt-mydevice driver on top of your generic framework to handle these small device specific bits.


Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yoder Stuart-B08248 Oct. 2, 2013, 1:03 p.m. UTC | #7
> -----Original Message-----
> From: Antonios Motakis [mailto:a.motakis@virtualopensystems.com]
> Sent: Wednesday, October 02, 2013 6:22 AM
> To: Yoder Stuart-B08248
> Cc: Alex Williamson; kvm-arm; Linux IOMMU; Linux Samsung SOC; KVM devel
> mailing list; Alexander Graf; VirtualOpenSystems Technical Team
> Subject: Re: [PATCH 3/7] Return info for device and its memory regions
> and interrupts
> 
> On Tue, Oct 1, 2013 at 9:32 PM, Yoder Stuart-B08248
> <B08248@freescale.com> wrote:
> >> Antonios Motakis wrote
> >> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >> > I notice all the open firmware calls here and I'm curious,
> >> > will all platform devices be making use of open firmware?
> >> > I don't know if this is synonymous with device tree or not.
> >> > Thanks,
> >>
> >> This VFIO driver will support only devices implemented
> >> on the device tree. While there can be platform devices
> >> outside the device tree, I don't think it makes sense
> >> to support them from the same driver. This is why I
> >> originally called the driver VFIO_DT, however I renamed
> >> it to VFIO_PLATFORM after feedback from the first
> >> RFC. However personally, I still think the VFIO_DT name
> >> is more appropriate since we don't support all platform
> >> devices, only those that use the device tree.
> >
> > But there is no 'device tree' bus.  The bus type we're
> > dealing with is a platform bus.
> >
> > vfio for platform devices should be independent of whether
> > the device was discovered in a device tree or not.
> > All you're doing is exposing mappable regions and IRQs
> > to user space and it does not matter where the info originated.
> >
> > You should be using platform bus structs here not
> > reparsing device tree nodes.  The struct
> > platform_device already has resource info in the
> > struct:
> >
> > struct platform_device {
> >         const char      *name;
> >         u32             id;
> >         struct device   dev;
> >         u32             num_resources;
> >         struct resource *resource;
> > };
> >
> > Stuart
> >
> 
> I will look into this. However, can we rely to have access to all
> device resources through platform abstractions, for every type of
> platform device

The only resources we care about in vfio are mappable regions
and irqs.  So, yes I think we can rely on access to those
resources.

> It seems to me that platform devices that are not
> backed by a specific description mechanism (such as device tree) may
> include a lot of hard coded values etc in their drivers.

If the platform device struct does not have reg/irq resources described
then we can't expose them to user space with vfio.

Stuart

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonios Motakis Oct. 2, 2013, 1:14 p.m. UTC | #8
On Wed, Oct 2, 2013 at 3:03 PM, Yoder Stuart-B08248
<B08248@freescale.com> wrote:
>
>
>> -----Original Message-----
>> From: Antonios Motakis [mailto:a.motakis@virtualopensystems.com]
>> Sent: Wednesday, October 02, 2013 6:22 AM
>> To: Yoder Stuart-B08248
>> Cc: Alex Williamson; kvm-arm; Linux IOMMU; Linux Samsung SOC; KVM devel
>> mailing list; Alexander Graf; VirtualOpenSystems Technical Team
>> Subject: Re: [PATCH 3/7] Return info for device and its memory regions
>> and interrupts
>>
>> On Tue, Oct 1, 2013 at 9:32 PM, Yoder Stuart-B08248
>> <B08248@freescale.com> wrote:
>> >> Antonios Motakis wrote
>> >> > Alex Williamson <alex.williamson@redhat.com> wrote:
>> >> > I notice all the open firmware calls here and I'm curious,
>> >> > will all platform devices be making use of open firmware?
>> >> > I don't know if this is synonymous with device tree or not.
>> >> > Thanks,
>> >>
>> >> This VFIO driver will support only devices implemented
>> >> on the device tree. While there can be platform devices
>> >> outside the device tree, I don't think it makes sense
>> >> to support them from the same driver. This is why I
>> >> originally called the driver VFIO_DT, however I renamed
>> >> it to VFIO_PLATFORM after feedback from the first
>> >> RFC. However personally, I still think the VFIO_DT name
>> >> is more appropriate since we don't support all platform
>> >> devices, only those that use the device tree.
>> >
>> > But there is no 'device tree' bus.  The bus type we're
>> > dealing with is a platform bus.
>> >
>> > vfio for platform devices should be independent of whether
>> > the device was discovered in a device tree or not.
>> > All you're doing is exposing mappable regions and IRQs
>> > to user space and it does not matter where the info originated.
>> >
>> > You should be using platform bus structs here not
>> > reparsing device tree nodes.  The struct
>> > platform_device already has resource info in the
>> > struct:
>> >
>> > struct platform_device {
>> >         const char      *name;
>> >         u32             id;
>> >         struct device   dev;
>> >         u32             num_resources;
>> >         struct resource *resource;
>> > };
>> >
>> > Stuart
>> >
>>
>> I will look into this. However, can we rely to have access to all
>> device resources through platform abstractions, for every type of
>> platform device
>
> The only resources we care about in vfio are mappable regions
> and irqs.  So, yes I think we can rely on access to those
> resources.
>
>> It seems to me that platform devices that are not
>> backed by a specific description mechanism (such as device tree) may
>> include a lot of hard coded values etc in their drivers.
>
> If the platform device struct does not have reg/irq resources described
> then we can't expose them to user space with vfio.

Which is my concern actually. If the struct does not describe reg/irq
resources, or even worse if it describes them partially, do we want to
let the user shoot himself on the foot?

Granted this might happen with device tree too, but I find it much
more likely with generic platform devices.

Maybe we should adopt a whitelist instead?

>
> Stuart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stuart Yoder Nov. 7, 2013, 8:38 p.m. UTC | #9
>>> I will look into this. However, can we rely to have access to all
>>> device resources through platform abstractions, for every type of
>>> platform device
>>
>> The only resources we care about in vfio are mappable regions
>> and irqs.  So, yes I think we can rely on access to those
>> resources.
>>
>>> It seems to me that platform devices that are not
>>> backed by a specific description mechanism (such as device tree) may
>>> include a lot of hard coded values etc in their drivers.
>>
>> If the platform device struct does not have reg/irq resources described
>> then we can't expose them to user space with vfio.
>
> Which is my concern actually. If the struct does not describe reg/irq
> resources, or even worse if it describes them partially, do we want to
> let the user shoot himself on the foot?
>
> Granted this might happen with device tree too, but I find it much
> more likely with generic platform devices.
>
> Maybe we should adopt a whitelist instead?

Just realize I never replied to this query...

If there are no IRQs in the platform device struct we should
just return 0x0 for number of interrupts.
If the information in the platform device struct is only partially
complete, we should just return an error to the caller of the
ioctl.

Stuart
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/vfio/vfio_platform.c b/drivers/vfio/vfio_platform.c
index b9686b0..a0abcfa 100644
--- a/drivers/vfio/vfio_platform.c
+++ b/drivers/vfio/vfio_platform.c
@@ -28,6 +28,10 @@ 
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
 
 #define DRIVER_VERSION  "0.1"
 #define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
@@ -54,10 +58,13 @@  static long vfio_platform_ioctl(void *device_data,
 			   unsigned int cmd, unsigned long arg)
 {
 	struct vfio_platform_device *vdev = device_data;
+	struct device_node *of_node = vdev->pdev->dev.of_node;
 	unsigned long minsz;
 
 	if (cmd == VFIO_DEVICE_GET_INFO) {
 		struct vfio_device_info info;
+		struct resource res;
+		int cnt = 0;
 
 		minsz = offsetofend(struct vfio_device_info, num_irqs);
 
@@ -68,18 +75,57 @@  static long vfio_platform_ioctl(void *device_data,
 			return -EINVAL;
 
 		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
-		info.num_regions = 0;
-		info.num_irqs = 0;
+
+		while (!of_address_to_resource(of_node, cnt, &res))
+			cnt++;
+
+		info.num_regions = cnt;
+
+		info.num_irqs = of_irq_count(of_node);
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
-	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+		struct vfio_region_info info;
+		struct resource res;
 
-	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
-		return -EINVAL;
+		minsz = offsetofend(struct vfio_region_info, offset);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if(of_address_to_resource(of_node, info.index, &res))
+			return -EINVAL;
+
+		info.offset = res.start;	/* map phys addr with offset */
+		info.size = resource_size(&res);
+		info.flags = 0;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
+
+	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
+		struct vfio_irq_info info;
+		struct resource res;
+
+		minsz = offsetofend(struct vfio_irq_info, count);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		of_irq_to_resource(of_node, info.index, &res);
+
+		info.flags = 0;
+		info.count = 1;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
-	else if (cmd == VFIO_DEVICE_SET_IRQS)
+	} else if (cmd == VFIO_DEVICE_SET_IRQS)
 		return -EINVAL;
 
 	else if (cmd == VFIO_DEVICE_RESET)