diff mbox series

[RFC,V1,09/12] libxl: Handle virtio-mmio irq in more correct way

Message ID 1596478888-23030-10-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Tyshchenko Aug. 3, 2020, 6:21 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch makes possible to use device passthrough again.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 tools/libxl/libxl_arm.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

Comments

Stefano Stabellini Aug. 4, 2020, 11:22 p.m. UTC | #1 
On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch makes possible to use device passthrough again.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  tools/libxl/libxl_arm.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 620b499..4f748e3 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -9,6 +9,10 @@
>  #include <assert.h>
>  #include <xen/device_tree_defs.h>
>  
> +#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x02000000)
> +#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
> +#define GUEST_VIRTIO_MMIO_SPI   33

They should be in xen/include/public/arch-arm.h

Is one interrupt enough if there are multiple virtio devices? Is it one
interrupt for all virtio devices, or one for each device?

Of course this patch should be folded in the patch to add virtio support
to libxl.


>  static const char *gicv_to_string(libxl_gic_version gic_version)
>  {
>      switch (gic_version) {
> @@ -27,8 +31,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>  {
>      uint32_t nr_spis = 0;
>      unsigned int i;
> -    uint32_t vuart_irq;
> -    bool vuart_enabled = false;
> +    uint32_t vuart_irq, virtio_irq;
> +    bool vuart_enabled = false, virtio_enabled = false;
>  
>      /*
>       * If pl011 vuart is enabled then increment the nr_spis to allow allocation
> @@ -40,6 +44,17 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          vuart_enabled = true;
>      }
>  
> +    /*
> +     * XXX: Handle properly virtio
> +     * A proper solution would be the toolstack to allocate the interrupts
> +     * used by each virtio backend and let the backend now which one is used
> +     */
> +    if (libxl_defbool_val(d_config->b_info.arch_arm.virtio)) {
> +        nr_spis += (GUEST_VIRTIO_MMIO_SPI - 32) + 1;
> +        virtio_irq = GUEST_VIRTIO_MMIO_SPI;
> +        virtio_enabled = true;
> +    }
> +
>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>          uint32_t irq = d_config->b_info.irqs[i];
>          uint32_t spi;
> @@ -59,6 +74,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>              return ERROR_FAIL;
>          }
>  
> +        /* The same check as for vpl011 */
> +        if (virtio_enabled && irq == virtio_irq) {
> +            LOG(ERROR, "Physical IRQ %u conflicting with virtio SPI\n", irq);
> +            return ERROR_FAIL;
> +        }
> +
>          if (irq < 32)
>              continue;
>  
> @@ -68,10 +89,6 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>              nr_spis = spi + 1;
>      }
>  
> -
> -    /* XXX: Handle properly virtio */
> -    nr_spis = 1;
> -
>      LOG(DEBUG, "Configure the domain");
>  
>      config->arch.nr_spis = nr_spis;
> @@ -663,10 +680,6 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> -#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x02000000)
> -#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
> -#define GUEST_VIRTIO_MMIO_SPI   33
> -
>  static int make_virtio_mmio_node(libxl__gc *gc, void *fdt)
>  {
>      int res;
> -- 
> 2.7.4
>
Oleksandr Tyshchenko Aug. 5, 2020, 8:51 p.m. UTC | #2 
On 05.08.20 02:22, Stefano Stabellini wrote:

Hi Stefano

> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch makes possible to use device passthrough again.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   tools/libxl/libxl_arm.c | 33 +++++++++++++++++++++++----------
>>   1 file changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 620b499..4f748e3 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -9,6 +9,10 @@
>>   #include <assert.h>
>>   #include <xen/device_tree_defs.h>
>>   
>> +#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x02000000)
>> +#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
>> +#define GUEST_VIRTIO_MMIO_SPI   33
> They should be in xen/include/public/arch-arm.h

ok


>
> Is one interrupt enough if there are multiple virtio devices? Is it one
> interrupt for all virtio devices, or one for each device?

   One interrupt for each virtio device. I experimented with current 
series and assigned 4 disk partitions to the guest. This resulted in 4 
separate device-tree nodes, and each node had individual SPI and MMIO range.


>
> Of course this patch should be folded in the patch to add virtio support
> to libxl.

ok
diff mbox series

Patch

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 620b499..4f748e3 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -9,6 +9,10 @@ 
 #include <assert.h>
 #include <xen/device_tree_defs.h>
 
+#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x02000000)
+#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
+#define GUEST_VIRTIO_MMIO_SPI   33
+
 static const char *gicv_to_string(libxl_gic_version gic_version)
 {
     switch (gic_version) {
@@ -27,8 +31,8 @@  int libxl__arch_domain_prepare_config(libxl__gc *gc,
 {
     uint32_t nr_spis = 0;
     unsigned int i;
-    uint32_t vuart_irq;
-    bool vuart_enabled = false;
+    uint32_t vuart_irq, virtio_irq;
+    bool vuart_enabled = false, virtio_enabled = false;
 
     /*
      * If pl011 vuart is enabled then increment the nr_spis to allow allocation
@@ -40,6 +44,17 @@  int libxl__arch_domain_prepare_config(libxl__gc *gc,
         vuart_enabled = true;
     }
 
+    /*
+     * XXX: Handle properly virtio
+     * A proper solution would be the toolstack to allocate the interrupts
+     * used by each virtio backend and let the backend now which one is used
+     */
+    if (libxl_defbool_val(d_config->b_info.arch_arm.virtio)) {
+        nr_spis += (GUEST_VIRTIO_MMIO_SPI - 32) + 1;
+        virtio_irq = GUEST_VIRTIO_MMIO_SPI;
+        virtio_enabled = true;
+    }
+
     for (i = 0; i < d_config->b_info.num_irqs; i++) {
         uint32_t irq = d_config->b_info.irqs[i];
         uint32_t spi;
@@ -59,6 +74,12 @@  int libxl__arch_domain_prepare_config(libxl__gc *gc,
             return ERROR_FAIL;
         }
 
+        /* The same check as for vpl011 */
+        if (virtio_enabled && irq == virtio_irq) {
+            LOG(ERROR, "Physical IRQ %u conflicting with virtio SPI\n", irq);
+            return ERROR_FAIL;
+        }
+
         if (irq < 32)
             continue;
 
@@ -68,10 +89,6 @@  int libxl__arch_domain_prepare_config(libxl__gc *gc,
             nr_spis = spi + 1;
     }
 
-
-    /* XXX: Handle properly virtio */
-    nr_spis = 1;
-
     LOG(DEBUG, "Configure the domain");
 
     config->arch.nr_spis = nr_spis;
@@ -663,10 +680,6 @@  static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
-#define GUEST_VIRTIO_MMIO_BASE  xen_mk_ullong(0x02000000)
-#define GUEST_VIRTIO_MMIO_SIZE  xen_mk_ullong(0x200)
-#define GUEST_VIRTIO_MMIO_SPI   33
-
 static int make_virtio_mmio_node(libxl__gc *gc, void *fdt)
 {
     int res;