[1/2] vfio/platform: make the vfio-platform device non abstract
diff mbox

Message ID 1487683100-16013-2-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Feb. 21, 2017, 1:18 p.m. UTC
Up to now the vfio-platform device has been abstract and could not be
instantiated. The integration of a new vfio platform device required
to create a dummy derived device which only set the compatibility
string.

Following the few vfio-platform device integration we have seen
the actual requested adaptation happens on device tree node creation
(sysbus-fdt).

So this patch removes the abstract setting and defines 2 new options,
manufacturer and model that are used to build a compatibility string.

This latter will be used to match the device tree node creation
function

sysbus-fdt does not support the instantiation of the vfio-platform
device yet.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/platform.c              | 18 ++++++++++++++++--
 include/hw/vfio/vfio-platform.h |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Sinan Kaya Feb. 21, 2017, 2:45 p.m. UTC | #1
Hi Eric,
Thanks for the long anticipated patch.

On 2/21/2017 8:18 AM, Eric Auger wrote:
> +    DEFINE_PROP_STRING("manufacturer", VFIOPlatformDevice, manufacturer),
> +    DEFINE_PROP_STRING("model", VFIOPlatformDevice, model),

The OF string is not always of the format "manufacturer, model".

HIDMA for instance uses a syntax of "qcom, hidma-1.0" and "qcom, hidma-2.0".
This syntax is used for cases where an IP gets used in multiple SOCs. 

Maybe, we should call it just compatible-string and try not to make meaning
out of it.

Sinan
Auger Eric Feb. 21, 2017, 2:59 p.m. UTC | #2
Hi Sinan,
On 21/02/2017 15:45, Sinan Kaya wrote:
> Hi Eric,
> Thanks for the long anticipated patch.
> 
> On 2/21/2017 8:18 AM, Eric Auger wrote:
>> +    DEFINE_PROP_STRING("manufacturer", VFIOPlatformDevice, manufacturer),
>> +    DEFINE_PROP_STRING("model", VFIOPlatformDevice, model),
> 
> The OF string is not always of the format "manufacturer, model".
> 
> HIDMA for instance uses a syntax of "qcom, hidma-1.0" and "qcom, hidma-2.0".
> This syntax is used for cases where an IP gets used in multiple SOCs. 
> 
> Maybe, we should call it just compatible-string and try not to make meaning
> out of it.
> 
> Sinan
> 

in  http://www.devicetree.org/specifications-pdf I found the description
at the bottom.

I your case what would prevent from creating 2 entries in the table and
then use either in qemu cmd line?

VFIO_PLATFORM_BINDING("qcom", "hidma-1.0", add_amd_hidma_fdt_node),
VFIO_PLATFORM_BINDING("qcom", "hidma-2.0", add_amd_hidma_fdt_node),

The issue is the comma in the property is interpreted as a separator for
options, hence that choice of having 2 separate options.

Besides I will fix the CONFIG_LINUX compilation issue asap.

Thanks

Eric

"
The recommended format is "manufacturer,model", where manufacturer is a
string describing the name of the manufacturer (such as a stock ticker
symbol), and model specifies the model number.

Example:
compatible = "fsl,mpc8641-uart", "ns16550";
In this example, an operating system would first try to locate a device
driver that supported
fsl,mpc8641-uart. If a driver was not found, it would then try to locate
a driver that supported the more general ns16550 device type.
"
Sinan Kaya Feb. 21, 2017, 3:10 p.m. UTC | #3
On 2/21/2017 9:59 AM, Auger Eric wrote:
> Hi Sinan,
> On 21/02/2017 15:45, Sinan Kaya wrote:
>> Hi Eric,
>> Thanks for the long anticipated patch.
>>
>> On 2/21/2017 8:18 AM, Eric Auger wrote:
>>> +    DEFINE_PROP_STRING("manufacturer", VFIOPlatformDevice, manufacturer),
>>> +    DEFINE_PROP_STRING("model", VFIOPlatformDevice, model),
>>
>> The OF string is not always of the format "manufacturer, model".
>>
>> HIDMA for instance uses a syntax of "qcom, hidma-1.0" and "qcom, hidma-2.0".
>> This syntax is used for cases where an IP gets used in multiple SOCs. 
>>
>> Maybe, we should call it just compatible-string and try not to make meaning
>> out of it.
>>
>> Sinan
>>
> 
> in  http://www.devicetree.org/specifications-pdf I found the description
> at the bottom.
> 
> I your case what would prevent from creating 2 entries in the table and
> then use either in qemu cmd line?
> 

I see your point. You are saying that hidma-1.0 would be the model and qcom
would be the manufacturer. 

This should work.

However, I have more concerns below.

> VFIO_PLATFORM_BINDING("qcom", "hidma-1.0", add_amd_hidma_fdt_node),
> VFIO_PLATFORM_BINDING("qcom", "hidma-2.0", add_amd_hidma_fdt_node),
> 
> The issue is the comma in the property is interpreted as a separator for
> options, hence that choice of having 2 separate options.

Note that the goal is to be able to instantiate any platform device by just
passing a compatible string and the object from the host. We don't want
to make changes to QEMU every time somebody comes up with a new platform device.
This step is unnecessary. 

A general purpose solution should be able to passthrough any platform device.

I have AHCI compatible SATA drive and HIDMA use cases for this purposes. The
code should be able to probe the given device with the compatible string without
requiring any changes to QEMU.

I didn't see this in the code. Maybe, it is just because I'm QEMU code
illiterate.

I shouldn't need to submit a patch to QEMU to add these two lines.

VFIO_PLATFORM_BINDING("qcom", "hidma-1.0", add_qcom_hidma_fdt_node),
VFIO_PLATFORM_BINDING("qcom", "hidma-2.0", add_qcom_hidma_fdt_node),


> 
> Besides I will fix the CONFIG_LINUX compilation issue asap.
> 
> Thanks
> 
> Eric
> 
> "
> The recommended format is "manufacturer,model", where manufacturer is a
> string describing the name of the manufacturer (such as a stock ticker
> symbol), and model specifies the model number.
> 
> Example:
> compatible = "fsl,mpc8641-uart", "ns16550";
> In this example, an operating system would first try to locate a device
> driver that supported
> fsl,mpc8641-uart. If a driver was not found, it would then try to locate
> a driver that supported the more general ns16550 device type.
> "
>
Auger Eric Feb. 21, 2017, 3:23 p.m. UTC | #4
Hi Sinan,

On 21/02/2017 16:10, Sinan Kaya wrote:
> On 2/21/2017 9:59 AM, Auger Eric wrote:
>> Hi Sinan,
>> On 21/02/2017 15:45, Sinan Kaya wrote:
>>> Hi Eric,
>>> Thanks for the long anticipated patch.
>>>
>>> On 2/21/2017 8:18 AM, Eric Auger wrote:
>>>> +    DEFINE_PROP_STRING("manufacturer", VFIOPlatformDevice, manufacturer),
>>>> +    DEFINE_PROP_STRING("model", VFIOPlatformDevice, model),
>>>
>>> The OF string is not always of the format "manufacturer, model".
>>>
>>> HIDMA for instance uses a syntax of "qcom, hidma-1.0" and "qcom, hidma-2.0".
>>> This syntax is used for cases where an IP gets used in multiple SOCs. 
>>>
>>> Maybe, we should call it just compatible-string and try not to make meaning
>>> out of it.
>>>
>>> Sinan
>>>
>>
>> in  http://www.devicetree.org/specifications-pdf I found the description
>> at the bottom.
>>
>> I your case what would prevent from creating 2 entries in the table and
>> then use either in qemu cmd line?
>>
> 
> I see your point. You are saying that hidma-1.0 would be the model and qcom
> would be the manufacturer. 
> 
> This should work.
> 
> However, I have more concerns below.
> 
>> VFIO_PLATFORM_BINDING("qcom", "hidma-1.0", add_amd_hidma_fdt_node),
>> VFIO_PLATFORM_BINDING("qcom", "hidma-2.0", add_amd_hidma_fdt_node),
>>
>> The issue is the comma in the property is interpreted as a separator for
>> options, hence that choice of having 2 separate options.
> 
> Note that the goal is to be able to instantiate any platform device by just
> passing a compatible string and the object from the host. We don't want
> to make changes to QEMU every time somebody comes up with a new platform device.
> This step is unnecessary.

Well we need to generate a device tree node for your device. We have not
proven yet the function that creates this latter can be generic. There
are some helpers that can read the host dt node, copy some properties
from the host to the guest. But still at the moment we can't make things
as generic as PCIe and this is why the platform passthrough still is
controversial. So this patch aims at lowering the integration efforts
but minimal specialization needs to be done in sysbus-fdt. Same on
kernel side with the reset module.

Thanks

Eric


> 
> A general purpose solution should be able to passthrough any platform device.
> 
> I have AHCI compatible SATA drive and HIDMA use cases for this purposes. The
> code should be able to probe the given device with the compatible string without
> requiring any changes to QEMU.
> 
> I didn't see this in the code. Maybe, it is just because I'm QEMU code
> illiterate.
> 
> I shouldn't need to submit a patch to QEMU to add these two lines.
> 
> VFIO_PLATFORM_BINDING("qcom", "hidma-1.0", add_qcom_hidma_fdt_node),
> VFIO_PLATFORM_BINDING("qcom", "hidma-2.0", add_qcom_hidma_fdt_node),
> 
> 
>>
>> Besides I will fix the CONFIG_LINUX compilation issue asap.
>>
>> Thanks
>>
>> Eric
>>
>> "
>> The recommended format is "manufacturer,model", where manufacturer is a
>> string describing the name of the manufacturer (such as a stock ticker
>> symbol), and model specifies the model number.
>>
>> Example:
>> compatible = "fsl,mpc8641-uart", "ns16550";
>> In this example, an operating system would first try to locate a device
>> driver that supported
>> fsl,mpc8641-uart. If a driver was not found, it would then try to locate
>> a driver that supported the more general ns16550 device type.
>> "
>>
> 
>
Sinan Kaya Feb. 21, 2017, 3:34 p.m. UTC | #5
On 2/21/2017 10:23 AM, Auger Eric wrote:
>> Note that the goal is to be able to instantiate any platform device by just
>> passing a compatible string and the object from the host. We don't want
>> to make changes to QEMU every time somebody comes up with a new platform device.
>> This step is unnecessary.
> Well we need to generate a device tree node for your device. We have not
> proven yet the function that creates this latter can be generic. There
> are some helpers that can read the host dt node, copy some properties
> from the host to the guest. But still at the moment we can't make things
> as generic as PCIe and this is why the platform passthrough still is
> controversial. So this patch aims at lowering the integration efforts
> but minimal specialization needs to be done in sysbus-fdt. Same on
> kernel side with the reset module.

Understood. You need to bring in the entire device tree parameters for a full-blown
solution. We can always treat this as a TODO.

I was more interested in simple devices like XHCI, AHCI, HIDMA where there
is no platform device attribute besides the interrupt number and memory
resources. QEMU already has access to these.

I was hoping to cover this at least. More complex objects need their own
implementation like the AMD driver in your example.

I'm pasting my comment from the archives here. 

https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03267.html

"Why submit a new SATA AHCI driver for QEMU just to set the compat string?"


> 
> Thanks
> 
> Eric
> 
>
Auger Eric Feb. 21, 2017, 9:04 p.m. UTC | #6
Hi Sinan,

On 21/02/2017 16:34, Sinan Kaya wrote:
> On 2/21/2017 10:23 AM, Auger Eric wrote:
>>> Note that the goal is to be able to instantiate any platform device by just
>>> passing a compatible string and the object from the host. We don't want
>>> to make changes to QEMU every time somebody comes up with a new platform device.
>>> This step is unnecessary.
>> Well we need to generate a device tree node for your device. We have not
>> proven yet the function that creates this latter can be generic. There
>> are some helpers that can read the host dt node, copy some properties
>> from the host to the guest. But still at the moment we can't make things
>> as generic as PCIe and this is why the platform passthrough still is
>> controversial. So this patch aims at lowering the integration efforts
>> but minimal specialization needs to be done in sysbus-fdt. Same on
>> kernel side with the reset module.
> 
> Understood. You need to bring in the entire device tree parameters for a full-blown
> solution. We can always treat this as a TODO.
> 
> I was more interested in simple devices like XHCI, AHCI, HIDMA where there
> is no platform device attribute besides the interrupt number and memory
> resources. QEMU already has access to these.
> 
> I was hoping to cover this at least. More complex objects need their own
> implementation like the AMD driver in your example.
> 
> I'm pasting my comment from the archives here. 
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03267.html
With respect to the compatible string we have 2 strategies: either
1) the user passes it along the -device option line or
2) qemu retrieves it in the host dt and applies the same on guest side.

I currently implemented 1)

Then assuming the above mentioned devices have very basic dt nodes
(compatible, reg, interrupts) we could imagine to copy the host property
into the guest device tree. But looking at

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/usb/usb-xhci.txt

there can be optional properties which are much more complex to handle
(clocks for instance).

We could potentially have a minimalist node creation function that
examines the host dt node and in case it only contains compat, reg,
interrupts (and maybe dma_coherent) copies those on guest side. In case
other properties exist and no specialized node creation function exists
qemu would abort.

Same for AHCI:

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/ata/ahci-platform.txt

HIDMA (https://patchwork.kernel.org/patch/8168281/) also seem to have
non trivial properties which would require a custom node creation function.

So I think this definitively deserves more discussions and agreements
upstream before starting more time consuming developments. This is not
for 2.9 anyway so I will move the series into an RFC.

Thanks

Eric
> 
> "Why submit a new SATA AHCI driver for QEMU just to set the compat string?"
> 
> 
>>
>> Thanks
>>
>> Eric
>>
>>
> 
>
Sinan Kaya Feb. 21, 2017, 9:57 p.m. UTC | #7
Hi Eric,

On 2/21/2017 4:04 PM, Auger Eric wrote:
> Hi Sinan,
> 
> On 21/02/2017 16:34, Sinan Kaya wrote:
>> On 2/21/2017 10:23 AM, Auger Eric wrote:
>>>> Note that the goal is to be able to instantiate any platform device by just
>>>> passing a compatible string and the object from the host. We don't want
>>>> to make changes to QEMU every time somebody comes up with a new platform device.
>>>> This step is unnecessary.
>>> Well we need to generate a device tree node for your device. We have not
>>> proven yet the function that creates this latter can be generic. There
>>> are some helpers that can read the host dt node, copy some properties
>>> from the host to the guest. But still at the moment we can't make things
>>> as generic as PCIe and this is why the platform passthrough still is
>>> controversial. So this patch aims at lowering the integration efforts
>>> but minimal specialization needs to be done in sysbus-fdt. Same on
>>> kernel side with the reset module.
>>
>> Understood. You need to bring in the entire device tree parameters for a full-blown
>> solution. We can always treat this as a TODO.
>>
>> I was more interested in simple devices like XHCI, AHCI, HIDMA where there
>> is no platform device attribute besides the interrupt number and memory
>> resources. QEMU already has access to these.
>>
>> I was hoping to cover this at least. More complex objects need their own
>> implementation like the AMD driver in your example.
>>
>> I'm pasting my comment from the archives here. 
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03267.html
> With respect to the compatible string we have 2 strategies: either
> 1) the user passes it along the -device option line or
> 2) qemu retrieves it in the host dt and applies the same on guest side.
> 
> I currently implemented 1)

That works.

> 
> Then assuming the above mentioned devices have very basic dt nodes
> (compatible, reg, interrupts) we could imagine to copy the host property
> into the guest device tree. But looking at

I like that idea. Just copy compatible, reg and interrupts. Don't take
anything else.

> 
> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/usb/usb-xhci.txt
> 
> there can be optional properties which are much more complex to handle
> (clocks for instance).

This is really what is needed to make USB working on a server. 

usb@f0931000 {
	compatible = "generic-xhci";
	reg = <0xf0931000 0x8c8>;
	interrupts = <0x0 0x4e 0x0>;
};

If an implementation needs clocks or anything else, then it is not supported
by what I'm proposing. That HW implementation needs their own driver.

> 
> We could potentially have a minimalist node creation function that
> examines the host dt node and in case it only contains compat, reg,
> interrupts (and maybe dma_coherent) copies those on guest side. In case
> other properties exist and no specialized node creation function exists
> qemu would abort.

Yes, this is also what I was suggesting.

> 
> Same for AHCI:
> 
> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/ata/ahci-platform.txt
> 

This is all we need for SATA on a server. 

	I0_SATA: sata@0xFF88000000 {
		compatible ="generic-ahci";
		reg = <0xFF 0x88000000 0x0 0x200>;
		interrupts = <0 (745-32) 0>;
		dma-coherent;
	}; 


> HIDMA (https://patchwork.kernel.org/patch/8168281/) also seem to have
> non trivial properties which would require a custom node creation function.
> 

HIDMA documentation is confusing due to multiple drivers (Management and the DMA
channel). We are only doing virtualization on a DMA channel. 

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt

This is all guest machine needs for HIDMA. The other parameters are optional
and can be passed via kernel command line.

 hidma_24: dma-controller@0x5c050000 {
	compatible = "qcom,hidma-1.0";
	reg = <0 0x5c050000 0x0 0x1000>,
		<0 0x5c0b0000 0x0 0x1000>;
	interrupts = <0 389 0>;
 };

> So I think this definitively deserves more discussions and agreements
> upstream before starting more time consuming developments. This is not
> for 2.9 anyway so I will move the series into an RFC.

Sure, I don't mind discussions. I'm focused on my server use cases where most
of the clocks/phys etc. are managed by the platform firmware. The platform 
devices are as simple as describing the name, interrupt and register base
addresses.

If somebody needs more than compatible, reg and interrupts; then they
definitely need their own driver. 

Sinan

Patch
diff mbox

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index a4663c9..73c0489 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -637,7 +637,20 @@  static void vfio_platform_realize(DeviceState *dev, Error **errp)
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
     SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
     VFIODevice *vbasedev = &vdev->vbasedev;
-    int i, ret;
+    int i, ret = -EINVAL;
+
+    if (!vdev->compat) {
+        if (!vdev->model) {
+            error_setg(errp, "no usable compatible string");
+            goto out;
+        }
+        if (!vdev->manufacturer) {
+            vdev->compat = g_strdup(vdev->model);
+        } else {
+            vdev->compat = g_strjoin(",", vdev->manufacturer,
+                                     vdev->model, NULL);
+        }
+    }
 
     vbasedev->type = VFIO_DEVICE_TYPE_PLATFORM;
     vbasedev->ops = &vfio_platform_ops;
@@ -678,6 +691,8 @@  static const VMStateDescription vfio_platform_vmstate = {
 static Property vfio_platform_dev_properties[] = {
     DEFINE_PROP_STRING("host", VFIOPlatformDevice, vbasedev.name),
     DEFINE_PROP_STRING("sysfsdev", VFIOPlatformDevice, vbasedev.sysfsdev),
+    DEFINE_PROP_STRING("manufacturer", VFIOPlatformDevice, manufacturer),
+    DEFINE_PROP_STRING("model", VFIOPlatformDevice, model),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPlatformDevice, vbasedev.no_mmap, false),
     DEFINE_PROP_UINT32("mmap-timeout-ms", VFIOPlatformDevice,
                        mmap_timeout, 1100),
@@ -704,7 +719,6 @@  static const TypeInfo vfio_platform_dev_info = {
     .instance_size = sizeof(VFIOPlatformDevice),
     .class_init = vfio_platform_class_init,
     .class_size = sizeof(VFIOPlatformDeviceClass),
-    .abstract   = true,
 };
 
 static void register_vfio_platform_dev_type(void)
diff --git a/include/hw/vfio/vfio-platform.h b/include/hw/vfio/vfio-platform.h
index 9baaa2db..31b9a98 100644
--- a/include/hw/vfio/vfio-platform.h
+++ b/include/hw/vfio/vfio-platform.h
@@ -55,6 +55,8 @@  typedef struct VFIOPlatformDevice {
     /* queue of pending IRQs */
     QSIMPLEQ_HEAD(pending_intp_queue, VFIOINTp) pending_intp_queue;
     char *compat; /* compatibility string */
+    char *manufacturer; /* manufacturer (1st part of the compatible property) */
+    char *model; /* model (2d part of the compatible property) */
     uint32_t mmap_timeout; /* delay to re-enable mmaps after interrupt */
     QEMUTimer *mmap_timer; /* allows fast-path resume after IRQ hit */
     QemuMutex intp_mutex; /* protect the intp_list IRQ state */