| Message ID | 1487683100-16013-2-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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. "
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. > " >
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. >> " >> > >
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 > >
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 >> >> > >
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
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 */
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(-)