From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Al Stone <ahs3@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Al Stone <al.stone@linaro.org>,
        linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linaro-kernel@lists.linaro.org, jason@lakedaemon.net,
        linaro-acpi@lists.linaro.org, patches@linaro.org,
        linux-kernel@vger.kernel.org, will.deacon@arm.com, tglx@linutronix.de,
        lenb@kernel.org
Subject: Re: [PATCH v3 2/3] ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro
Date: Sat, 04 Jul 2015 01:54:15 +0200
Message-ID: <15297999.EvgjakjXbx@vostro.rjw.lan>
User-Agent: KMail/4.11.5 (Linux/4.1.0-rc5+; KDE/4.11.5; x86_64; ; )
In-Reply-To: <5596E7C8.7040809@redhat.com>
References: <1435880916-2153-1-git-send-email-al.stone@linaro.org> <20150703140601.GL25907@e104818-lin.cambridge.arm.com> <5596E7C8.7040809@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3482
Lines: 93

On Friday, July 03, 2015 01:51:36 PM Al Stone wrote:
> On 07/03/2015 08:06 AM, Catalin Marinas wrote:
> > On Thu, Jul 02, 2015 at 05:48:35PM -0600, Al Stone wrote:
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index 39248d3..a3c26a4 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -19,6 +19,17 @@
> >>  #include <asm/psci.h>
> >>  #include <asm/smp_plat.h>
> >>  
> >> +/* Macros for consistency checks of the GICC subtable of MADT */
> >> +#define ACPI_MADT_GICC_51_LENGTH       76
> >> +#define ACPI_MADT_GICC_60_LENGTH       80
> >> +
> >> +#define BAD_MADT_GICC_ENTRY(entry, end) (                                   \
> >> +		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_51) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_51_LENGTH))    ||  \
> >> +		((ACPI_FADT_SPEC_VERSION == ACPI_FADT_SPEC_VERSION_60) &&   \
> >> +		 (entry->header.length != ACPI_MADT_GICC_60_LENGTH)))
> > 
> > This looks ugly but, well, we could live with this.
> 
> Nod.  It's right at the hairy edge of becoming a function, I think.
> 
> > However, I'd like to avoid having to extend this macro every time we get
> > a new spec released, like 6.1 defining another 80 or 84 etc. So, how
> > about we only update this when there is an actual change in the length?
> > Something like:
> > 
> > #define ACPI_MADT_GICC_LENGTH	({					\
> > 	u8 length;							\
> > 	if (ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0)	\
> > 		length = 76;						\
> > 	else								\
> > 		length = 80;						\
> > 	length;								\
> > })
> > 
> > or just:
> > 
> > #define ACPI_MADT_GICC_LENGTH	\
> > 	(ACPI_FADT_SPEC_VERSION < ACPI_FADT_SPEC_VERSION_6_0 ? 76 : 80)
> > 
> > (the latter is simpler but may not look nice if we change it again in
> > 6.1; though we could re-write this macro when needed, not a problem)
> > 
> 
> Perhaps the sanity checking for the MADT subtables needs to be revisited
> and a more general solution provided -- this is not the only MADT subtable
> with this problem and it may occur again.
> 
> Even the versions above are not technically compliant with the spec.  If
> we implement what the spec currently says, it might look something like
> this:
> 
> #define ACPI_MADT_GICC_LENGTH ({					\
> 	u8 length;							\
> 	switch (ACPI_FADT_SPEC_VERSION) {				\
> 	case ACPI_FADT_SPEC_VERSION_5_0:				\
> 		length = 40;						\
> 		break;							\
> 	case ACPI_FADT_SPEC_VERSION_5_1:				\
> 		length = 76;						\
> 		break;							\
> 	default:	/* use 6.0 size */				\
> 		length = 80;						\
> 	}								\
> 	length;								\
> })
> 
> So it's just messy and there will be a need for change.  Let me think about
> making this a function instead of a macro; it may make sense to really fix
> BAD_MADT_ENTRY in general instead of just dealing with the GICC subtable,
> but it could also be overkill.

So here's my suggestion.

First, make ARM64 boot with 4.2+ in the simplest way possible.

Second, set out to fix BAD_MADT_ENTRY() etc.  Start with fixing ACPICA to
distinguish between the different formats depending on the spec version and
follow up from there.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/