On 20 May 2015 at 23:33, Julien Grall <julien.gr...@citrix.com> 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.b...@linaro.org>
> > Signed-off-by: Parth Dixit <parth.di...@linaro.org>
> > ---
> > 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
> **)>dt);
> > +
> > + 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@lists.xen.org
http://lists.xen.org/xen-devel
