Re: [PATCH v14 08/11] virtio-iommu-pci: Introduce the x-dt-binding option

[Jean-Philippe Brucker]

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 <address@hidden>
> >> Suggested-by: Michael S. Tsirkin <address@hidden>
> > 
> > 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
> > 
>