Re: [PATCH 01/16] ACPI: GTDT: Parse information related to the EL2 virtual timer

[Marc Zyngier <maz@kernel.org>]

Hi Sudeep,

On Wed, 13 May 2026 10:41:51 +0100,
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> 
> On Thu, May 07, 2026 at 01:55:29PM +0100, Marc Zyngier wrote:
> > Since ARMv8.1, the architecture has grown an EL2-private virtual
> > timer. This has been described in ACPI since ACPI v6.3 and revision
> > 3 of the GTDT table.
> > 
> > An aditional structure was added in ACPICA, though in a rather
> > bizarre way, and merged in v5.1 as 8f5a14d053100 ("ACPICA: ACPI 6.3:
> > add GTDT Revision 3 support").
> > 
> > Finally plug the table parsing in GTDT, and allow it to be eventually
> > presented to the architected timer driver.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/acpi/arm64/gtdt.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> > index ffc867bac2d60..1a58007fccf0a 100644
> > --- a/drivers/acpi/arm64/gtdt.c
> > +++ b/drivers/acpi/arm64/gtdt.c
> > @@ -88,6 +88,19 @@ static int __init map_gt_gsi(u32 interrupt, u32 flags)
> >  	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> >  }
> >  
> > +struct gtdt_v3 {
> > +	struct acpi_table_gtdt	gtdt_v2;
> > +	struct acpi_gtdt_el2	el2_vtimer;
> > +};
> > +
> > +static struct acpi_gtdt_el2 *gtdt_to_el2_vtimer(struct acpi_table_gtdt *gtdt)
> > +{
> > +	if (gtdt->header.revision < 3)
> > +		return NULL;
> > +
> 
> We should validate the length as well just in case some platform bumped version
> but didn't add these fields. Again it is problem if the platform timers are
> also missing. We can just add the check in acpi_gtdt_init() if the version >=3
> perhaps? If (gtdt->header.length < sizeof(struct gtdt_v3) invalid table or
> something.

Yeah, that seems like a valuable thing to do.
>
> Also we have in platform_timer_valid()
> 	platform_timer >= (void *)(acpi_gtdt_desc.gtdt + 1)
> which is still valid, but again if someone messes up in v3 keeping
> offset where virtual EL2 timer GSI should be present as they just copied
> it from v2, then we might allow it as valid.
> 
> Do it make sense to add that check as well in acpi_gtdt_init()? I mean
> the offset should be >= sizeof(struct gtdt_v3).
> 
> Both checks can be added to acpi_gtdt_init() to avoid checking for each
> platform timer instance may be ?

That makes some sense to me.

What I'll do is introduce gtdt_v3 in a first patch, making sure that
the parser is correctly handling the difference (pointer arithmetic
and such), and then add the virtual timer interrupt handling in a
subsequent change.

> 
> Other than this couple of minor nit, this patch looks fine.
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@kernel.org>

Thanks for having had a look!

	M.

-- 
Without deviation from the norm, progress is not possible.