Re: [PATCH v7 20/25] ACPI / APEI: Use separate fixmap pages for arm64 NMI-like notifications
On Wed, Jan 23, 2019 at 06:33:02PM +, James Morse wrote:
> Was the best I had, but this trips the BUILD_BUG() too early.
> With it, x86 BUILD_BUG()s. With just the -1 the path gets pruned out, and
> there
> are no 'sdei' symbols in the object file.
>
> ...at this point, I stopped caring!
Yah, you said it: __end_of_fixed_addresses will practically give you the
BUG behavior:
if (idx >= __end_of_fixed_addresses) {
BUG();
return;
}
and ARM64 does the same.
> We already skip registering notifiers if the kconfig option wasn't selected.
>
> We can't catch this at compile time, as the dead-code elimination seems to
> happen in multiple passes.
>
> I'll switch the SDEI ones to __end_of_fixed_addresses, as both architectures
> BUG() when they see this.
Right.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 20/25] ACPI / APEI: Use separate fixmap pages for arm64 NMI-like notifications
Hi Boris,
On 21/01/2019 17:27, Borislav Petkov wrote:
> On Mon, Dec 03, 2018 at 06:06:08PM +, James Morse wrote:
>> Now that ghes notification helpers provide the fixmap slots and
>> take the lock themselves, multiple NMI-like notifications can
>> be used on arm64.
>>
>> These should be named after their notification method as they can't
>> all be called 'NMI'. x86's NOTIFY_NMI already is, change the SEA
>> fixmap entry to be called FIX_APEI_GHES_SEA.
>>
>> Future patches can add support for FIX_APEI_GHES_SEI and
>> FIX_APEI_GHES_SDEI_{NORMAL,CRITICAL}.
>>
>> Because all of ghes.c builds on both architectures, provide a
>> constant for each fixmap entry that the architecture will never
>> use.
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 849da0d43a21..6cbf9471b2a2 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -85,6 +85,14 @@
>> ((struct acpi_hest_generic_status *)\
>> ((struct ghes_estatus_node *)(estatus_node) + 1))
>>
>> +/* NMI-like notifications vary by architecture. Fill in the fixmap gaps */
>> +#ifndef CONFIG_HAVE_ACPI_APEI_NMI
>> +#define FIX_APEI_GHES_NMI -1
>> +#endif
>> +#ifndef CONFIG_ACPI_APEI_SEA
>> +#define FIX_APEI_GHES_SEA -1
>
> I'm guessing those -1 are going to cause __set_fixmap() to fail, right?
It shouldn't be possible, these are just to give the compiler something int
shaped to work with, until it prunes all the callers.
But for arm64, yes if would fail. -1 shouldn't alias an existing entry, and it
will get caught by:
| BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
I wanted BUILD_BUG_ON() here, as any user of these should be optimised out, but
the compiler choked on that.
__end_of_fixed_addresses would be a better arch-agnostic invalid value. It has
to be defined as the last value in the enum for core code's fix_to_virt() to
work.
These two look like something left behind from when we had different #ifdeffery.
The users of these two are now behind arch specific #ifdefs that since patch 12
of this series, can't be turned off, so I can remove these.
We do need them for SDEI, as it is relying on IS_ENABLED() and the compiler's
dead code elimination. But the compiler wants that symbol to have the right type
before it gets that far.
|#define FIX_APEI_GHES_SDEI_NORMAL (BUILD_BUG(), -1)
Was the best I had, but this trips the BUILD_BUG() too early.
With it, x86 BUILD_BUG()s. With just the -1 the path gets pruned out, and there
are no 'sdei' symbols in the object file.
...at this point, I stopped caring!
> I'm wondering if we could catch that situation in ghes_map() already to
> protect ourselves against future changes in the fixmap code...
We already skip registering notifiers if the kconfig option wasn't selected.
We can't catch this at compile time, as the dead-code elimination seems to
happen in multiple passes.
I'll switch the SDEI ones to __end_of_fixed_addresses, as both architectures
BUG() when they see this.
Thanks,
James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 20/25] ACPI / APEI: Use separate fixmap pages for arm64 NMI-like notifications
On Mon, Dec 03, 2018 at 06:06:08PM +, James Morse wrote:
> Now that ghes notification helpers provide the fixmap slots and
> take the lock themselves, multiple NMI-like notifications can
> be used on arm64.
>
> These should be named after their notification method as they can't
> all be called 'NMI'. x86's NOTIFY_NMI already is, change the SEA
> fixmap entry to be called FIX_APEI_GHES_SEA.
>
> Future patches can add support for FIX_APEI_GHES_SEI and
> FIX_APEI_GHES_SDEI_{NORMAL,CRITICAL}.
>
> Because all of ghes.c builds on both architectures, provide a
> constant for each fixmap entry that the architecture will never
> use.
>
> Signed-off-by: James Morse
>
> ---
> Changes since v6:
> * Added #ifdef definitions of each missing fixmap entry.
>
> Changes since v3:
> * idx/lock are now in a separate struct.
> * Add to the comment above ghes_fixmap_lock_irq so that it makes more
>sense in isolation.
>
> fixup for split fixmap
> ---
> arch/arm64/include/asm/fixmap.h | 2 +-
> drivers/acpi/apei/ghes.c| 10 +-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> index ec1e6d6fa14c..966dd4bb23f2 100644
> --- a/arch/arm64/include/asm/fixmap.h
> +++ b/arch/arm64/include/asm/fixmap.h
> @@ -55,7 +55,7 @@ enum fixed_addresses {
> #ifdef CONFIG_ACPI_APEI_GHES
> /* Used for GHES mapping from assorted contexts */
> FIX_APEI_GHES_IRQ,
> - FIX_APEI_GHES_NMI,
> + FIX_APEI_GHES_SEA,
> #endif /* CONFIG_ACPI_APEI_GHES */
>
> #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 849da0d43a21..6cbf9471b2a2 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -85,6 +85,14 @@
> ((struct acpi_hest_generic_status *)\
>((struct ghes_estatus_node *)(estatus_node) + 1))
>
> +/* NMI-like notifications vary by architecture. Fill in the fixmap gaps */
> +#ifndef CONFIG_HAVE_ACPI_APEI_NMI
> +#define FIX_APEI_GHES_NMI-1
> +#endif
> +#ifndef CONFIG_ACPI_APEI_SEA
> +#define FIX_APEI_GHES_SEA-1
I'm guessing those -1 are going to cause __set_fixmap() to fail, right?
I'm wondering if we could catch that situation in ghes_map() already to
protect ourselves against future changes in the fixmap code...
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v7 20/25] ACPI / APEI: Use separate fixmap pages for arm64 NMI-like notifications
Now that ghes notification helpers provide the fixmap slots and
take the lock themselves, multiple NMI-like notifications can
be used on arm64.
These should be named after their notification method as they can't
all be called 'NMI'. x86's NOTIFY_NMI already is, change the SEA
fixmap entry to be called FIX_APEI_GHES_SEA.
Future patches can add support for FIX_APEI_GHES_SEI and
FIX_APEI_GHES_SDEI_{NORMAL,CRITICAL}.
Because all of ghes.c builds on both architectures, provide a
constant for each fixmap entry that the architecture will never
use.
Signed-off-by: James Morse
---
Changes since v6:
* Added #ifdef definitions of each missing fixmap entry.
Changes since v3:
* idx/lock are now in a separate struct.
* Add to the comment above ghes_fixmap_lock_irq so that it makes more
sense in isolation.
fixup for split fixmap
---
arch/arm64/include/asm/fixmap.h | 2 +-
drivers/acpi/apei/ghes.c| 10 +-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
index ec1e6d6fa14c..966dd4bb23f2 100644
--- a/arch/arm64/include/asm/fixmap.h
+++ b/arch/arm64/include/asm/fixmap.h
@@ -55,7 +55,7 @@ enum fixed_addresses {
#ifdef CONFIG_ACPI_APEI_GHES
/* Used for GHES mapping from assorted contexts */
FIX_APEI_GHES_IRQ,
- FIX_APEI_GHES_NMI,
+ FIX_APEI_GHES_SEA,
#endif /* CONFIG_ACPI_APEI_GHES */
#ifdef CONFIG_UNMAP_KERNEL_AT_EL0
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 849da0d43a21..6cbf9471b2a2 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -85,6 +85,14 @@
((struct acpi_hest_generic_status *)\
((struct ghes_estatus_node *)(estatus_node) + 1))
+/* NMI-like notifications vary by architecture. Fill in the fixmap gaps */
+#ifndef CONFIG_HAVE_ACPI_APEI_NMI
+#define FIX_APEI_GHES_NMI -1
+#endif
+#ifndef CONFIG_ACPI_APEI_SEA
+#define FIX_APEI_GHES_SEA -1
+#endif
+
static inline bool is_hest_type_generic_v2(struct ghes *ghes)
{
return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
@@ -954,7 +962,7 @@ int ghes_notify_sea(void)
int rv;
raw_spin_lock(_notify_lock_sea);
- rv = ghes_estatus_queue_notified(_sea, FIX_APEI_GHES_NMI);
+ rv = ghes_estatus_queue_notified(_sea, FIX_APEI_GHES_SEA);
raw_spin_unlock(_notify_lock_sea);
return rv;
--
2.19.2
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
