Re: [Qemu-devel] [PATCH v7 15/20] hw/arm/sysbus-fdt: Pass the VirtMachineState to the node creation functions

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

Hi Peter,

On 09/10/2017 19:47, Peter Maydell wrote:
> On 1 September 2017 at 18:21, Eric Auger <eric.auger@redhat.com> wrote:
>> The VirtMachineState contains some dt phandles that will be used
>> in some node creation functions. For instance we plan to use the
>> PCI host controller phandle in the smmu node creation function. So
>> let's pass the VirtMachineState handle down to the node creation
>> functions by enhancing the involved datatypes.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/sysbus-fdt.c         | 3 +++
>>  hw/arm/virt.c               | 1 +
>>  include/hw/arm/sysbus-fdt.h | 2 ++
>>  3 files changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index d68e3dc..d92a983 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -36,6 +36,7 @@
>>  #include "hw/vfio/vfio-platform.h"
>>  #include "hw/vfio/vfio-calxeda-xgmac.h"
>>  #include "hw/vfio/vfio-amd-xgbe.h"
>> +#include "hw/arm/virt.h"
>>  #include "hw/arm/fdt.h"
>>
>>  /*
>> @@ -47,6 +48,7 @@ typedef struct PlatformBusFDTData {
>>      int irq_start; /* index of the first IRQ usable by platform bus devices */
>>      const char *pbus_node_name; /* name of the platform bus node */
>>      PlatformBusDevice *pbus;
>> +    VirtMachineState *vms;
>>  } PlatformBusFDTData;
> 
> sysbus-fdt isn't virt specific, so this doesn't belong here.
Correct. I plan to replace this by MachineState instead.
> 
> More generally, why is sysbus-fdt involved in this at all?
> I expected that instantiating and wiring up the SMMU would
> be the job of hw/arm/virt.c, like any other device we
> might have on the board.
I wished to have the same type of option as for x86 where
"-device intel-iommu" is passed to the QEMU command line. smmuv3 device
being a SysBusDevice, a natural framework to handle its node creation
function is sysbus-fdt. Having a -device approach is practical to pass
other options to the device (this was typically the case for the
"caching-mode" option). On Intel there are caching-mode, passthrough
(pt) options.

Although the smmu caching-mode option may not survive in this form, we
can foresee other options to come. In the future we may pass the PCI bus
number we want to plug the smmu into.

Also the invocation method would be identical for virtio-iommu.

I explored the other (and simpler) alternative of passing an option to
virt machine but I think this approach is less flexible. Also as you
pointed out, by default, smmu would be on, if we stick to the existing
convention. Given the perf overhead I don't think this is what we want.

Thanks

Eric
> 
> thanks
> -- PMM
>