[RFC PATCH 1/3] x86/boot/e820: Make __e820__mapped_all() external

[RFC PATCH 1/3] x86/boot/e820: Make __e820__mapped_all() external

[Aaron Tomlin]

No functional change.

This patch allows __e820__mapped_all() to be available for
external use, in preparation to enhance the error message
generated by arch_rmrr_sanity_check().

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 arch/x86/include/asm/e820/api.h | 1 +
 arch/x86/kernel/e820.c          | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..bf78daa08897 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -13,6 +13,7 @@ extern unsigned long pci_mem_start;
 extern bool e820__mapped_raw_any(u64 start, u64 end, enum e820_type type);
 extern bool e820__mapped_any(u64 start, u64 end, enum e820_type type);
 extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
+struct e820_entry *__e820__mapped_all(u64 start, u64 end, enum e820_type type);
 
 extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index f267205f2d5a..09b1c863a766 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -109,7 +109,7 @@ EXPORT_SYMBOL_GPL(e820__mapped_any);
  * Note: this function only works correctly once the E820 table is sorted and
  * not-overlapping (at least for the range specified), which is the case normally.
  */
-static struct e820_entry *__e820__mapped_all(u64 start, u64 end,
+struct e820_entry *__e820__mapped_all(u64 start, u64 end,
 					     enum e820_type type)
 {
 	int i;
-- 
2.34.1

[RFC PATCH 2/3] x86/boot/e820: Make e820_type_to_string() external

[Aaron Tomlin]

No functional change.

This patch allows e820_type_to_string() to be available for
external use, in preparation to enhance the error message
generated by arch_rmrr_sanity_check().

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 arch/x86/include/asm/e820/api.h | 1 +
 arch/x86/kernel/e820.c          | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index bf78daa08897..ceb301e591de 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -43,6 +43,7 @@ extern void e820__reallocate_tables(void);
 extern void e820__register_nosave_regions(unsigned long limit_pfn);
 
 extern int  e820__get_entry_type(u64 start, u64 end);
+const char *e820_type_to_string(struct e820_entry *entry);
 
 /*
  * Returns true iff the specified range [start,end) is completely contained inside
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 09b1c863a766..95b994cf80cd 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1071,7 +1071,7 @@ void __init e820__finish_early_params(void)
 	}
 }
 
-static const char *__init e820_type_to_string(struct e820_entry *entry)
+const char *__init e820_type_to_string(struct e820_entry *entry)
 {
 	switch (entry->type) {
 	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
-- 
2.34.1

[RFC PATCH 3/3] iommu/vt-d: Show region type in arch_rmrr_sanity_check()

[Aaron Tomlin]

This patch will attempt to describe the region type in the event
that a given RMRR entry is not within a reserved region.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 arch/x86/include/asm/iommu.h | 9 ++++++---
 arch/x86/kernel/e820.c       | 5 +++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index bf1ed2ddc74b..d21366644520 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -17,12 +17,15 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
 {
 	u64 start = rmrr->base_address;
 	u64 end = rmrr->end_address + 1;
+	struct e820_entry *entry;
 
-	if (e820__mapped_all(start, end, E820_TYPE_RESERVED))
+	entry = __e820__mapped_all(start, end, 0);
+
+	if (entry && entry->type == E820_TYPE_RESERVED)
 		return 0;
 
-	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
-	       start, end - 1);
+	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%s: %#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
+	       e820_type_to_string(entry), start, end - 1);
 	return -EINVAL;
 }
 
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 95b994cf80cd..165e9a444bb9 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1073,7 +1073,7 @@ void __init e820__finish_early_params(void)
 
 const char *__init e820_type_to_string(struct e820_entry *entry)
 {
-	switch (entry->type) {
+	switch (entry && entry->type) {
 	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
 	case E820_TYPE_RAM:		return "System RAM";
 	case E820_TYPE_ACPI:		return "ACPI Tables";
@@ -1083,8 +1083,9 @@ const char *__init e820_type_to_string(struct e820_entry *entry)
 	case E820_TYPE_PMEM:		return "Persistent Memory";
 	case E820_TYPE_RESERVED:	return "Reserved";
 	case E820_TYPE_SOFT_RESERVED:	return "Soft Reserved";
-	default:			return "Unknown E820 type";
+	default:			break;
 	}
+	return "Unknown E820 type";
 }
 
 static unsigned long __init e820_type_to_iomem_type(struct e820_entry *entry)
-- 
2.34.1

Re: [RFC PATCH 3/3] iommu/vt-d: Show region type in arch_rmrr_sanity_check()

[Aaron Tomlin]

On Sat 2022-06-11 21:48 +0100, Aaron Tomlin wrote:
> This patch will attempt to describe the region type in the event
> that a given RMRR entry is not within a reserved region.

Any thoughts?


Kind regards,

-- 
Aaron Tomlin

Re: [RFC PATCH 3/3] iommu/vt-d: Show region type in arch_rmrr_sanity_check()

[Robin Murphy]

On 2022-06-11 21:48, Aaron Tomlin wrote:
> This patch will attempt to describe the region type in the event
> that a given RMRR entry is not within a reserved region.

Hmm, is this useful information for the user? You'd hope the firmware 
vendor knows the memory map already, but either way, is it particularly 
likely that anyone would be noticing and caring about this warning in a 
context where they couldn't just scroll further up the log and 
cross-reference the full memory map listing? If so, it might be worth 
clarifying what that use-case is, since as it stands there doesn't seem 
to be much justification for the "why" here.

> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   arch/x86/include/asm/iommu.h | 9 ++++++---
>   arch/x86/kernel/e820.c       | 5 +++--
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
> index bf1ed2ddc74b..d21366644520 100644
> --- a/arch/x86/include/asm/iommu.h
> +++ b/arch/x86/include/asm/iommu.h
> @@ -17,12 +17,15 @@ arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
>   {
>   	u64 start = rmrr->base_address;
>   	u64 end = rmrr->end_address + 1;
> +	struct e820_entry *entry;
>   
> -	if (e820__mapped_all(start, end, E820_TYPE_RESERVED))
> +	entry = __e820__mapped_all(start, end, 0);
> +
> +	if (entry && entry->type == E820_TYPE_RESERVED)
>   		return 0;
>   
> -	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
> -	       start, end - 1);
> +	pr_err(FW_BUG "No firmware reserved region can cover this RMRR [%s: %#018Lx-%#018Lx], contact BIOS vendor for fixes\n",
> +	       e820_type_to_string(entry), start, end - 1);
>   	return -EINVAL;
>   }
>   
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 95b994cf80cd..165e9a444bb9 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -1073,7 +1073,7 @@ void __init e820__finish_early_params(void)
>   
>   const char *__init e820_type_to_string(struct e820_entry *entry)
>   {
> -	switch (entry->type) {
> +	switch (entry && entry->type) {

Have you tested this for anything other than E820_TYPE_RAM? I think 
sufficiently up-to-date compilers should warn you here anyway.

Robin.

>   	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
>   	case E820_TYPE_RAM:		return "System RAM";
>   	case E820_TYPE_ACPI:		return "ACPI Tables";
> @@ -1083,8 +1083,9 @@ const char *__init e820_type_to_string(struct e820_entry *entry)
>   	case E820_TYPE_PMEM:		return "Persistent Memory";
>   	case E820_TYPE_RESERVED:	return "Reserved";
>   	case E820_TYPE_SOFT_RESERVED:	return "Soft Reserved";
> -	default:			return "Unknown E820 type";
> +	default:			break;
>   	}
> +	return "Unknown E820 type";
>   }
>   
>   static unsigned long __init e820_type_to_iomem_type(struct e820_entry *entry)

Re: [RFC PATCH 3/3] iommu/vt-d: Show region type in arch_rmrr_sanity_check()

[Aaron Tomlin]

On Mon 2022-07-04 11:39 +0100, Robin Murphy wrote:
> On 2022-06-11 21:48, Aaron Tomlin wrote:
> > This patch will attempt to describe the region type in the event
> > that a given RMRR entry is not within a reserved region.
> 
> Hmm, is this useful information for the user? You'd hope the firmware vendor
> knows the memory map already, but either way, is it particularly likely that
> anyone would be noticing and caring about this warning in a context where
> they couldn't just scroll further up the log and cross-reference the full
> memory map listing? If so, it might be worth clarifying what that use-case
> is, since as it stands there doesn't seem to be much justification for the
> "why" here.

Hi Robin,

Thanks for looking at this.

Honestly, the only justification for the modification/or proposed changes
is to have more insight when this statement is provided in total isolation
and the RAM map listing (as per e820__print_table()) is no longer available
to reference.

> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index 95b994cf80cd..165e9a444bb9 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -1073,7 +1073,7 @@ void __init e820__finish_early_params(void)
> >   const char *__init e820_type_to_string(struct e820_entry *entry)
> >   {
> > -	switch (entry->type) {
> > +	switch (entry && entry->type) {
> 
> Have you tested this for anything other than E820_TYPE_RAM? I think
> sufficiently up-to-date compilers should warn you here anyway.

I have not.


Kind regards,

-- 
Aaron Tomlin

Re: [RFC PATCH 1/3] x86/boot/e820: Make __e820__mapped_all() external

[Aaron Tomlin]

On Sat 2022-06-11 21:48 +0100, Aaron Tomlin wrote:
> No functional change.
> 
> This patch allows __e820__mapped_all() to be available for
> external use, in preparation to enhance the error message
> generated by arch_rmrr_sanity_check().
> 

Any more feedback?


Kind regards,

-- 
Aaron Tomlin

Re: [RFC PATCH 1/3] x86/boot/e820: Make __e820__mapped_all() external

[Aaron Tomlin]

On Thu 2022-09-29 09:25 +0100, Aaron Tomlin wrote:
> On Sat 2022-06-11 21:48 +0100, Aaron Tomlin wrote:
> > No functional change.
> > 
> > This patch allows __e820__mapped_all() to be available for
> > external use, in preparation to enhance the error message
> > generated by arch_rmrr_sanity_check().
> > 
> 
> Any more feedback?

[Adding iommu@lists.linux.dev and charlotte@extrahop.com on Cc]

-- 
Aaron Tomlin