| Message ID | 20200207093203.3788-9-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
Hi Eric, On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > At the moment, the kernel only supports device tree > integration of the virtio-iommu. DT bindings between the > PCI root complex and the IOMMU must be created by the machine > in conformance to: > > Documentation/devicetree/bindings/virtio/iommu.txt. > > To make sure the end-user is aware of this, force him to use the > temporary device option "x-dt-binding" and also double check the > machine has a hotplug handler for the virtio-iommu-pci device. > This hotplug handler is in charge of creating those DT bindings. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Michael S. Tsirkin <mst@redhat.com> [...] > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > > + if (!dev->dt_binding) { > + error_setg(errp, > + "Instantiation currently only is possible if the machine " > + "creates device tree iommu-map bindings, ie. ACPI is not " > + "yet supported"); > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); "use -device virtio-iommu-pci,x-dt-binding"? Can the option be safely removed as soon as we implement a topology description for the remaining platforms? Or will we need to carry it forever for backward-compatibility (ie. ensure that an old command-line invocation that contains this option still works)? Thanks, Jean
Hi Jean, On 2/7/20 11:05 AM, Jean-Philippe Brucker wrote: > Hi Eric, > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: >> At the moment, the kernel only supports device tree >> integration of the virtio-iommu. DT bindings between the >> PCI root complex and the IOMMU must be created by the machine >> in conformance to: >> >> Documentation/devicetree/bindings/virtio/iommu.txt. >> >> To make sure the end-user is aware of this, force him to use the >> temporary device option "x-dt-binding" and also double check the >> machine has a hotplug handler for the virtio-iommu-pci device. >> This hotplug handler is in charge of creating those DT bindings. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> > [...] >> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); >> DeviceState *vdev = DEVICE(&dev->vdev); >> >> + if (!dev->dt_binding) { >> + error_setg(errp, >> + "Instantiation currently only is possible if the machine " >> + "creates device tree iommu-map bindings, ie. ACPI is not " >> + "yet supported"); >> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > > "use -device virtio-iommu-pci,x-dt-binding"? > > Can the option be safely removed as soon as we implement a topology > description for the remaining platforms? Or will we need to carry it > forever for backward-compatibility (ie. ensure that an old command-line > invocation that contains this option still works)? "x-" properties are experimental so yes it is bound to be removed as soon as we get the topology description feature. I now have to figure out how to fix the make-check issue it introduces :-( Thanks Eric > > Thanks, > Jean >
On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > At the moment, the kernel only supports device tree > integration of the virtio-iommu. DT bindings between the > PCI root complex and the IOMMU must be created by the machine > in conformance to: > > Documentation/devicetree/bindings/virtio/iommu.txt. > > To make sure the end-user is aware of this, force him to use the > temporary device option "x-dt-binding" and also double check the > machine has a hotplug handler for the virtio-iommu-pci device. > This hotplug handler is in charge of creating those DT bindings. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Michael S. Tsirkin <mst@redhat.com> how about setting it by default from machine class? See [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default which does it for spapr. > --- > > May be squashed with previous patch > --- > hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c > index d539fcce75..3d06e14000 100644 > --- a/hw/virtio/virtio-iommu-pci.c > +++ b/hw/virtio/virtio-iommu-pci.c > @@ -14,6 +14,7 @@ > #include "virtio-pci.h" > #include "hw/virtio/virtio-iommu.h" > #include "hw/qdev-properties.h" > +#include "qapi/error.h" > > typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > > @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > struct VirtIOIOMMUPCI { > VirtIOPCIProxy parent_obj; > VirtIOIOMMU vdev; > + bool dt_binding; > }; > > static Property virtio_iommu_pci_properties[] = { > DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), > + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > > + if (!dev->dt_binding) { > + error_setg(errp, > + "Instantiation currently only is possible if the machine " > + "creates device tree iommu-map bindings, ie. ACPI is not " > + "yet supported"); > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > + return; > + } > + > + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { > + error_setg(errp, > + "The machine does not implement a virtio-iommu-pci hotplug " > + " handler that creates the device tree iommu-map bindings"); > + return; > + } > qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > object_property_set_link(OBJECT(dev), > OBJECT(pci_get_bus(&vpci_dev->pci_dev)), > -- > 2.20.1
On Fri, Feb 07, 2020 at 11:05:40AM +0100, Jean-Philippe Brucker wrote: > Hi Eric, > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > > At the moment, the kernel only supports device tree > > integration of the virtio-iommu. DT bindings between the > > PCI root complex and the IOMMU must be created by the machine > > in conformance to: > > > > Documentation/devicetree/bindings/virtio/iommu.txt. > > > > To make sure the end-user is aware of this, force him to use the > > temporary device option "x-dt-binding" and also double check the > > machine has a hotplug handler for the virtio-iommu-pci device. > > This hotplug handler is in charge of creating those DT bindings. > > > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > [...] > > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > + if (!dev->dt_binding) { > > + error_setg(errp, > > + "Instantiation currently only is possible if the machine " > > + "creates device tree iommu-map bindings, ie. ACPI is not " > > + "yet supported"); > > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > > "use -device virtio-iommu-pci,x-dt-binding"? > > Can the option be safely removed as soon as we implement a topology > description for the remaining platforms? Or will we need to carry it > forever for backward-compatibility (ie. ensure that an old command-line > invocation that contains this option still works)? > > Thanks, > Jean I'd worry that if we actually document it then users will come to depend on it for sure, even though it starts with x-.
Hi On 2/7/20 11:24 AM, Michael S. Tsirkin wrote: > On Fri, Feb 07, 2020 at 11:05:40AM +0100, Jean-Philippe Brucker wrote: >> Hi Eric, >> >> On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: >>> At the moment, the kernel only supports device tree >>> integration of the virtio-iommu. DT bindings between the >>> PCI root complex and the IOMMU must be created by the machine >>> in conformance to: >>> >>> Documentation/devicetree/bindings/virtio/iommu.txt. >>> >>> To make sure the end-user is aware of this, force him to use the >>> temporary device option "x-dt-binding" and also double check the >>> machine has a hotplug handler for the virtio-iommu-pci device. >>> This hotplug handler is in charge of creating those DT bindings. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> [...] >>> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >>> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); >>> DeviceState *vdev = DEVICE(&dev->vdev); >>> >>> + if (!dev->dt_binding) { >>> + error_setg(errp, >>> + "Instantiation currently only is possible if the machine " >>> + "creates device tree iommu-map bindings, ie. ACPI is not " >>> + "yet supported"); >>> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); >> >> "use -device virtio-iommu-pci,x-dt-binding"? >> >> Can the option be safely removed as soon as we implement a topology >> description for the remaining platforms? Or will we need to carry it >> forever for backward-compatibility (ie. ensure that an old command-line >> invocation that contains this option still works)? >> >> Thanks, >> Jean > > I'd worry that if we actually document it then users will come to > depend on it for sure, even though it starts with x-. Let's rephrase my previous answer. Once we get the topology description feature we can leave the x-dt-binding property supported but do not test it anymore and document it at deprecated? Thanks Eric >
Hi, On 2/7/20 11:23 AM, Michael S. Tsirkin wrote: > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: >> At the moment, the kernel only supports device tree >> integration of the virtio-iommu. DT bindings between the >> PCI root complex and the IOMMU must be created by the machine >> in conformance to: >> >> Documentation/devicetree/bindings/virtio/iommu.txt. >> >> To make sure the end-user is aware of this, force him to use the >> temporary device option "x-dt-binding" and also double check the >> machine has a hotplug handler for the virtio-iommu-pci device. >> This hotplug handler is in charge of creating those DT bindings. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > how about setting it by default from machine class? Do you mean in ARM virt machine class? But this wouldn't prevent a user from launching an ACPI booted guest. I thought you wanted the end-user to know what he does. I don't figure out a way to know if the guest is booted in dt or acpi mode. I can get access to those info: - whether acpi is enabled - whether a FW is loaded But a FW can be loaded, acpi enabled and eventually the guest is DT booted with acpi=off in kernel opts. Maybe at this point I could only support the case where no FW is loaded. In machvirt I would not register the virtio-iommu-pci hotplug handler in case a FW is loaded. Then I could get rid of the new x-dt-binding prop. Thoughts? Eric > See > [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later > [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default > which does it for spapr. > >> --- >> >> May be squashed with previous patch >> --- >> hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c >> index d539fcce75..3d06e14000 100644 >> --- a/hw/virtio/virtio-iommu-pci.c >> +++ b/hw/virtio/virtio-iommu-pci.c >> @@ -14,6 +14,7 @@ >> #include "virtio-pci.h" >> #include "hw/virtio/virtio-iommu.h" >> #include "hw/qdev-properties.h" >> +#include "qapi/error.h" >> >> typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >> >> @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >> struct VirtIOIOMMUPCI { >> VirtIOPCIProxy parent_obj; >> VirtIOIOMMU vdev; >> + bool dt_binding; >> }; >> >> static Property virtio_iommu_pci_properties[] = { >> DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), >> + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); >> DeviceState *vdev = DEVICE(&dev->vdev); >> >> + if (!dev->dt_binding) { >> + error_setg(errp, >> + "Instantiation currently only is possible if the machine " >> + "creates device tree iommu-map bindings, ie. ACPI is not " >> + "yet supported"); >> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); >> + return; >> + } >> + >> + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { >> + error_setg(errp, >> + "The machine does not implement a virtio-iommu-pci hotplug " >> + " handler that creates the device tree iommu-map bindings"); >> + return; >> + } >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >> object_property_set_link(OBJECT(dev), >> OBJECT(pci_get_bus(&vpci_dev->pci_dev)), >> -- >> 2.20.1 >
On Fri, Feb 07, 2020 at 11:51:55AM +0100, Auger Eric wrote: > Hi, > > On 2/7/20 11:23 AM, Michael S. Tsirkin wrote: > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > >> At the moment, the kernel only supports device tree > >> integration of the virtio-iommu. DT bindings between the > >> PCI root complex and the IOMMU must be created by the machine > >> in conformance to: > >> > >> Documentation/devicetree/bindings/virtio/iommu.txt. > >> > >> To make sure the end-user is aware of this, force him to use the > >> temporary device option "x-dt-binding" and also double check the > >> machine has a hotplug handler for the virtio-iommu-pci device. > >> This hotplug handler is in charge of creating those DT bindings. > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > > how about setting it by default from machine class? > Do you mean in ARM virt machine class? But this wouldn't prevent a user > from launching an ACPI booted guest. I thought you wanted the end-user > to know what he does. > > I don't figure out a way to know if the guest is booted in dt or acpi > mode. I can get access to those info: > - whether acpi is enabled That's the default on virt machine right? Then if there is a FW it can choose either ACPI or DT? > - whether a FW is loaded > > But a FW can be loaded, acpi enabled and eventually the guest is DT > booted with acpi=off in kernel opts. > > Maybe at this point I could only support the case where no FW is loaded. > In machvirt I would not register the virtio-iommu-pci hotplug handler in > case a FW is loaded. Then I could get rid of the new x-dt-binding prop. > > Thoughts? Yes, I'm hoping we can get the topology description into next version of Linux, and it would be nicer not to introduce backward-compatible baggage for something that should be solved within a few months. If we have to warn the user then checking the FW seems like a good compromise, and easy to remove later. Thanks, Jean > > Eric > > See > > [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later > > [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default > > which does it for spapr. > > > > >> --- > >> > >> May be squashed with previous patch > >> --- > >> hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c > >> index d539fcce75..3d06e14000 100644 > >> --- a/hw/virtio/virtio-iommu-pci.c > >> +++ b/hw/virtio/virtio-iommu-pci.c > >> @@ -14,6 +14,7 @@ > >> #include "virtio-pci.h" > >> #include "hw/virtio/virtio-iommu.h" > >> #include "hw/qdev-properties.h" > >> +#include "qapi/error.h" > >> > >> typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > >> > >> @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > >> struct VirtIOIOMMUPCI { > >> VirtIOPCIProxy parent_obj; > >> VirtIOIOMMU vdev; > >> + bool dt_binding; > >> }; > >> > >> static Property virtio_iommu_pci_properties[] = { > >> DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), > >> + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > >> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > >> DeviceState *vdev = DEVICE(&dev->vdev); > >> > >> + if (!dev->dt_binding) { > >> + error_setg(errp, > >> + "Instantiation currently only is possible if the machine " > >> + "creates device tree iommu-map bindings, ie. ACPI is not " > >> + "yet supported"); > >> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > >> + return; > >> + } > >> + > >> + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { > >> + error_setg(errp, > >> + "The machine does not implement a virtio-iommu-pci hotplug " > >> + " handler that creates the device tree iommu-map bindings"); > >> + return; > >> + } > >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > >> object_property_set_link(OBJECT(dev), > >> OBJECT(pci_get_bus(&vpci_dev->pci_dev)), > >> -- > >> 2.20.1 > > >
On Fri, Feb 07, 2020 at 11:51:55AM +0100, Auger Eric wrote: > Hi, > > On 2/7/20 11:23 AM, Michael S. Tsirkin wrote: > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > >> At the moment, the kernel only supports device tree > >> integration of the virtio-iommu. DT bindings between the > >> PCI root complex and the IOMMU must be created by the machine > >> in conformance to: > >> > >> Documentation/devicetree/bindings/virtio/iommu.txt. > >> > >> To make sure the end-user is aware of this, force him to use the > >> temporary device option "x-dt-binding" and also double check the > >> machine has a hotplug handler for the virtio-iommu-pci device. > >> This hotplug handler is in charge of creating those DT bindings. > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > > how about setting it by default from machine class? > Do you mean in ARM virt machine class? But this wouldn't prevent a user > from launching an ACPI booted guest. I thought you wanted the end-user > to know what he does. > > I don't figure out a way to know if the guest is booted in dt or acpi > mode. I can get access to those info: > - whether acpi is enabled > - whether a FW is loaded > > But a FW can be loaded, acpi enabled and eventually the guest is DT > booted with acpi=off in kernel opts. So let's say this configuration does not support the virtio-iommu at the moment. Is that a big deal? > Maybe at this point I could only support the case where no FW is loaded. > In machvirt I would not register the virtio-iommu-pci hotplug handler in > case a FW is loaded. Then I could get rid of the new x-dt-binding prop. > > Thoughts? > > Eric Also an option. > > See > > [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later > > [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default > > which does it for spapr. > > > > >> --- > >> > >> May be squashed with previous patch > >> --- > >> hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c > >> index d539fcce75..3d06e14000 100644 > >> --- a/hw/virtio/virtio-iommu-pci.c > >> +++ b/hw/virtio/virtio-iommu-pci.c > >> @@ -14,6 +14,7 @@ > >> #include "virtio-pci.h" > >> #include "hw/virtio/virtio-iommu.h" > >> #include "hw/qdev-properties.h" > >> +#include "qapi/error.h" > >> > >> typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > >> > >> @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; > >> struct VirtIOIOMMUPCI { > >> VirtIOPCIProxy parent_obj; > >> VirtIOIOMMU vdev; > >> + bool dt_binding; > >> }; > >> > >> static Property virtio_iommu_pci_properties[] = { > >> DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), > >> + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > >> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > >> DeviceState *vdev = DEVICE(&dev->vdev); > >> > >> + if (!dev->dt_binding) { > >> + error_setg(errp, > >> + "Instantiation currently only is possible if the machine " > >> + "creates device tree iommu-map bindings, ie. ACPI is not " > >> + "yet supported"); > >> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > >> + return; > >> + } > >> + > >> + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { > >> + error_setg(errp, > >> + "The machine does not implement a virtio-iommu-pci hotplug " > >> + " handler that creates the device tree iommu-map bindings"); > >> + return; > >> + } > >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > >> object_property_set_link(OBJECT(dev), > >> OBJECT(pci_get_bus(&vpci_dev->pci_dev)), > >> -- > >> 2.20.1 > >
Hi Jean, On 2/7/20 12:15 PM, Jean-Philippe Brucker wrote: > On Fri, Feb 07, 2020 at 11:51:55AM +0100, Auger Eric wrote: >> Hi, >> >> On 2/7/20 11:23 AM, Michael S. Tsirkin wrote: >>> On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: >>>> At the moment, the kernel only supports device tree >>>> integration of the virtio-iommu. DT bindings between the >>>> PCI root complex and the IOMMU must be created by the machine >>>> in conformance to: >>>> >>>> Documentation/devicetree/bindings/virtio/iommu.txt. >>>> >>>> To make sure the end-user is aware of this, force him to use the >>>> temporary device option "x-dt-binding" and also double check the >>>> machine has a hotplug handler for the virtio-iommu-pci device. >>>> This hotplug handler is in charge of creating those DT bindings. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> how about setting it by default from machine class? >> Do you mean in ARM virt machine class? But this wouldn't prevent a user >> from launching an ACPI booted guest. I thought you wanted the end-user >> to know what he does. >> >> I don't figure out a way to know if the guest is booted in dt or acpi >> mode. I can get access to those info: >> - whether acpi is enabled > > That's the default on virt machine right? I think that's the default for qemu. you can siwtch it off on the cmd line using "-no-acpi" Then if there is a FW it can > choose either ACPI or DT? yes that's why I can't actually know which one is going to be used. > >> - whether a FW is loaded In case there is no FW I am sure this is dt mode. So I would be inclined to only support that mode atm. >> >> But a FW can be loaded, acpi enabled and eventually the guest is DT >> booted with acpi=off in kernel opts. >> >> Maybe at this point I could only support the case where no FW is loaded. >> In machvirt I would not register the virtio-iommu-pci hotplug handler in >> case a FW is loaded. Then I could get rid of the new x-dt-binding prop. >> >> Thoughts? > > Yes, I'm hoping we can get the topology description into next version of > Linux, and it would be nicer not to introduce backward-compatible baggage > for something that should be solved within a few month I agree. So I am inclined to remove the x-dt-binding prop then I just expose allow the virtio-iommu-pci to be instantiated if no FW is loaded. The constraint will be removed when we get topology description support in kernel. Thanks Eric If we have to > warn the user then checking the FW seems like a good compromise, and easy > to remove later. > > Thanks, > Jean > >> >> Eric >>> See >>> [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later >>> [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default >>> which does it for spapr. >> >>> >>>> --- >>>> >>>> May be squashed with previous patch >>>> --- >>>> hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c >>>> index d539fcce75..3d06e14000 100644 >>>> --- a/hw/virtio/virtio-iommu-pci.c >>>> +++ b/hw/virtio/virtio-iommu-pci.c >>>> @@ -14,6 +14,7 @@ >>>> #include "virtio-pci.h" >>>> #include "hw/virtio/virtio-iommu.h" >>>> #include "hw/qdev-properties.h" >>>> +#include "qapi/error.h" >>>> >>>> typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >>>> >>>> @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >>>> struct VirtIOIOMMUPCI { >>>> VirtIOPCIProxy parent_obj; >>>> VirtIOIOMMU vdev; >>>> + bool dt_binding; >>>> }; >>>> >>>> static Property virtio_iommu_pci_properties[] = { >>>> DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), >>>> + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> >>>> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >>>> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); >>>> DeviceState *vdev = DEVICE(&dev->vdev); >>>> >>>> + if (!dev->dt_binding) { >>>> + error_setg(errp, >>>> + "Instantiation currently only is possible if the machine " >>>> + "creates device tree iommu-map bindings, ie. ACPI is not " >>>> + "yet supported"); >>>> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); >>>> + return; >>>> + } >>>> + >>>> + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { >>>> + error_setg(errp, >>>> + "The machine does not implement a virtio-iommu-pci hotplug " >>>> + " handler that creates the device tree iommu-map bindings"); >>>> + return; >>>> + } >>>> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >>>> object_property_set_link(OBJECT(dev), >>>> OBJECT(pci_get_bus(&vpci_dev->pci_dev)), >>>> -- >>>> 2.20.1 >>> >> >
Hi Michael, On 2/7/20 1:00 PM, Michael S. Tsirkin wrote: > On Fri, Feb 07, 2020 at 11:51:55AM +0100, Auger Eric wrote: >> Hi, >> >> On 2/7/20 11:23 AM, Michael S. Tsirkin wrote: >>> On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: >>>> At the moment, the kernel only supports device tree >>>> integration of the virtio-iommu. DT bindings between the >>>> PCI root complex and the IOMMU must be created by the machine >>>> in conformance to: >>>> >>>> Documentation/devicetree/bindings/virtio/iommu.txt. >>>> >>>> To make sure the end-user is aware of this, force him to use the >>>> temporary device option "x-dt-binding" and also double check the >>>> machine has a hotplug handler for the virtio-iommu-pci device. >>>> This hotplug handler is in charge of creating those DT bindings. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> how about setting it by default from machine class? >> Do you mean in ARM virt machine class? But this wouldn't prevent a user >> from launching an ACPI booted guest. I thought you wanted the end-user >> to know what he does. >> >> I don't figure out a way to know if the guest is booted in dt or acpi >> mode. I can get access to those info: >> - whether acpi is enabled >> - whether a FW is loaded >> >> But a FW can be loaded, acpi enabled and eventually the guest is DT >> booted with acpi=off in kernel opts. > > So let's say this configuration does not support the virtio-iommu > at the moment. Is that a big deal? > >> Maybe at this point I could only support the case where no FW is loaded. >> In machvirt I would not register the virtio-iommu-pci hotplug handler in >> case a FW is loaded. Then I could get rid of the new x-dt-binding prop. >> >> Thoughts? >> >> Eric > > Also an option. OK so I will drop the x-dt-binding property and implement this option. Thanks Eric > >>> See >>> [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later >>> [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default >>> which does it for spapr. >> >>> >>>> --- >>>> >>>> May be squashed with previous patch >>>> --- >>>> hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c >>>> index d539fcce75..3d06e14000 100644 >>>> --- a/hw/virtio/virtio-iommu-pci.c >>>> +++ b/hw/virtio/virtio-iommu-pci.c >>>> @@ -14,6 +14,7 @@ >>>> #include "virtio-pci.h" >>>> #include "hw/virtio/virtio-iommu.h" >>>> #include "hw/qdev-properties.h" >>>> +#include "qapi/error.h" >>>> >>>> typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >>>> >>>> @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; >>>> struct VirtIOIOMMUPCI { >>>> VirtIOPCIProxy parent_obj; >>>> VirtIOIOMMU vdev; >>>> + bool dt_binding; >>>> }; >>>> >>>> static Property virtio_iommu_pci_properties[] = { >>>> DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), >>>> + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> >>>> @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >>>> VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); >>>> DeviceState *vdev = DEVICE(&dev->vdev); >>>> >>>> + if (!dev->dt_binding) { >>>> + error_setg(errp, >>>> + "Instantiation currently only is possible if the machine " >>>> + "creates device tree iommu-map bindings, ie. ACPI is not " >>>> + "yet supported"); >>>> + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); >>>> + return; >>>> + } >>>> + >>>> + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { >>>> + error_setg(errp, >>>> + "The machine does not implement a virtio-iommu-pci hotplug " >>>> + " handler that creates the device tree iommu-map bindings"); >>>> + return; >>>> + } >>>> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >>>> object_property_set_link(OBJECT(dev), >>>> OBJECT(pci_get_bus(&vpci_dev->pci_dev)), >>>> -- >>>> 2.20.1 >>> > >
On Fri, Feb 07, 2020 at 05:24:54AM -0500, Michael S. Tsirkin wrote: > On Fri, Feb 07, 2020 at 11:05:40AM +0100, Jean-Philippe Brucker wrote: > > Hi Eric, > > > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > > > At the moment, the kernel only supports device tree > > > integration of the virtio-iommu. DT bindings between the > > > PCI root complex and the IOMMU must be created by the machine > > > in conformance to: > > > > > > Documentation/devicetree/bindings/virtio/iommu.txt. > > > > > > To make sure the end-user is aware of this, force him to use the > > > temporary device option "x-dt-binding" and also double check the > > > machine has a hotplug handler for the virtio-iommu-pci device. > > > This hotplug handler is in charge of creating those DT bindings. > > > > > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > [...] > > > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > + if (!dev->dt_binding) { > > > + error_setg(errp, > > > + "Instantiation currently only is possible if the machine " > > > + "creates device tree iommu-map bindings, ie. ACPI is not " > > > + "yet supported"); > > > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > > > > "use -device virtio-iommu-pci,x-dt-binding"? > > > > Can the option be safely removed as soon as we implement a topology > > description for the remaining platforms? Or will we need to carry it > > forever for backward-compatibility (ie. ensure that an old command-line > > invocation that contains this option still works)? > > > > Thanks, > > Jean > > I'd worry that if we actually document it then users will come to > depend on it for sure, even though it starts with x-. I thought x- parameters can be dropped directly with totally no grarantee... Otherwise how do we differenciate x- with the common parameters, and how do we introduce remove-prone parameters? Thanks,
On Fri, Feb 07, 2020 at 06:04:05PM -0500, Peter Xu wrote: > On Fri, Feb 07, 2020 at 05:24:54AM -0500, Michael S. Tsirkin wrote: > > On Fri, Feb 07, 2020 at 11:05:40AM +0100, Jean-Philippe Brucker wrote: > > > Hi Eric, > > > > > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > > > > At the moment, the kernel only supports device tree > > > > integration of the virtio-iommu. DT bindings between the > > > > PCI root complex and the IOMMU must be created by the machine > > > > in conformance to: > > > > > > > > Documentation/devicetree/bindings/virtio/iommu.txt. > > > > > > > > To make sure the end-user is aware of this, force him to use the > > > > temporary device option "x-dt-binding" and also double check the > > > > machine has a hotplug handler for the virtio-iommu-pci device. > > > > This hotplug handler is in charge of creating those DT bindings. > > > > > > > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > [...] > > > > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > > > + if (!dev->dt_binding) { > > > > + error_setg(errp, > > > > + "Instantiation currently only is possible if the machine " > > > > + "creates device tree iommu-map bindings, ie. ACPI is not " > > > > + "yet supported"); > > > > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > > > > > > "use -device virtio-iommu-pci,x-dt-binding"? > > > > > > Can the option be safely removed as soon as we implement a topology > > > description for the remaining platforms? Or will we need to carry it > > > forever for backward-compatibility (ie. ensure that an old command-line > > > invocation that contains this option still works)? > > > > > > Thanks, > > > Jean > > > > I'd worry that if we actually document it then users will come to > > depend on it for sure, even though it starts with x-. > > I thought x- parameters can be dropped directly with totally no > grarantee... Otherwise how do we differenciate x- with the common > parameters, and how do we introduce remove-prone parameters? > > Thanks, It's all about not breaking users. Yes we document that x- interfaces are unstable. But that documentation is only good for well-behaved users such as libvirt. End-users tend not to read the docs and the subtleties of stable/unstable interface are lost on them, so we really must never actively ask end users to set an x- flag. > -- > Peter Xu
On Sun, Feb 09, 2020 at 03:58:57PM -0500, Michael S. Tsirkin wrote: > On Fri, Feb 07, 2020 at 06:04:05PM -0500, Peter Xu wrote: > > On Fri, Feb 07, 2020 at 05:24:54AM -0500, Michael S. Tsirkin wrote: > > > On Fri, Feb 07, 2020 at 11:05:40AM +0100, Jean-Philippe Brucker wrote: > > > > Hi Eric, > > > > > > > > On Fri, Feb 07, 2020 at 10:32:00AM +0100, Eric Auger wrote: > > > > > At the moment, the kernel only supports device tree > > > > > integration of the virtio-iommu. DT bindings between the > > > > > PCI root complex and the IOMMU must be created by the machine > > > > > in conformance to: > > > > > > > > > > Documentation/devicetree/bindings/virtio/iommu.txt. > > > > > > > > > > To make sure the end-user is aware of this, force him to use the > > > > > temporary device option "x-dt-binding" and also double check the > > > > > machine has a hotplug handler for the virtio-iommu-pci device. > > > > > This hotplug handler is in charge of creating those DT bindings. > > > > > > > > > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > > [...] > > > > > @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > > > VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); > > > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > > > > > + if (!dev->dt_binding) { > > > > > + error_setg(errp, > > > > > + "Instantiation currently only is possible if the machine " > > > > > + "creates device tree iommu-map bindings, ie. ACPI is not " > > > > > + "yet supported"); > > > > > + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); > > > > > > > > "use -device virtio-iommu-pci,x-dt-binding"? > > > > > > > > Can the option be safely removed as soon as we implement a topology > > > > description for the remaining platforms? Or will we need to carry it > > > > forever for backward-compatibility (ie. ensure that an old command-line > > > > invocation that contains this option still works)? > > > > > > > > Thanks, > > > > Jean > > > > > > I'd worry that if we actually document it then users will come to > > > depend on it for sure, even though it starts with x-. > > > > I thought x- parameters can be dropped directly with totally no > > grarantee... Otherwise how do we differenciate x- with the common > > parameters, and how do we introduce remove-prone parameters? > > > > Thanks, > > It's all about not breaking users. Yes we document that x- > interfaces are unstable. But that documentation is only > good for well-behaved users such as libvirt. End-users > tend not to read the docs and the subtleties of > stable/unstable interface are lost on them, so we really must never > actively ask end users to set an x- flag. Ah I see the point. Yes we should never suggest that, or at least also mention that it's experimental and prone to change. Thanks,
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c index d539fcce75..3d06e14000 100644 --- a/hw/virtio/virtio-iommu-pci.c +++ b/hw/virtio/virtio-iommu-pci.c @@ -14,6 +14,7 @@ #include "virtio-pci.h" #include "hw/virtio/virtio-iommu.h" #include "hw/qdev-properties.h" +#include "qapi/error.h" typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; @@ -27,10 +28,12 @@ typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI; struct VirtIOIOMMUPCI { VirtIOPCIProxy parent_obj; VirtIOIOMMU vdev; + bool dt_binding; }; static Property virtio_iommu_pci_properties[] = { DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0), + DEFINE_PROP_BOOL("x-dt-binding", VirtIOIOMMUPCI, dt_binding, false), DEFINE_PROP_END_OF_LIST(), }; @@ -39,6 +42,21 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); + if (!dev->dt_binding) { + error_setg(errp, + "Instantiation currently only is possible if the machine " + "creates device tree iommu-map bindings, ie. ACPI is not " + "yet supported"); + error_append_hint(errp, "use -virtio-iommu-pci,x-dt-binding\n"); + return; + } + + if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) { + error_setg(errp, + "The machine does not implement a virtio-iommu-pci hotplug " + " handler that creates the device tree iommu-map bindings"); + return; + } qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); object_property_set_link(OBJECT(dev), OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
At the moment, the kernel only supports device tree integration of the virtio-iommu. DT bindings between the PCI root complex and the IOMMU must be created by the machine in conformance to: Documentation/devicetree/bindings/virtio/iommu.txt. To make sure the end-user is aware of this, force him to use the temporary device option "x-dt-binding" and also double check the machine has a hotplug handler for the virtio-iommu-pci device. This hotplug handler is in charge of creating those DT bindings. Signed-off-by: Eric Auger <eric.auger@redhat.com> Suggested-by: Michael S. Tsirkin <mst@redhat.com> --- May be squashed with previous patch --- hw/virtio/virtio-iommu-pci.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)