Re: [PATCH v15 8/9] hw/arm/virt: Add the virtio-iommu device tree mappings

[Auger Eric]

Hi Peter, Michael,

On 2/11/20 6:31 PM, Auger Eric wrote:
> Hi Peter,
> 
> On 2/11/20 4:00 PM, Peter Maydell wrote:
>> On Sat, 8 Feb 2020 at 12:01, Eric Auger <address@hidden> wrote:
>>>
>>> Adds the "virtio,pci-iommu" node in the host bridge node and
>>> the RID mapping, excluding the IOMMU RID.
>>>
>>> This is done in the virtio-iommu-pci hotplug handler which
>>> gets called only if no firmware is loaded or if -no-acpi is
>>> passed on the command line. As non DT integration is
>>> not yet supported by the kernel we must make sure we
>>> are in DT mode. This limitation will be removed as soon
>>> as the topology description feature gets supported.
>>>
>>> Signed-off-by: Eric Auger <address@hidden>
>>>
>>> +static void create_virtio_iommu(VirtMachineState *vms, Error **errp)
>>> +{
>>> +    const char compat[] = "virtio,pci-iommu";
>>> +    uint16_t bdf = vms->virtio_iommu_bdf;
>>> +    char *node;
>>> +
>>> +    vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
>>> +
>>> +    node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, 
>>> bdf);
>>> +    qemu_fdt_add_subnode(vms->fdt, node);
>>> +    qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat));
>>> +    qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg",
>>> +                                 1, bdf << 8, 1, 0, 1, 0,
>>> +                                 1, 0, 1, 0);
>>> +
>>> +    qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1);
>>> +    qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle);
>>> +    g_free(node);
>>> +
>>> +    qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map",
>>> +                           0x0, vms->iommu_phandle, 0x0, bdf,
>>> +                           bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - 
>>> bdf);
>>> +}
>>
>> This function name implies that we're creating the IOMMU device
>> here (which would be a weird thing to do in a hotplug callback
>> for some other device), but it looks like we're only adding
>> device tree nodes ?
> yes the actual iommu device is created through the -device option. I can
> rename into create_iommu_dt_bindings
So I renamed it into create_virtio_iommu_dt_bindings()
>>
>> Given that we write the FDT blob into the guest RAM on bootup,
>> how does making changes to it here on hotplug (which I assume
>> to be 'after boot, whenever the user hot-plugs something') work?
> 
> the virtio-iommu is not supposed to be hotplugged but rather
> cold-plugged. I use this hotplug mechanism to detect its presence and
> add the related dt mappings. Maybe I can add a check to detect if the
> bootup is over?
I added in virtio-iommu-pci virtio_iommu_pci_class_init()
dc->hotpluggable = false;

As far as I understand this makes the virtio-iommu-pci device not
hotpluggable (same is used for intel-iommu):

(QEMU) device_add id=hot0 driver=virtio-iommu-pci bus=head.0 addr=4
{"error": {"class": "GenericError", "desc": "Parameter 'driver' expects
pluggable device type"}}

Is that OK?

Thanks

Eric

> 
> Thoughts?
> 
> Eric
>>
>> thanks
>> -- PMM
>>
> 
>