| Message ID | 1431893048-5214-20-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
+shannon On 21 May 2015 at 17:59, Julien Grall <julien.grall@citrix.com> wrote: > Hi Parth, > > On 17/05/15 21:03, Parth Dixit wrote: >> ACPI on Xen hypervisor uses MADT table for proper GIC initialization. >> It needs to parse GIC related subtables, collect CPU interface and distributor >> addresses and call driver initialization function (which is hardware >> abstraction agnostic). In a similar way, FDT initialize GICv2. >> >> Modify MADT table to clear GICH and GICV which are not needed >> in Dom0. >> >> NOTE: This commit allow to initialize GICv2 only. >> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org> >> Signed-off-by: Parth Dixit <parth.dixit@linaro.org> >> --- >> xen/arch/arm/gic-v2.c | 132 +++++++++++++++++++++++++++++++++++++++++- >> xen/arch/arm/gic.c | 18 +++++- >> xen/drivers/char/arm-uart.c | 136 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 284 insertions(+), 2 deletions(-) >> create mode 100644 xen/drivers/char/arm-uart.c >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 7276951..fcdcd19 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -28,6 +28,8 @@ >> #include <xen/list.h> >> #include <xen/device_tree.h> >> #include <xen/libfdt/libfdt.h> >> +#include <xen/acpi.h> >> +#include <acpi/actables.h> >> #include <asm/p2m.h> >> #include <asm/domain.h> >> #include <asm/platform.h> >> @@ -35,6 +37,7 @@ >> >> #include <asm/io.h> >> #include <asm/gic.h> >> +#include <asm/acpi.h> >> >> /* >> * LR register definitions are GIC v2 specific. >> @@ -692,9 +695,122 @@ static int __init dt_gicv2_init(void) >> return 0; >> } >> >> +#ifdef CONFIG_ACPI >> +static int __init >> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_interrupt *processor; >> + >> + processor = (struct acpi_madt_generic_interrupt *)header; >> + >> + if (BAD_MADT_ENTRY(processor, end)) > > if ( ... ) > >> + return -EINVAL; > > The indentation looks wrong. > >> + >> + /* Read from APIC table and fill up the GIC variables */ >> + >> + gicv2.cbase = processor->base_address; >> + gicv2.hbase = processor->gich_base_address; >> + gicv2.vbase = processor->gicv_base_address; > > This is per processor right? The value should be the same on each CPU as > we don't support non-banked GIC. > > You would have to check that each value is the same on every CPU. > >> + gicv2_info.maintenance_irq = processor->vgic_maintenance_interrupt; >> + if( processor->flags & ACPI_MADT_ENABLED ) > > The check doesn't seem necessary, > >> + { >> + if( processor->flags & ACPI_MADT_VGIC ) >> + set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_EDGE_BOTH); >> + else >> + set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_LEVEL_MASK); > > set_irq_type for local IRQ is very expensive. This should be done only one. > >> + } >> + >> + processor->gich_base_address = 0; >> + processor->gicv_base_address = 0; >> + processor->vgic_maintenance_interrupt = 0; >> + clean_dcache_va_range(processor, sizeof(struct acpi_madt_generic_interrupt)); > > Same remark as for the GTDT and MADT table. This doesn't belong to the > GIC initialization but DOM0 building. > > Furthermore, the number of CPU for domain may be different as long as > the processor->flags. > > Overall, I think this table should be recreated for DOM0. > >> + return 0; >> +} >> + >> +static int __init >> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_distributor *dist; >> + >> + dist = (struct acpi_madt_generic_distributor *)header; >> + >> + if (BAD_MADT_ENTRY(dist, end)) > > if ( ... ) > >> + return -EINVAL; > > The indentation looks wrong > >> + >> + gicv2.dbase = dist->base_address; >> + >> + return 0; >> +} >> + >> +static int __init acpi_gicv2_init(void) >> +{ >> + acpi_status status; >> + struct acpi_table_header *table; >> + u8 checksum; >> + int res; > > Given the usage of this variable I would rename it to count. > >> + >> + status = acpi_get_table(ACPI_SIG_MADT, 0, &table); > > For a generic ACPI device code it would make sense to pass the ACPI > table in parameter as you will likely have to retrieve the same table > within the same class of device (see Serial too). > >> + >> + if ( ACPI_FAILURE(status) ) >> + { > > Wrong indentation within the block. > >> + const char *msg = acpi_format_exception(status); > > Missing blank line > >> + printk("\nFailed to get MADT table, %s\n", msg); > > The first "\n" is not necessary. > >> + return 1; > > Please return a meaningful error code. > >> + } >> + >> + /* Collect CPU base addresses */ >> + res = acpi_parse_entries(ACPI_SIG_MADT, >> + sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_cpu, table, >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + 0); > > Wrong indentation > >> + if ( res < 0 ) > > 0 means the no GICC which should be throwing an error too. I would > replace the check by > > res <= 0 > >> + { > >> + printk("Error during GICC entries parsing\n"); >> + printk("\nFailed to initialize GIC IRQ controller\n"); > > One single printk("No valid GICC entries exists\n") would have been enough. > >> + return -EINVAL; >> + } >> + >> + /* >> + * Find distributor base address. We expect one distributor entry since >> + * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade. >> + */ >> + res = acpi_parse_entries(ACPI_SIG_MADT, >> + sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_distributor, table, >> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >> + 0); > > Wrong indentation > >> + >> + if ( res < 0 ) > > 0 means no distributor which means the ACPI is not valid. I would > replace the check by > > res <= 0 > > > You also need to check that we don't have multiple GICD entry. > >> + { >> + printk("Error during GICD entries parsing\n"); >> + printk("\nFailed to initialize GIC IRQ controller\n"); > > One single printk is enough. > >> + return -EINVAL; >> + } >> + >> + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length); >> + table->checksum -= checksum; >> + clean_dcache_va_range(table, sizeof(struct acpi_table_header)); > > This is coming out of nowhere without any explanation. > > I guess this is related to the change in the ACPI table for DOM0, right? > If so, this should not be in the GIC initialization but in a specific > DOM0 building code. > >> + return 0; >> +} >> +#else >> +static int __init acpi_gicv2_init(void) >> +{ >> +/* Should never reach here */ >> + return -EINVAL; >> +} >> +#endif >> + >> static int gicv2_init(void) >> { >> - dt_gicv2_init(); >> + if( acpi_disabled ) >> + dt_gicv2_init(); >> + else >> + acpi_gicv2_init(); > > acpi_gicv2_init is returning an error code. You should not ignore it. > >> >> /* TODO: Add check on distributor, cpu size */ >> >> @@ -790,7 +906,21 @@ DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC) >> .dt_match = gicv2_dt_match, >> .init = dt_gicv2_preinit, >> DT_DEVICE_END > > Missing blank line. > >> +#ifdef CONFIG_ACPI >> +/* Set up the GIC */ >> +static int __init acpi_gicv2_preinit(const void *data) >> +{ >> + gicv2_info.hw_version = GIC_V2; >> + register_gic_ops(&gicv2_ops); >> + >> + return 0; >> +} >> >> +ACPI_DEVICE_START(agicv2, "GICv2", DEVICE_GIC) >> + .class_type = GIC_V2, >> + .init = acpi_gicv2_preinit, >> +ACPI_DEVICE_END >> +#endif >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 701c306..e33bfd7 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -27,6 +27,7 @@ >> #include <xen/softirq.h> >> #include <xen/list.h> >> #include <xen/device_tree.h> >> +#include <xen/acpi.h> >> #include <asm/p2m.h> >> #include <asm/domain.h> >> #include <asm/platform.h> >> @@ -34,6 +35,7 @@ >> #include <asm/io.h> >> #include <asm/gic.h> >> #include <asm/vgic.h> >> +#include <asm/acpi.h> >> >> static void gic_restore_pending_irqs(struct vcpu *v); >> >> @@ -261,9 +263,23 @@ void __init dt_gic_preinit(void) >> dt_device_set_used_by(node, DOMID_XEN); >> } >> >> +static void __init acpi_gic_init(void) > > This function should be called acpi_gic_preinit as it's used during > pre-initialization. > >> +{ >> + int rc; >> + >> + /* NOTE: Only one GIC is supported */ >> + /* hardcode to gic v2 for now */ >> + rc = acpi_device_init(DEVICE_GIC, NULL, GIC_V2); > > This won't work for multiple GIC later. You should not try to use a > generic implementation just because it's there. > > If you don't find a way to deal with genericity, I prefer a call to > acpi_gicv2_preinit directly. > >> + if ( rc ) >> + panic("Unable to find compatible GIC in the ACPI table"); > > This is not true. You check the presence of the GIC ACPI table later. > >> +} >> + >> void __init gic_preinit(void) >> { >> - dt_gic_preinit(); >> + if( acpi_disabled ) >> + dt_gic_preinit(); >> + else >> + acpi_gic_init(); >> } >> >> /* Set up the GIC */ >> diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c >> new file mode 100644 >> index 0000000..2603508 >> --- /dev/null >> +++ b/xen/drivers/char/arm-uart.c > > This file doesn't belong to this patch. I will review it when it will be > placed in the right patch. > > Regards, > > -- > Julien Grall
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 7276951..fcdcd19 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -28,6 +28,8 @@ #include <xen/list.h> #include <xen/device_tree.h> #include <xen/libfdt/libfdt.h> +#include <xen/acpi.h> +#include <acpi/actables.h> #include <asm/p2m.h> #include <asm/domain.h> #include <asm/platform.h> @@ -35,6 +37,7 @@ #include <asm/io.h> #include <asm/gic.h> +#include <asm/acpi.h> /* * LR register definitions are GIC v2 specific. @@ -692,9 +695,122 @@ static int __init dt_gicv2_init(void) return 0; } +#ifdef CONFIG_ACPI +static int __init +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_interrupt *processor; + + processor = (struct acpi_madt_generic_interrupt *)header; + + if (BAD_MADT_ENTRY(processor, end)) + return -EINVAL; + + /* Read from APIC table and fill up the GIC variables */ + + gicv2.cbase = processor->base_address; + gicv2.hbase = processor->gich_base_address; + gicv2.vbase = processor->gicv_base_address; + gicv2_info.maintenance_irq = processor->vgic_maintenance_interrupt; + if( processor->flags & ACPI_MADT_ENABLED ) + { + if( processor->flags & ACPI_MADT_VGIC ) + set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_EDGE_BOTH); + else + set_irq_type(gicv2_info.maintenance_irq, ACPI_IRQ_TYPE_LEVEL_MASK); + } + + processor->gich_base_address = 0; + processor->gicv_base_address = 0; + processor->vgic_maintenance_interrupt = 0; + clean_dcache_va_range(processor, sizeof(struct acpi_madt_generic_interrupt)); + + return 0; +} + +static int __init +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_distributor *dist; + + dist = (struct acpi_madt_generic_distributor *)header; + + if (BAD_MADT_ENTRY(dist, end)) + return -EINVAL; + + gicv2.dbase = dist->base_address; + + return 0; +} + +static int __init acpi_gicv2_init(void) +{ + acpi_status status; + struct acpi_table_header *table; + u8 checksum; + int res; + + status = acpi_get_table(ACPI_SIG_MADT, 0, &table); + + if ( ACPI_FAILURE(status) ) + { + const char *msg = acpi_format_exception(status); + printk("\nFailed to get MADT table, %s\n", msg); + return 1; + } + + /* Collect CPU base addresses */ + res = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_cpu, table, + ACPI_MADT_TYPE_GENERIC_INTERRUPT, + 0); + if ( res < 0 ) + { + printk("Error during GICC entries parsing\n"); + printk("\nFailed to initialize GIC IRQ controller\n"); + return -EINVAL; + } + + /* + * Find distributor base address. We expect one distributor entry since + * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade. + */ + res = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_distributor, table, + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, + 0); + + if ( res < 0 ) + { + printk("Error during GICD entries parsing\n"); + printk("\nFailed to initialize GIC IRQ controller\n"); + return -EINVAL; + } + + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length); + table->checksum -= checksum; + clean_dcache_va_range(table, sizeof(struct acpi_table_header)); + + return 0; +} +#else +static int __init acpi_gicv2_init(void) +{ +/* Should never reach here */ + return -EINVAL; +} +#endif + static int gicv2_init(void) { - dt_gicv2_init(); + if( acpi_disabled ) + dt_gicv2_init(); + else + acpi_gicv2_init(); /* TODO: Add check on distributor, cpu size */ @@ -790,7 +906,21 @@ DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC) .dt_match = gicv2_dt_match, .init = dt_gicv2_preinit, DT_DEVICE_END +#ifdef CONFIG_ACPI +/* Set up the GIC */ +static int __init acpi_gicv2_preinit(const void *data) +{ + gicv2_info.hw_version = GIC_V2; + register_gic_ops(&gicv2_ops); + + return 0; +} +ACPI_DEVICE_START(agicv2, "GICv2", DEVICE_GIC) + .class_type = GIC_V2, + .init = acpi_gicv2_preinit, +ACPI_DEVICE_END +#endif /* * Local variables: * mode: C diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 701c306..e33bfd7 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -27,6 +27,7 @@ #include <xen/softirq.h> #include <xen/list.h> #include <xen/device_tree.h> +#include <xen/acpi.h> #include <asm/p2m.h> #include <asm/domain.h> #include <asm/platform.h> @@ -34,6 +35,7 @@ #include <asm/io.h> #include <asm/gic.h> #include <asm/vgic.h> +#include <asm/acpi.h> static void gic_restore_pending_irqs(struct vcpu *v); @@ -261,9 +263,23 @@ void __init dt_gic_preinit(void) dt_device_set_used_by(node, DOMID_XEN); } +static void __init acpi_gic_init(void) +{ + int rc; + + /* NOTE: Only one GIC is supported */ + /* hardcode to gic v2 for now */ + rc = acpi_device_init(DEVICE_GIC, NULL, GIC_V2); + if ( rc ) + panic("Unable to find compatible GIC in the ACPI table"); +} + void __init gic_preinit(void) { - dt_gic_preinit(); + if( acpi_disabled ) + dt_gic_preinit(); + else + acpi_gic_init(); } /* Set up the GIC */ diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c new file mode 100644 index 0000000..2603508 --- /dev/null +++ b/xen/drivers/char/arm-uart.c @@ -0,0 +1,136 @@ +/* + * xen/drivers/char/dt-uart.c + * + * Generic uart retrieved via the device tree + * + * Julien Grall <julien.grall@linaro.org> + * Copyright (c) 2013 Linaro Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <asm/device.h> +#include <asm/types.h> +#include <xen/console.h> +#include <xen/device_tree.h> +#include <xen/serial.h> +#include <xen/errno.h> +#include <xen/acpi.h> + +/* + * Configure UART port with a string: + * path:options + * + * @path: full path used in the device tree for the UART. If the path + * doesn't start with '/', we assuming that it's an alias. + * @options: UART speficic options (see in each UART driver) + */ +static char __initdata opt_dtuart[256] = ""; +string_param("dtuart", opt_dtuart); + +static void __init dt_uart_init(void) +{ + struct dt_device_node *dev; + int ret; + const char *devpath = opt_dtuart; + char *options; + + if ( !console_has("dtuart") ) + return; /* Not for us */ + + if ( !strcmp(opt_dtuart, "") ) + { + const struct dt_device_node *chosen = dt_find_node_by_path("/chosen"); + + if ( chosen ) + { + const char *stdout; + + ret = dt_property_read_string(chosen, "stdout-path", &stdout); + if ( ret >= 0 ) + { + printk("Taking dtuart configuration from /chosen/stdout-path\n"); + if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart)) + >= sizeof(opt_dtuart) ) + printk("WARNING: /chosen/stdout-path too long, truncated\n"); + } + else if ( ret != -EINVAL /* Not present */ ) + printk("Failed to read /chosen/stdout-path (%d)\n", ret); + } + } + + if ( !strcmp(opt_dtuart, "") ) + { + printk("No dtuart path configured\n"); + return; + } + + options = strchr(opt_dtuart, ':'); + if ( options != NULL ) + *(options++) = '\0'; + else + options = ""; + + printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options); + if ( *devpath == '/' ) + dev = dt_find_node_by_path(devpath); + else + dev = dt_find_node_by_alias(devpath); + + if ( !dev ) + { + printk("Unable to find device \"%s\"\n", devpath); + return; + } + + ret = device_init(dev, DEVICE_SERIAL, options); + + if ( ret ) + printk("Unable to initialize dtuart: %d\n", ret); +} + +static void __init acpi_uart_init(void) +{ +#ifdef CONFIG_ACPI + struct acpi_table_spcr *spcr=NULL; + int ret; + + acpi_get_table(ACPI_SIG_SPCR, 0,(struct acpi_table_header **)&spcr); + + if ( spcr == NULL ) + printk("Unable to get spcr table\n"); + else + { + ret = acpi_device_init(DEVICE_SERIAL, NULL, spcr->interface_type); + + if ( ret ) + printk("Unable to initialize acpi uart: %d\n", ret); + } +#endif +} + +void __init uart_init(void) +{ + if ( acpi_disabled ) + dt_uart_init(); + else + acpi_uart_init(); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */