Re: [PATCH v6 04/22] arm/gic: Add a new callback for creating MADT table for Dom0

[Julien Grall <julien.grall@arm.com>]

Hi Shannon,

On 17/03/2016 09:40, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Add a new member in gic_hw_operations which is used to creat MADT table

s/create/create/

> for Dom0.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic-v2.c     | 34 ++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c        |  5 +++++
>   xen/include/asm-arm/gic.h |  3 +++
>   4 files changed, 89 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 0fcb894..02db5f2 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -685,6 +685,35 @@ static void __init gicv2_dt_init(void)
>   }
>
>   #ifdef CONFIG_ACPI
> +static u32 gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
> +{
> +    struct acpi_subtable_header *header;
> +    struct acpi_madt_generic_interrupt *host_gicc, *gicc;
> +    u32 i, size, table_len = 0;
> +    u8 *base_ptr = d->arch.efi_acpi_table + offset;
> +
> +    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    if ( !header )
> +        panic("Can't get GICC entry");

I would prefer if you return an error here. In the future we may want to 
support hardware domain creation later (see the xen option hardware_dom).

> +    host_gicc = container_of(header, struct acpi_madt_generic_interrupt,
> +                             header);
> +
> +    size = sizeof(struct acpi_madt_generic_interrupt);
> +    /* Add Generic Interrupt */
> +    for ( i = 0; i < d->max_vcpus; i++ )
> +    {
> +        gicc = (struct acpi_madt_generic_interrupt *)(base_ptr + table_len);
> +        ACPI_MEMCPY(gicc, host_gicc, size);
> +        gicc->cpu_interface_number = i;
> +        gicc->uid = i;
> +        gicc->flags = ACPI_MADT_ENABLED;
> +        gicc->arm_mpidr = vcpuid_to_vaffinity(i);

What about the other fields?

* parking_version: DOM0 will always use PSCI => should be set to 0
* performance_interrupt: There is currently PMU support for DOM0
* gic{v,h}_base_address/vgic_interrupt: they are used by Xen => should 
not be exposed to the guest

> +        table_len += size;
> +    }
> +
> +    return table_len;
> +}
> +
>   static int __init
>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                           const unsigned long end)

[...]

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index f83fd88..d9fce4b 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1236,6 +1236,48 @@ static void __init gicv3_dt_init(void)
>   }
>
>   #ifdef CONFIG_ACPI
> +static u32 gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
> +{
> +    struct acpi_subtable_header *header;
> +    struct acpi_madt_generic_interrupt *host_gicc, *gicc;
> +    struct acpi_madt_generic_redistributor *gicr;
> +    u8 *base_ptr = d->arch.efi_acpi_table + offset;
> +    u32 i, table_len = 0, size;
> +
> +    /* Add Generic Interrupt */
> +    header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    if ( !header )
> +        panic("Can't get GICC entry");
> +    host_gicc = container_of(header, struct acpi_madt_generic_interrupt,
> +                             header);
> +
> +    size = sizeof(struct acpi_madt_generic_interrupt);
> +    for ( i = 0; i < d->max_vcpus; i++ )
> +    {
> +        gicc = (struct acpi_madt_generic_interrupt *)(base_ptr + table_len);
> +        ACPI_MEMCPY(gicc, host_gicc, size);
> +        gicc->cpu_interface_number = i;
> +        gicc->uid = i;
> +        gicc->flags = ACPI_MADT_ENABLED;
> +        gicc->arm_mpidr = vcpuid_to_vaffinity(i);
> +        table_len += size;

Ditto for the fields.

Furthermore, you use the Generic Redistributor table to describe the 
redistributor regions. So the field gicr_base_address should be set to 0.

> +    }
> +
> +    /* Add Generic Redistributor */
> +    size = sizeof(struct acpi_madt_generic_redistributor);
> +    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> +    {
> +        gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
> +        gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
> +        gicr->header.length = size;
> +        gicr->base_address = d->arch.vgic.rdist_regions[i].base;
> +        gicr->length = d->arch.vgic.rdist_regions[i].size;
> +        table_len += size;
> +    }
> +
> +    return table_len;
> +}
> +
>   static int __init
>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                           const unsigned long end)

[...]

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel