Re: [Qemu-devel] [PATCH v3 20/20] arm/virt: Add support for GICv2 virtualization extensions

[Jan Kiszka <jan.kiszka@web.de>]

On 2018-07-05 08:51, Jan Kiszka wrote:
> On 2018-06-29 15:29, Luc Michel wrote:
>> Add support for GICv2 virtualization extensions by mapping the necessary
>> I/O regions and connecting the maintenance IRQ lines.
>>
>> Declare those additions in the device tree and in the ACPI tables.
>>
>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>> ---
>>  hw/arm/virt-acpi-build.c |  4 ++++
>>  hw/arm/virt.c            | 50 +++++++++++++++++++++++++++++++++-------
>>  include/hw/arm/virt.h    |  3 +++
>>  3 files changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 6ea47e2588..3b74bf0372 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -659,6 +659,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>          gicc->length = sizeof(*gicc);
>>          if (vms->gic_version == 2) {
>>              gicc->base_address = cpu_to_le64(memmap[VIRT_GIC_CPU].base);
>> +            gicc->gich_base_address = cpu_to_le64(memmap[VIRT_GIC_HYP].base);
>> +            gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
>>          }
>>          gicc->cpu_interface_number = cpu_to_le32(i);
>>          gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>> @@ -670,6 +672,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>          }
>>          if (vms->virt && vms->gic_version == 3) {
>>              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GICV3_MAINT_IRQ));
>> +        } else if (vms->virt && vms->gic_version == 2) {
>> +            gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GICV2_MAINT_IRQ));
>>          }
>>      }
>>  
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 742f68afca..e45b9de3be 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -131,6 +131,8 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
>>      [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
>>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>> +    [VIRT_GIC_HYP] =            { 0x08030000, 0x00001000 },
>> +    [VIRT_GIC_VCPU] =           { 0x08040000, 0x00001000 },
>>      /* The space in between here is reserved for GICv3 CPU/vCPU/HYP */
>>      [VIRT_GIC_ITS] =            { 0x08080000, 0x00020000 },
>>      /* This redistributor space allows up to 2*64kB*123 CPUs */
>> @@ -438,11 +440,26 @@ static void fdt_add_gic_node(VirtMachineState *vms)
>>          /* 'cortex-a15-gic' means 'GIC v2' */
>>          qemu_fdt_setprop_string(vms->fdt, "/intc", "compatible",
>>                                  "arm,cortex-a15-gic");
>> -        qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
>> -                                      2, vms->memmap[VIRT_GIC_DIST].base,
>> -                                      2, vms->memmap[VIRT_GIC_DIST].size,
>> -                                      2, vms->memmap[VIRT_GIC_CPU].base,
>> -                                      2, vms->memmap[VIRT_GIC_CPU].size);
>> +        if (!vms->virt) {
>> +            qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
>> +                                         2, vms->memmap[VIRT_GIC_DIST].base,
>> +                                         2, vms->memmap[VIRT_GIC_DIST].size,
>> +                                         2, vms->memmap[VIRT_GIC_CPU].base,
>> +                                         2, vms->memmap[VIRT_GIC_CPU].size);
>> +        } else {
>> +            qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
>> +                                         2, vms->memmap[VIRT_GIC_DIST].base,
>> +                                         2, vms->memmap[VIRT_GIC_DIST].size,
>> +                                         2, vms->memmap[VIRT_GIC_CPU].base,
>> +                                         2, vms->memmap[VIRT_GIC_CPU].size,
>> +                                         2, vms->memmap[VIRT_GIC_HYP].base,
>> +                                         2, vms->memmap[VIRT_GIC_HYP].size,
>> +                                         2, vms->memmap[VIRT_GIC_VCPU].base,
>> +                                         2, vms->memmap[VIRT_GIC_VCPU].size);
>> +            qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
>> +                                   GIC_FDT_IRQ_TYPE_PPI, ARCH_GICV2_MAINT_IRQ,
>> +                                   GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>> +        }
>>      }
>>  
>>      qemu_fdt_setprop_cell(vms->fdt, "/intc", "phandle", vms->gic_phandle);
>> @@ -563,6 +580,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>              qdev_prop_set_uint32(gicdev, "redist-region-count[1]",
>>                  MIN(smp_cpus - redist0_count, redist1_capacity));
>>          }
>> +    } else {
>> +        if (!kvm_irqchip_in_kernel()) {
>> +            qdev_prop_set_bit(gicdev, "has-virtualization-extensions",
>> +                              vms->virt);
>> +        }
>>      }
>>      qdev_init_nofail(gicdev);
>>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>> @@ -574,6 +596,10 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>          }
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>> +        if (vms->virt) {
>> +            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_HYP].base);
>> +            sysbus_mmio_map(gicbusdev, 3, vms->memmap[VIRT_GIC_VCPU].base);
>> +        }
>>      }
>>  
>>      /* Wire the outputs from each CPU's generic timer and the GICv3
>> @@ -600,9 +626,17 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>                                                     ppibase + timer_irq[irq]));
>>          }
>>  
>> -        qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
>> -                                    qdev_get_gpio_in(gicdev, ppibase
>> -                                                     + ARCH_GICV3_MAINT_IRQ));
>> +        if (type == 3) {
>> +            qemu_irq irq = qdev_get_gpio_in(gicdev,
>> +                                            ppibase + ARCH_GICV3_MAINT_IRQ);
>> +            qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt",
>> +                                        0, irq);
>> +        } else if (vms->virt) {
>> +            qemu_irq irq = qdev_get_gpio_in(gicdev,
>> +                                            ppibase + ARCH_GICV2_MAINT_IRQ);
>> +            sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus, irq);
>> +        }
>> +
>>          qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
>>                                      qdev_get_gpio_in(gicdev, ppibase
>>                                                       + VIRTUAL_PMU_IRQ));
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 9a870ccb6a..9e2f33f2d1 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -42,6 +42,7 @@
>>  #define NUM_VIRTIO_TRANSPORTS 32
>>  #define NUM_SMMU_IRQS          4
>>  
>> +#define ARCH_GICV2_MAINT_IRQ  9
>>  #define ARCH_GICV3_MAINT_IRQ  9
>>  
>>  #define ARCH_TIMER_VIRT_IRQ   11
>> @@ -60,6 +61,8 @@ enum {
>>      VIRT_GIC_DIST,
>>      VIRT_GIC_CPU,
>>      VIRT_GIC_V2M,
>> +    VIRT_GIC_HYP,
>> +    VIRT_GIC_VCPU,
>>      VIRT_GIC_ITS,
>>      VIRT_GIC_REDIST,
>>      VIRT_GIC_REDIST2,
>>
> 
> This one apparently requires rebasing over master. Did this manually.
> 
> But now I'm running into troubles with reading back GICD ITARGETSR.
> Maybe we are emulating an "early implementation" here?
> 
> [from the related Jailhouse code [1]]
> /*
>  * Get the CPU interface ID for this cpu. It can be discovered by
>  * reading the banked value of the PPI and IPI TARGET registers
>  * Patch 2bb3135 in Linux explains why the probe may need to scans the
>  * first 8 registers: some early implementation returned 0 for the first
>  * ITARGETSR registers.
>  * Since those didn't have virtualization extensions, we can safely
>  * ignore that case.
>  */
> 
> But maybe I'm just off with the configuration, checking...
> 

As suspected, it's a bug in QEMU, this resolves it, and I can run Linux
as root cell and a bare metal non-root cell:

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7d24348d96..199f953ddb 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -965,7 +965,11 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
             if (irq >= 29 && irq <= 31) {
                 res = cm;
             } else {
-                res = GIC_DIST_TARGET(irq);
+                if (irq < GIC_INTERNAL) {
+                    res = 1 << gic_get_current_cpu(s);
+                } else {
+                    res = GIC_DIST_TARGET(irq);
+                }
             }
         }
     } else if (offset < 0xf00) {

Didn't test Linux as non-root cell (secondary guest) yet, but that
should work as well. I'm seeing issues in an error shutdown path, but
that might be the same on real hw, needs cross-checking.

Jan