diff mbox series

[v2,09/21] hw/vfio/pci: Convert CONFIG_KVM check to runtime one

Message ID 20250308230917.18907-10-philmd@linaro.org (mailing list archive)
State New
Headers show
Series hw/vfio: Build various objects once | expand

Commit Message

Philippe Mathieu-Daudé March 8, 2025, 11:09 p.m. UTC 
Use the runtime kvm_enabled() helper to check whether
KVM is available or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Eric Auger March 10, 2025, 9:54 a.m. UTC | #1 
Hi Philippe,

On 3/9/25 12:09 AM, Philippe Mathieu-Daudé wrote:
> Use the runtime kvm_enabled() helper to check whether
> KVM is available or not.

Miss the "why" of this patch.

By the way I fail to remember/see where kvm_allowed is set.

I am also confused because we still have some code, like in
vfio/common.c which does both checks:
#ifdef CONFIG_KVM
        if (kvm_enabled()) {
            max_memslots = kvm_get_max_memslots();
        }
#endif


Thanks

Eric

>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/pci.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index fdbc15885d4..9872884ff8a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -118,8 +118,13 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>  
>  static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
> -#ifdef CONFIG_KVM
> -    int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
> +    int irq_fd;
> +
> +    if (!kvm_enabled()) {
> +        return true;
> +    }
> +
> +    irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
>  
>      if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
>          vdev->intx.route.mode != PCI_INTX_ENABLED ||
> @@ -171,16 +176,13 @@ fail_irqfd:
>  fail:
>      qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
>      vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
> +
>      return false;
> -#else
> -    return true;
> -#endif
>  }
>  
>  static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
>  {
> -#ifdef CONFIG_KVM
> -    if (!vdev->intx.kvm_accel) {
> +    if (!kvm_enabled() || !vdev->intx.kvm_accel) {
>          return;
>      }
>  
> @@ -211,7 +213,6 @@ static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
>      vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  
>      trace_vfio_intx_disable_kvm(vdev->vbasedev.name);
> -#endif
>  }
>  
>  static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route)
> @@ -278,7 +279,6 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>      vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
>      pci_config_set_interrupt_pin(vdev->pdev.config, pin);
>  
> -#ifdef CONFIG_KVM
>      /*
>       * Only conditional to avoid generating error messages on platforms
>       * where we won't actually use the result anyway.
> @@ -287,7 +287,6 @@ static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>          vdev->intx.route = pci_device_route_intx_to_irq(&vdev->pdev,
>                                                          vdev->intx.pin);
>      }
> -#endif
>  
>      ret = event_notifier_init(&vdev->intx.interrupt, 0);
>      if (ret) {
BALATON Zoltan March 10, 2025, 12:54 p.m. UTC | #2 
On Mon, 10 Mar 2025, Eric Auger wrote:
> Hi Philippe,
>
> On 3/9/25 12:09 AM, Philippe Mathieu-Daudé wrote:
>> Use the runtime kvm_enabled() helper to check whether
>> KVM is available or not.
>
> Miss the "why" of this patch.
>
> By the way I fail to remember/see where kvm_allowed is set.

It's in include/system/kvm.h

> I am also confused because we still have some code, like in
> vfio/common.c which does both checks:
> #ifdef CONFIG_KVM
>         if (kvm_enabled()) {
>             max_memslots = kvm_get_max_memslots();
>         }
> #endif

I think this is because if KVM is not available the if cannot be true so 
it can be left out altogether. This may make sense on platforms like 
Windows and macOS where QEMU is compiled without KVM so basically 
everywhere except Linux.

Regards,
BALATON Zoltan
Eric Auger March 10, 2025, 2:53 p.m. UTC | #3 
Hi,


On 3/10/25 1:54 PM, BALATON Zoltan wrote:
> On Mon, 10 Mar 2025, Eric Auger wrote:
>> Hi Philippe,
>>
>> On 3/9/25 12:09 AM, Philippe Mathieu-Daudé wrote:
>>> Use the runtime kvm_enabled() helper to check whether
>>> KVM is available or not.
>>
>> Miss the "why" of this patch.
>>
>> By the way I fail to remember/see where kvm_allowed is set.
>
> It's in include/system/kvm.h

There you can only find the kvm_enabled() macro definition.

I was eventually able to locate it:
accel/accel-system.c:    *(acc->allowed) = true;
in accel_init_machine()


>
>> I am also confused because we still have some code, like in
>> vfio/common.c which does both checks:
>> #ifdef CONFIG_KVM
>>         if (kvm_enabled()) {
>>             max_memslots = kvm_get_max_memslots();
>>         }
>> #endif
>
> I think this is because if KVM is not available the if cannot be true
> so it can be left out altogether. This may make sense on platforms
> like Windows and macOS where QEMU is compiled without KVM so basically
> everywhere except Linux.
But in practice we have a stub for kvm_get_max_memslots in
accel/stubs/kvm-stub.c.

Eric
>
> Regards,
> BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fdbc15885d4..9872884ff8a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -118,8 +118,13 @@  static void vfio_intx_eoi(VFIODevice *vbasedev)
 
 static bool vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
 {
-#ifdef CONFIG_KVM
-    int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
+    int irq_fd;
+
+    if (!kvm_enabled()) {
+        return true;
+    }
+
+    irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
 
     if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
         vdev->intx.route.mode != PCI_INTX_ENABLED ||
@@ -171,16 +176,13 @@  fail_irqfd:
 fail:
     qemu_set_fd_handler(irq_fd, vfio_intx_interrupt, NULL, vdev);
     vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
+
     return false;
-#else
-    return true;
-#endif
 }
 
 static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
 {
-#ifdef CONFIG_KVM
-    if (!vdev->intx.kvm_accel) {
+    if (!kvm_enabled() || !vdev->intx.kvm_accel) {
         return;
     }
 
@@ -211,7 +213,6 @@  static void vfio_intx_disable_kvm(VFIOPCIDevice *vdev)
     vfio_unmask_single_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
 
     trace_vfio_intx_disable_kvm(vdev->vbasedev.name);
-#endif
 }
 
 static void vfio_intx_update(VFIOPCIDevice *vdev, PCIINTxRoute *route)
@@ -278,7 +279,6 @@  static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
     vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
     pci_config_set_interrupt_pin(vdev->pdev.config, pin);
 
-#ifdef CONFIG_KVM
     /*
      * Only conditional to avoid generating error messages on platforms
      * where we won't actually use the result anyway.
@@ -287,7 +287,6 @@  static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
         vdev->intx.route = pci_device_route_intx_to_irq(&vdev->pdev,
                                                         vdev->intx.pin);
     }
-#endif
 
     ret = event_notifier_init(&vdev->intx.interrupt, 0);
     if (ret) {