| Message ID | 1420560724-5345-1-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-01-06 at 16:12 +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() 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. You're checking whether a device type has been registered, not the device itself -- so could you make it kvm_check_device_type()? > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > can people comment whether there is an easier way to detect KVM > device registration _outside_ of virt/kvm/kvm_main.c? Using the > KVM_CREATE_DEVICE_TEST flag sounds like a fit, but > kvm_ioctl_create_device() isn't exported. Out of curiosity, why do you need to test it from inside the kernel but outside kvm_main.c? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Scott, thanks for looking at the patch. On 06/01/15 20:52, Scott Wood wrote: > On Tue, 2015-01-06 at 16:12 +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() 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. > > You're checking whether a device type has been registered, not the > device itself -- so could you make it kvm_check_device_type()? Sure. If you are OK with the general approach, I will include the patch in my GICv3 KVM emulation series. >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> Hi, >> >> can people comment whether there is an easier way to detect KVM >> device registration _outside_ of virt/kvm/kvm_main.c? Using the >> KVM_CREATE_DEVICE_TEST flag sounds like a fit, but >> kvm_ioctl_create_device() isn't exported. > > Out of curiosity, why do you need to test it from inside the kernel but > outside kvm_main.c? I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c. The problem is that while KVM_CREATE_DEVICE works fine with checking the availability of the requested device, KVM_CREATE_IRQCHIP does not - and the latter is handled in the arch specific parts of the code. At the moment the GIC_V2 is the only IRQ chip, so it's all or nothing right now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not always being available, so KVM_CREATE_IRQCHIP may fail although there is an in-kernel IRQ chip available. Instead of hacking something up I thought it would be cleaner to use the existing framework and just map KVM_CREATE_IRQCHIP to KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2). Thanks, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-01-07 at 10:55 +0000, Andre Przywara wrote: > Hi Scott, > > thanks for looking at the patch. > > On 06/01/15 20:52, Scott Wood wrote: > > Out of curiosity, why do you need to test it from inside the kernel but > > outside kvm_main.c? > > I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c. > The problem is that while KVM_CREATE_DEVICE works fine with checking the > availability of the requested device, KVM_CREATE_IRQCHIP does not - and > the latter is handled in the arch specific parts of the code. At the > moment the GIC_V2 is the only IRQ chip, so it's all or nothing right > now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not > always being available, so KVM_CREATE_IRQCHIP may fail although there is > an in-kernel IRQ chip available. > Instead of hacking something up I thought it would be cleaner to use the > existing framework and just map KVM_CREATE_IRQCHIP to > KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2). In that case you'd need the full create_device functionality from arch/arm/kvm, not just testing whether a device type is present, right? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/15 17:45, Scott Wood wrote: > On Wed, 2015-01-07 at 10:55 +0000, Andre Przywara wrote: >> Hi Scott, >> >> thanks for looking at the patch. >> >> On 06/01/15 20:52, Scott Wood wrote: >>> Out of curiosity, why do you need to test it from inside the kernel but >>> outside kvm_main.c? >> >> I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c. >> The problem is that while KVM_CREATE_DEVICE works fine with checking the >> availability of the requested device, KVM_CREATE_IRQCHIP does not - and >> the latter is handled in the arch specific parts of the code. At the >> moment the GIC_V2 is the only IRQ chip, so it's all or nothing right >> now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not >> always being available, so KVM_CREATE_IRQCHIP may fail although there is >> an in-kernel IRQ chip available. >> Instead of hacking something up I thought it would be cleaner to use the >> existing framework and just map KVM_CREATE_IRQCHIP to >> KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2). > > In that case you'd need the full create_device functionality from > arch/arm/kvm, not just testing whether a device type is present, right? Well, not really. On KVM_CREATE_IRQCHIP kvm_vgic_create is called, which is also what the KVM device .create function for GIC_V2 does. So yes, we don't fully use the KVM device framework, but just use the same functionality. Not sure if it would make sense to use more of the KVM device framework, as currently there is no issue with the current approach and I just need to know whether the GIC_V2 has been registered. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-01-07 at 18:11 +0000, Andre Przywara wrote: > On 07/01/15 17:45, Scott Wood wrote: > > On Wed, 2015-01-07 at 10:55 +0000, Andre Przywara wrote: > >> Hi Scott, > >> > >> thanks for looking at the patch. > >> > >> On 06/01/15 20:52, Scott Wood wrote: > >>> Out of curiosity, why do you need to test it from inside the kernel but > >>> outside kvm_main.c? > >> > >> I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c. > >> The problem is that while KVM_CREATE_DEVICE works fine with checking the > >> availability of the requested device, KVM_CREATE_IRQCHIP does not - and > >> the latter is handled in the arch specific parts of the code. At the > >> moment the GIC_V2 is the only IRQ chip, so it's all or nothing right > >> now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not > >> always being available, so KVM_CREATE_IRQCHIP may fail although there is > >> an in-kernel IRQ chip available. > >> Instead of hacking something up I thought it would be cleaner to use the > >> existing framework and just map KVM_CREATE_IRQCHIP to > >> KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2). > > > > In that case you'd need the full create_device functionality from > > arch/arm/kvm, not just testing whether a device type is present, right? > > Well, not really. On KVM_CREATE_IRQCHIP kvm_vgic_create is called, which > is also what the KVM device .create function for GIC_V2 does. > So yes, we don't fully use the KVM device framework, but just use the > same functionality. > Not sure if it would make sense to use more of the KVM device framework, > as currently there is no issue with the current approach and I just need > to know whether the GIC_V2 has been registered. If you're communicating privately with the vgic code to create the irqchip, can't you also communicate privately to see if that functionality is available? I'm not opposed to this patch, but it seems odd to use the device framework just for testing whether some other interface is available. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Scott, On 07/01/15 18:16, Scott Wood wrote: > On Wed, 2015-01-07 at 18:11 +0000, Andre Przywara wrote: >> On 07/01/15 17:45, Scott Wood wrote: >>> On Wed, 2015-01-07 at 10:55 +0000, Andre Przywara wrote: >>>> Hi Scott, >>>> >>>> thanks for looking at the patch. >>>> >>>> On 06/01/15 20:52, Scott Wood wrote: >>>>> Out of curiosity, why do you need to test it from inside the kernel but >>>>> outside kvm_main.c? >>>> >>>> I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c. >>>> The problem is that while KVM_CREATE_DEVICE works fine with checking the >>>> availability of the requested device, KVM_CREATE_IRQCHIP does not - and >>>> the latter is handled in the arch specific parts of the code. At the >>>> moment the GIC_V2 is the only IRQ chip, so it's all or nothing right >>>> now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not >>>> always being available, so KVM_CREATE_IRQCHIP may fail although there is >>>> an in-kernel IRQ chip available. >>>> Instead of hacking something up I thought it would be cleaner to use the >>>> existing framework and just map KVM_CREATE_IRQCHIP to >>>> KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2). >>> >>> In that case you'd need the full create_device functionality from >>> arch/arm/kvm, not just testing whether a device type is present, right? >> >> Well, not really. On KVM_CREATE_IRQCHIP kvm_vgic_create is called, which >> is also what the KVM device .create function for GIC_V2 does. >> So yes, we don't fully use the KVM device framework, but just use the >> same functionality. >> Not sure if it would make sense to use more of the KVM device framework, >> as currently there is no issue with the current approach and I just need >> to know whether the GIC_V2 has been registered. > > If you're communicating privately with the vgic code to create the > irqchip, can't you also communicate privately to see if that > functionality is available? I'm not opposed to this patch, but it seems > odd to use the device framework just for testing whether some other > interface is available. Mainly because I wanted to avoid a solution that is private to the VGIC and ARM. Also hacking this into the VGIC code looks much uglier than simply using kvm_check_device_type(). Currently I just register GIC_V2 or GIC_V3 according to the hardware's capabilities. Now I have to duplicate this decision making in the VGIC code again. I now worked around the issue without this patch now, but it looks quite ugly. I will send out a new version of the patch series tomorrow and will put you on CC: for the respective patches. So if in doubt we can continue this discussion on those patches later. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 83d770e..78e94ef 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(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..42604a9 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(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; +} + +int kvm_register_device_ops(struct kvm_device_ops *ops, u32 type) +{ + int ret; + + ret = kvm_check_device(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; - - ops = kvm_device_ops_table[cd->type]; - if (ops == NULL) - return -ENODEV; + ret = kvm_check_device(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;
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() 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. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Hi, can people comment whether there is an easier way to detect KVM device registration _outside_ of virt/kvm/kvm_main.c? Using the KVM_CREATE_DEVICE_TEST flag sounds like a fit, but kvm_ioctl_create_device() isn't exported. Also is my return code semantics too exerted? Cheers, Andre. include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 33 +++++++++++++++++++++------------ 2 files changed, 22 insertions(+), 12 deletions(-)