Hi Christoffer,
On 09/01/15 12:33, Christoffer Dall wrote:
> On Fri, Jan 09, 2015 at 11:54:36AM +0000, Andre Przywara wrote:
>> While we can easily register and unregister KVM devices, there is
>> currently no easy way of checking whether a device has been
>> registered.
>> Introduce kvm_check_device_type() for that purpose and use it in two
>> existing functions. Also change the return code for an invalid
>> type number from ENOSPC to EINVAL.
>> This function will be later used by another patch set to check
>> whether a KVM_CREATE_IRQCHIP ioctl is valid.
>
> I feel like this is misguided and the vgic should be able to figure this
> stuff out internally. Did you have code for this approach somewhere
> that I can take a look at?
I pushed my WIP patch on top of the kvm-gicv3/v6 tree.
Given how that looks I reckoned the generic solution would be more
preferable.
Basically we internally decide in the _probe function whether we support
GICv2 emulation or not, which is mostly driven by device tree
properties. So at the moment I just register the GIC_V2 KVM device or
not. Now with the "vgic internal" solution I misuse the GICV address
base as a hint of the GICv2 emulation availability. Alternatively I have
to introduce a new variable to mirror what the KVM device array already
holds, which seems kind of exerted to me.
Besides that I am not sure if the GICV address hint will always be a
reliable indicator and what we will do if there will be another GIC
model to be emulated in the future (maybe we need that for the ITS
emulation already?)
So I prefer the more generic solution.
Let me know what you think, I can as well drop 18/20 and merge the above
mentioned patch.
> I forget: Are we still requiring KVM_CREATE_IRQCHIP for VGICv3 or are we
> just relying on users to use KVM_CREATE_DEVICE for anything in the
> future?
Since KVM_CREATE_IRQCHIP does not take an argument, we cannot use it for
GICv3. So GICv3 mandates KVM_CREATE_DEVICE. We need userspace
adjustments for GICv3 anyway, so that's not a problem.
Cheers,
Andre.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> Changelog:
>> (new in v6)
>>
>> include/linux/kvm_host.h | 1 +
>> virt/kvm/kvm_main.c | 33 +++++++++++++++++++++------------
>> 2 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 83d770e..723d0d2 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1038,6 +1038,7 @@ void kvm_device_get(struct kvm_device *dev);
>> void kvm_device_put(struct kvm_device *dev);
>> struct kvm_device *kvm_device_from_filp(struct file *filp);
>> int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type);
>> +int kvm_check_device_type(u32 type);
>> void kvm_unregister_device_ops(u32 type);
>>
>> extern struct kvm_device_ops kvm_mpic_ops;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 1cc6e2e..c7711b2 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2341,13 +2341,24 @@ static struct kvm_device_ops *kvm_device_ops_table[KVM_DEV_TYPE_MAX] = {
>> #endif
>> };
>>
>> -int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
>> +int kvm_check_device_type(u32 type)
>> {
>> if (type >= ARRAY_SIZE(kvm_device_ops_table))
>> - return -ENOSPC;
>> + return -EINVAL;
>>
>> - if (kvm_device_ops_table[type] != NULL)
>> - return -EEXIST;
>> + if (kvm_device_ops_table[type] == NULL)
>> + return -ENODEV;
>> +
>> + return -EEXIST;
>
> I think this looks a bit screwy, the function always return errors...?
>
>> +}
>> +
>> +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type)
>> +{
>> + int ret;
>> +
>> + ret = kvm_check_device_type(type);
>> + if (ret != -ENODEV)
>> + return ret;
>>
>> kvm_device_ops_table[type] = ops;
>> return 0;
>> @@ -2364,19 +2375,17 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
>> {
>> struct kvm_device_ops *ops = NULL;
>> struct kvm_device *dev;
>> - bool test = cd->flags & KVM_CREATE_DEVICE_TEST;
>> int ret;
>>
>> - if (cd->type >= ARRAY_SIZE(kvm_device_ops_table))
>> - return -ENODEV;
>
> now you're basically changing a userspace-facing ABI because the ENODEV
> becomes an EINVAL, right?
>
>> -
>> - ops = kvm_device_ops_table[cd->type];
>> - if (ops == NULL)
>> - return -ENODEV;
>> + ret = kvm_check_device_type(cd->type);
>> + if (ret != -EEXIST)
>> + return ret;
>>
>> - if (test)
>> + if (cd->flags & KVM_CREATE_DEVICE_TEST)
>> return 0;
>>
>> + ops = kvm_device_ops_table[cd->type];
>> +
>> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> if (!dev)
>> return -ENOMEM;
>> --
>> 1.7.9.5
>>
>
> Thanks,
> -Christoffer
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
