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

2022-07-04 Thread 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2022-07-04 Thread 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 
---
  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)

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2022-07-03 Thread 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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

2022-06-12 Thread 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 
---
 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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu