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.au...@redhat.com>
>> Suggested-by: Michael S. Tsirkin <m...@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
>
