Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Eric Auger <eric.auger@linaro.org>]

Hi,
On 08/12/2015 03:23 PM, Christoffer Dall wrote:
> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>>>  Hello!
>>>
>>>> I still think this is the wrong approach -- see my remarks
>>>> in the previous round of patch review.
>>>
>>>  You know... I thought a little bit...
>>>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? 
yes
If yes, we can just leave this test as it is, because if it says that
GICv2 is supported by KVM_CREATE_DEVICE, then:
>>> 1. We use new API. No KVM_IRQCHIP_CREATE.
>>> 2. GICv3 may be supported.
>>>
>>>  Therefore, if we do this check, and it succeeds, then we just
>>> proceed, and later actually try to create GICv3. If it fails for some
>>> reason, we will see error message anyway. So would it be OK just not
>>> to touch kvm_arch_irqchip_create() at all?
>>
>> No, because then if the kernel only supports GICv3 the
>> code in kvm_arch_irqchip_create() (as it is currently written)
>> will erroneously fall back to using the old API.
>>
>> Christoffer: the question was, why does kvm_arch_irqchip_create()
>> not only check the KVM_CAP_DEVICE_CTRL but also try to see
>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
>> kernels which have the capability bit set but which can't
>> actually use KVM_CREATE_DEVICE to create the irqchip?
> 
> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is
> an orthogonal concept from how to create the vgic, and you could
> advertise this capability without also supporting the GICv2 device
> type.
> 
> However, I don't believe such kernels exist
the capability was advertised for arm/arm64 with GICv2
7330672  KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8
months ago) <Christoffer Dall>
so effectively I don't think we have any arm kernel advertising the CAP
without GICv3.

 and they cannot in the
> future as that would be because we would remove an actively supported
> API.
> 
>>
>> My preference here would be for kvm_arch_irqchip_create()
>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
>> for its "can we use the new API" test; that will then
>> work whether we have a GICv2 or GICv3 in the host. (The
>> actual GIC device creation later on might fail, of course,
>> but that can be handled at that point. The only thing
>> we need to do as early as kvm_arch_irqchip_create is
>> determine whether we must use the old API.)
another way was proposed in the past was consisting in calling first
ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true);
if this succeeds this means we have the new API (the only one used vy
v3) and hence we have it as well for VGIC_V2, we can return ...
if this fails we just can say VGICv3 KVM device hasn't registered, try
KVM_DEV_TYPE_ARM_VGIC_V2
if we have it return else fall back to older API

I think it worked as well?

Besides I think Peter's suggestion is simpler.

For info we also use KVM VFIO device now.

Best Regards

Eric
>>
> I'm fine with this.
> 
> -Christoffer
>