Re: [PATCH 11/11] kvmtool: add command line parameter to instantiate a vGICv3

[Marc Zyngier <marc.zyngier@arm.com>]

On 26/01/15 11:43, Andre Przywara wrote:
> Hi Will,
> 
> On 26/01/15 11:30, Will Deacon wrote:
>> On Fri, Jan 23, 2015 at 04:35:10PM +0000, Andre Przywara wrote:
>>> Add the command line parameter "--gicv3" to request GICv3 emulation
>>> in the kernel. Connect that to the already existing GICv3 code.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  tools/kvm/arm/aarch64/arm-cpu.c                    |    5 ++++-
>>>  .../kvm/arm/aarch64/include/kvm/kvm-config-arch.h  |    4 +++-
>>>  tools/kvm/arm/gic.c                                |   14 ++++++++++++++
>>>  tools/kvm/arm/include/arm-common/kvm-config-arch.h |    1 +
>>>  tools/kvm/arm/kvm-cpu.c                            |    2 +-
>>>  tools/kvm/arm/kvm.c                                |    3 ++-
>>>  6 files changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/kvm/arm/aarch64/arm-cpu.c b/tools/kvm/arm/aarch64/arm-cpu.c
>>> index a70d6bb..46d6d6a 100644
>>> --- a/tools/kvm/arm/aarch64/arm-cpu.c
>>> +++ b/tools/kvm/arm/aarch64/arm-cpu.c
>>> @@ -12,7 +12,10 @@
>>>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>>>  {
>>>  	int timer_interrupts[4] = {13, 14, 11, 10};
>>> -	gic__generate_fdt_nodes(fdt, gic_phandle, KVM_DEV_TYPE_ARM_VGIC_V2);
>>> +	gic__generate_fdt_nodes(fdt, gic_phandle,
>>> +				kvm->cfg.arch.gicv3 ?
>>> +					KVM_DEV_TYPE_ARM_VGIC_V3 :
>>> +					KVM_DEV_TYPE_ARM_VGIC_V2);
>>>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>>>  }
>>>  
>>> diff --git a/tools/kvm/arm/aarch64/include/kvm/kvm-config-arch.h b/tools/kvm/arm/aarch64/include/kvm/kvm-config-arch.h
>>> index 89860ae..106e52f 100644
>>> --- a/tools/kvm/arm/aarch64/include/kvm/kvm-config-arch.h
>>> +++ b/tools/kvm/arm/aarch64/include/kvm/kvm-config-arch.h
>>> @@ -3,7 +3,9 @@
>>>  
>>>  #define ARM_OPT_ARCH_RUN(cfg)						\
>>>  	OPT_BOOLEAN('\0', "aarch32", &(cfg)->aarch32_guest,		\
>>> -			"Run AArch32 guest"),
>>> +			"Run AArch32 guest"),				\
>>> +	OPT_BOOLEAN('\0', "gicv3", &(cfg)->gicv3,			\
>>> +			"Use a GICv3 interrupt controller in the guest"),
>>
>> On a GICv3-capable system, why would I *not* want to enable this option?
>> In other words, could we make this the default behaviour on systems that
>> support it, and if you need an override then it should be something like
>> --force-gicv2.
> 
> Well, you could have a guest kernel < 3.17, which does not have GICv3
> support. In general I consider GICv2 better tested, so I reckon that
> people will only want to use GICv3 emulation if there is a need for it
> (non-compat GICv3 host or more than 8 VCPUs in the guest). That probably
> changes over time, but for the time being I'd better keep the default at
> GICv2 emulation.

I think there is slightly more to it. You want the same command-line
options to give you the same result on different platform (provided that
the HW is available, see below). Changing the default depending on the
platform you're is not very good for reproducibility.

> Having said that, there could be a fallback in case GICv2 emulation is
> not available. Let me take a look at that.

You could try and pick a GICv3 emulation if v2 is not available, and
probably print a warning in that case.

> Also thinking about the future (ITS emulation) I found that I'd like to
> replace this option with something more generic like --irqchip=.

That's an orthogonal issue, but yes, this is probably better.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...