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

[Eric Auger]

On 08/12/2015 04:24 PM, Christoffer Dall wrote:
> On Wed, Aug 12, 2015 at 4:14 PM, Eric Auger <address@hidden> wrote:
>> Hi,
>> On 08/12/2015 03:23 PM, Christoffer Dall wrote:
>>> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <address@hidden> wrote:
>>>> On 12 August 2015 at 13:27, Pavel Fedin <address@hidden> 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.
>>
> Note that there's a difference between "I called the create-device
> ioctl, and the creation of the device failed" vs. "I called the ioctl
> and I got an error because the ioctl is not supported", only in the
> latter case should you fall back to the older API.
yes understood; practically kvm_ioctl_create_device used in test mode
just looks for the specific device in the registered KVM device list and
that's it. whatever the case, API not supported or device not found it
reports -ENODEV.

Eric
> 
> Not sure if the end result is the same with the suggested approach,
> based on the right error values etc.
> 
> -Christoffer
>