diff mbox series

[v3,20/20] arm/virt: Add support for GICv2 virtualization extensions

Message ID 20180629132954.24269-21-luc.michel@greensocs.com
State New
Headers show
Series arm_gic: add virtualization extensions support | expand

Commit Message

Luc Michel June 29, 2018, 1:29 p.m. UTC
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(-)

Comments

Jan Kiszka July 5, 2018, 6:51 a.m. UTC | #1
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...

Jan

[1]
https://github.com/siemens/jailhouse/blob/master/hypervisor/arch/arm-common/gic-v2.c#L148
Jan Kiszka July 5, 2018, 8 a.m. UTC | #2
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
Luc Michel July 5, 2018, 8:46 a.m. UTC | #3
On 07/05/2018 10:00 AM, Jan Kiszka wrote:
> 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.Hi Jan, thanks for your feedback!

Yes I encountered the same issue with Xen in SMP (see my cover letter).
Re-reading the GICv2 specs, it's now clear to me that a read to
ITARGETSR0 to ITARGETSR7 should return "the number of the processor
performing the read". Reading the message of commit 2bb3135 in Linux, it
seems that older versions of the GIC exposed this value in IRQs 29, 30,
31, hence the
   if (irq >= 29 && irq <= 31) { res = cm; }
in the current QEMU implementation.

I should probably add a patch to fix that. I'll have to dig in specs of
older GIC revisions to see when this behaviour actually appeared.

Maybe I wait for some reviews before sending a new re-roll?
Peter, any thoughts?

Thanks.

Luc

> 
> Jan
>
Peter Maydell July 5, 2018, 9:28 a.m. UTC | #4
On 5 July 2018 at 09:46, Luc Michel <luc.michel@greensocs.com> wrote:
> Yes I encountered the same issue with Xen in SMP (see my cover letter).
> Re-reading the GICv2 specs, it's now clear to me that a read to
> ITARGETSR0 to ITARGETSR7 should return "the number of the processor
> performing the read". Reading the message of commit 2bb3135 in Linux, it
> seems that older versions of the GIC exposed this value in IRQs 29, 30,
> 31, hence the
>    if (irq >= 29 && irq <= 31) { res = cm; }
> in the current QEMU implementation.
>
> I should probably add a patch to fix that. I'll have to dig in specs of
> older GIC revisions to see when this behaviour actually appeared.
>
> Maybe I wait for some reviews before sending a new re-roll?
> Peter, any thoughts?

I'm probably not going to have time to look at any of this
GICv2 stuff for a bit (due to softfreeze and other for-3.0
work), so don't wait for my responses if you think a reroll
makes sense.

thanks
-- PMM
Jan Kiszka July 6, 2018, 9:25 a.m. UTC | #5
On 2018-07-05 10:00, Jan Kiszka wrote:
> 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.

The shutdown issue actually turned out to be a Jailhouse bug. Fixed now
as well, and we run smoothly in GICv2 mode over QEMU, also with the
secondary Linux guest!

Jan
Peter Maydell July 12, 2018, 2:43 p.m. UTC | #6
On 29 June 2018 at 14:29, Luc Michel <luc.michel@greensocs.com> 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));

The maintenance interrupt number is the same for GICv2 and v3, so
this seems a bit unnecessary -- we can just rename the constant to
ARCH_GIC_MAINT_IRQ and not condition it on the GIC version at all.

>          }
>      }
>
> 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 },

This is too small a size -- it doesn't include the GICV_DIR.
I would recommend making both of these sized 0x10000, ie a full
64K page. We don't want to have anything else in there for the
case where we're using 64K pages on a 64-bit guest CPU.

>      /* 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);

You'll find this needs to be fixed up a bit when rebased on current master.

> +        }
>      }

thanks
-- PMM
Peter Maydell July 12, 2018, 2:57 p.m. UTC | #7
On 5 July 2018 at 09:46, Luc Michel <luc.michel@greensocs.com> wrote:
> On 07/05/2018 10:00 AM, Jan Kiszka wrote:
>> On 2018-07-05 08:51, Jan Kiszka wrote:
>>> 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);

We already have the CPU number of the current cpu in the 'cpu'
local variable, so we don't need to call gic_get_current_cpu() again,
and we have 1 << cpu in "cm".

>> +                } 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.Hi Jan, thanks for your feedback!
>
> Yes I encountered the same issue with Xen in SMP (see my cover letter).
> Re-reading the GICv2 specs, it's now clear to me that a read to
> ITARGETSR0 to ITARGETSR7 should return "the number of the processor
> performing the read". Reading the message of commit 2bb3135 in Linux, it
> seems that older versions of the GIC exposed this value in IRQs 29, 30,
> 31, hence the
>    if (irq >= 29 && irq <= 31) { res = cm; }
> in the current QEMU implementation.
>
> I should probably add a patch to fix that. I'll have to dig in specs of
> older GIC revisions to see when this behaviour actually appeared.

The "29..31 give the current CPU and others are zero" behaviour is
specific to the 11MPCore:
http://arminfo.emea.arm.com/help/topic/com.arm.doc.ddi0360f/CCHBHJFH.html
The GICv1 spec matches the GICv2 here.

So what we want is probably to refactor this to pull the 11MPCore
code out as the top level special case (since it's weird for
uniprocessor setups too). I'll send a patch in a bit...

thanks
-- PMM
diff mbox series

Patch

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,