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

[Auger Eric <eric.auger@redhat.com>]

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
>