Re: [Xen-devel] [PATCH v2 15/41] arm : acpi parse GTDT to initialize timer

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 20 May 2015 at 23:33, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

Hi Parth,

On 17/05/15 21:03, Parth Dixit wrote:
> Parse GTDT (Generic Timer Descriptor Table) to initialize timer.
> Using the information presented by GTDT to initialize the arch
> timer (not memory-mapped).
>
> Clear all el2 fields in GTDT table after initialization
> for passing it to Dom0.
>
> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
> xen/arch/arm/acpi/boot.c Â| 50 ++++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/time.c    | 38 +++++++++++++++++++++++++----------
>Â xen/include/asm-arm/acpi.h |Â 2 ++
>Â 3 files changed, 79 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 7d9758f..a8311ae 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -30,6 +30,8 @@
>
>Â #include <asm/acpi.h>
>Â #include <asm/smp.h>
> +#include <asm/irq.h>
> +#include <asm/time.h>
>
>Â /* Processors with enabled flag and sane MPIDR */
>Â static int enabled_cpus;
> @@ -40,6 +42,54 @@ static bool_t __initdata bootcpu_valid;
>Â /* arch-optional setting to enable display of offline cpus >= nr_cpu_ids */
>Â static unsigned int total_cpus = 0;
>
> +/* Initialize per-processor generic timer */
> +void __init acpi_preinit_xen_time(unsigned int generic_timer_irq[])

acpi/boot.c begin to be a junk room for all ACPI code. It would have
been more logic to have this code in arch/arm/time.c with proper #ifdef.

This would have drop the generic_timer_irq which is difficult to understand.

> +{
> +Â Â int type;
> +Â Â struct acpi_table_gtdt *gtdt=NULL;

*gtdt = NULL;

> +Â Â u8 checksum;
> +Â Â static const int GTDT_INTRL_TAB[] =

all uppercase name are used for define.

> +Â Â {
> +Â Â Â Â ACPI_IRQ_TYPE_LEVEL_HIGH,
> +Â Â Â Â ACPI_IRQ_TYPE_EDGE_RISING,
> +Â Â Â Â ACPI_IRQ_TYPE_LEVEL_LOW,
> +Â Â Â Â ACPI_IRQ_TYPE_EDGE_FALLING
> +Â Â };

It took me a while to understand how this work.

I would prefer an helper which check each field and return/set the
correct interrupt type.

> +
> +Â Â acpi_get_table(ACPI_SIG_GTDT, 0, (struct acpi_table_header **)&gtdt);
> +
> +Â Â if( gtdt )
> +Â Â {

If gtdt is NULL you will continue without any warning and potentially
crash later.

Also please do:

if ( !gtdt ) {
 /* handle error */
 return;
}

/* setup the timer */

It's easier to understand and remove one indentation.

> +Â Â Â Â /* Initialize all the generic timer IRQ variable from GTDT table */
> +
> +Â Â Â Â type = GTDT_INTRL_TAB[gtdt->non_secure_el1_flags & ACPI_GTDT_INTR_MASK];
> +Â Â Â Â set_irq_type(gtdt->non_secure_el1_interrupt, type);
> +Â Â Â Â generic_timer_irq[TIMER_PHYS_NONSECURE_PPI] =
> +Â Â Â Â Â Â gtdt->non_secure_el1_interrupt;
> +
> +Â Â Â Â type = GTDT_INTRL_TAB[gtdt->secure_el1_flags & ACPI_GTDT_INTR_MASK];
> +Â Â Â Â set_irq_type(gtdt->secure_el1_interrupt, type);
> +Â Â Â Â generic_timer_irq[TIMER_PHYS_SECURE_PPI] =
> +Â Â Â Â Â Â gtdt->secure_el1_interrupt;
> +
> +Â Â Â Â type = GTDT_INTRL_TAB[gtdt->non_secure_el2_flags & ACPI_GTDT_INTR_MASK];
> +Â Â Â Â set_irq_type(gtdt->non_secure_el2_interrupt, type);
> +Â Â Â Â generic_timer_irq[TIMER_HYP_PPI] =
> +Â Â Â Â Â Â gtdt->non_secure_el2_interrupt;
> +
> +Â Â Â Â type = GTDT_INTRL_TAB[gtdt->virtual_timer_flags & ACPI_GTDT_INTR_MASK];
> +Â Â Â Â set_irq_type(gtdt->virtual_timer_interrupt, type);
> +Â Â Â Â generic_timer_irq[TIMER_VIRT_PPI] =
> +Â Â Â Â Â Â gtdt->virtual_timer_interrupt;
> +
> +Â Â Â Â gtdt->non_secure_el2_interrupt = 0;
> +Â Â Â Â gtdt->non_secure_el2_flags = 0;
> +Â Â Â Â checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, gtdt), gtdt->header.length);
> +Â Â Â Â gtdt->header.checksum -= checksum;
> +Â Â Â Â clean_dcache_va_range(gtdt, sizeof(struct acpi_table_gtdt));

Similar comment as on the MADT, this function is initializing the timer
and not modifying the ACPI table.

I would prefer a separate function which is called during DOM0 building.
It would be easier to know how the ACPI table has been modified for DOM0.

Also, please add a comment to explain this block smth like /* Hide EL2
interrupt to DOM0 */. Maybe explaining that the table is re-used.

Furthermore, are you sure that the ACPI table will always be in R/W
memory? If not, we should duplicate the table and update the field.

> +Â Â }
> +}
> +
>Â /*
>Â Â* acpi_map_gic_cpu_interface - generates a logical cpu number
>Â Â* and map to MPIDR represented by GICC structure
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index ce6d3fd..bff95ab 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -29,6 +29,7 @@
>Â #include <xen/time.h>
>Â #include <xen/sched.h>
>Â #include <xen/event.h>
> +#include <xen/acpi.h>
>Â #include <asm/system.h>
>Â #include <asm/time.h>
>Â #include <asm/gic.h>
> @@ -64,7 +65,7 @@ unsigned int timer_get_irq(enum timer_ppi ppi)
>Â static __initdata struct dt_device_node *timer;
>
>Â /* Set up the timer on the boot CPU (early init function) */
> -void __init preinit_xen_time(void)
> +static void __init dt_preinit_xen_time(void)
>Â {
>Â Â Â static const struct dt_device_match timer_ids[] __initconst =
>Â Â Â {
> @@ -72,7 +73,6 @@ void __init preinit_xen_time(void)
>Â Â Â Â Â { /* sentinel */ },
>Â Â Â };
>Â Â Â int res;
> -Â Â u32 rate;
>
>Â Â Â timer = dt_find_matching_node(NULL, timer_ids);
>Â Â Â if ( !timer )
> @@ -84,8 +84,21 @@ void __init preinit_xen_time(void)
>Â Â Â if ( res )
>Â Â Â Â Â panic("Timer: Cannot initialize platform timer");
>
> -Â Â res = dt_property_read_u32(timer, "clock-frequency", &rate);
> -Â Â if ( res )
> +}
> +
> +
> +

Only one blank line is enough.

> +void __init preinit_xen_time(void)
> +{
> +Â Â u32 rate;
> +
> +Â Â /* Initialize all the generic timers presented in GTDT */
> +Â Â if ( acpi_disabled )
> +Â Â Â Â dt_preinit_xen_time();
> +Â Â else
> +Â Â Â Â acpi_preinit_xen_time(timer_irq);
> +
> +Â Â if( acpi_disabled && dt_property_read_u32(timer, "clock-frequency", &rate) )

A such check calls for more refactoring...

>Â Â Â Â Â cpu_khz = rate / 1000;
>Â Â Â else
>Â Â Â Â Â cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;

Duplicating this line wouldn't have been too bad compare to the check.

> @@ -99,14 +112,17 @@ int __init init_xen_time(void)
>Â Â Â int res;
>Â Â Â unsigned int i;
>
> -Â Â /* Retrieve all IRQs for the timer */
> -Â Â for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> +Â Â if( acpi_disabled )
>Â Â Â {
> -Â Â Â Â res = platform_get_irq(timer, i);
> -
> -Â Â Â Â if ( res < 0 )
> -Â Â Â Â Â Â panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> -Â Â Â Â timer_irq[i] = res;
> +Â Â Â Â /* Retrieve all IRQs for the timer */
> +Â Â Â Â for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> +Â Â Â Â {
> +Â Â Â Â Â Â res = platform_get_irq(timer, i);
> +
> +Â Â Â Â Â Â if ( res < 0 )
> +Â Â Â Â Â Â Â Â panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
> +Â Â Â Â Â Â timer_irq[i] = res;
> +Â Â Â Â }

Another helper for this device tree code?

>Â Â Â }
>
>Â Â Â /* Check that this CPU supports the Generic Timer interface */
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 1767143..482cc5b 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -36,10 +36,12 @@ bool_t acpi_psci_present(void);
>Â /* 1 to indicate HVC is present instead of SMC as the PSCI conduit */
>Â bool_t acpi_psci_hvc_present(void);
>Â void __init acpi_init_cpus(void);
> +void __init acpi_preinit_xen_time(unsigned int generic_timer_irq[]);
>Â #else
>Â static inline bool_t acpi_psci_present(void) { return false; }
>Â static inline bool_t acpi_psci_hvc_present(void) {return false; }
>Â static inline void acpi_init_cpus(void) { }
> +static inline void acpi_preinit_xen_time(unsigned int generic_timer_irq[]){ }
>Â #endif /* CONFIG_ACPI */
>
>Â /* Basic configuration for ACPI */
>

sure, will take care in next patchset.

Regards,

--
Julien Grall

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