VT-d RMRR (Reserved Memory Region Reporting) regions are reserved
for device use only and should not be part of allocable memory pool of OS.
BIOS e820_table reports complete memory map to OS, including OS usable
memory ranges and BIOS reserved memory ranges etc.
x86 BIOS may not be trusted to include RMRR regions as reserved type
of memory in its e820 memory map, hence validate every RMRR entry
with the e820 memory map to make sure the RMRR regions will not be
used by OS for any other purposes.
ia64 EFI is working fine so implement RMRR validation as a dummy function
Reviewed-by: Sohil Mehta <[email protected]>
Signed-off-by: Yian Chen <[email protected]>
---
arch/ia64/include/asm/iommu.h | 5 +++++
arch/x86/include/asm/iommu.h | 18 ++++++++++++++++++
drivers/iommu/intel-iommu.c | 8 +++++++-
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
index 7904f591a79b..eb0db20c9d4c 100644
--- a/arch/ia64/include/asm/iommu.h
+++ b/arch/ia64/include/asm/iommu.h
@@ -2,6 +2,8 @@
#ifndef _ASM_IA64_IOMMU_H
#define _ASM_IA64_IOMMU_H 1
+#include <linux/acpi.h>
+
/* 10 seconds */
#define DMAR_OPERATION_TIMEOUT (((cycles_t) local_cpu_data->itc_freq)*10)
@@ -9,6 +11,9 @@ extern void no_iommu_init(void);
#ifdef CONFIG_INTEL_IOMMU
extern int force_iommu, no_iommu;
extern int iommu_detected;
+
+static inline int __init
+arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr) { return 0; }
#else
#define no_iommu (1)
#define iommu_detected (0)
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index b91623d521d9..95fa65a5f0dc 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -2,10 +2,28 @@
#ifndef _ASM_X86_IOMMU_H
#define _ASM_X86_IOMMU_H
+#include <linux/acpi.h>
+
+#include <asm/e820/api.h>
+
extern int force_iommu, no_iommu;
extern int iommu_detected;
/* 10 seconds */
#define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
+static inline int __init
+arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
+{
+ u64 start = rmrr->base_address;
+ u64 end = rmrr->end_address + 1;
+
+ if (e820__mapped_all(start, end, 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);
+ return -EFAULT;
+}
+
#endif /* _ASM_X86_IOMMU_H */
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3f974919d3bd..722290014143 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4306,13 +4306,19 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
{
struct acpi_dmar_reserved_memory *rmrr;
struct dmar_rmrr_unit *rmrru;
+ int ret;
+
+ rmrr = (struct acpi_dmar_reserved_memory *)header;
+ ret = arch_rmrr_sanity_check(rmrr);
+ if (ret)
+ return ret;
rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
if (!rmrru)
goto out;
rmrru->hdr = header;
- rmrr = (struct acpi_dmar_reserved_memory *)header;
+
rmrru->base_address = rmrr->base_address;
rmrru->end_address = rmrr->end_address;
--
2.17.1
Hi,
On 10/16/19 12:49 AM, Yian Chen wrote:
> VT-d RMRR (Reserved Memory Region Reporting) regions are reserved
> for device use only and should not be part of allocable memory pool of OS.
>
> BIOS e820_table reports complete memory map to OS, including OS usable
> memory ranges and BIOS reserved memory ranges etc.
>
> x86 BIOS may not be trusted to include RMRR regions as reserved type
> of memory in its e820 memory map, hence validate every RMRR entry
> with the e820 memory map to make sure the RMRR regions will not be
> used by OS for any other purposes.
>
> ia64 EFI is working fine so implement RMRR validation as a dummy function
>
> Reviewed-by: Sohil Mehta <[email protected]>
> Signed-off-by: Yian Chen <[email protected]>
This patch looks good to me.
Reviewed-by: Lu Baolu <[email protected]>
> ---
> arch/ia64/include/asm/iommu.h | 5 +++++
> arch/x86/include/asm/iommu.h | 18 ++++++++++++++++++
> drivers/iommu/intel-iommu.c | 8 +++++++-
> 3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
> index 7904f591a79b..eb0db20c9d4c 100644
> --- a/arch/ia64/include/asm/iommu.h
> +++ b/arch/ia64/include/asm/iommu.h
> @@ -2,6 +2,8 @@
> #ifndef _ASM_IA64_IOMMU_H
> #define _ASM_IA64_IOMMU_H 1
>
> +#include <linux/acpi.h>
> +
> /* 10 seconds */
> #define DMAR_OPERATION_TIMEOUT (((cycles_t) local_cpu_data->itc_freq)*10)
>
> @@ -9,6 +11,9 @@ extern void no_iommu_init(void);
> #ifdef CONFIG_INTEL_IOMMU
> extern int force_iommu, no_iommu;
> extern int iommu_detected;
> +
> +static inline int __init
> +arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr) { return 0; }
> #else
> #define no_iommu (1)
> #define iommu_detected (0)
> diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
> index b91623d521d9..95fa65a5f0dc 100644
> --- a/arch/x86/include/asm/iommu.h
> +++ b/arch/x86/include/asm/iommu.h
> @@ -2,10 +2,28 @@
> #ifndef _ASM_X86_IOMMU_H
> #define _ASM_X86_IOMMU_H
>
> +#include <linux/acpi.h>
> +
> +#include <asm/e820/api.h>
> +
> extern int force_iommu, no_iommu;
> extern int iommu_detected;
>
> /* 10 seconds */
> #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
>
> +static inline int __init
> +arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
> +{
> + u64 start = rmrr->base_address;
> + u64 end = rmrr->end_address + 1;
> +
> + if (e820__mapped_all(start, end, 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);
> + return -EFAULT;
> +}
> +
> #endif /* _ASM_X86_IOMMU_H */
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3f974919d3bd..722290014143 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4306,13 +4306,19 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
> {
> struct acpi_dmar_reserved_memory *rmrr;
> struct dmar_rmrr_unit *rmrru;
> + int ret;
> +
> + rmrr = (struct acpi_dmar_reserved_memory *)header;
> + ret = arch_rmrr_sanity_check(rmrr);
> + if (ret)
> + return ret;
>
> rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
> if (!rmrru)
> goto out;
>
> rmrru->hdr = header;
> - rmrr = (struct acpi_dmar_reserved_memory *)header;
> +
> rmrru->base_address = rmrr->base_address;
> rmrru->end_address = rmrr->end_address;
>
>
Hi,
On Tue, Oct 15, 2019 at 09:49:32AM -0700, Yian Chen wrote:
> VT-d RMRR (Reserved Memory Region Reporting) regions are reserved
> for device use only and should not be part of allocable memory pool of OS.
>
> BIOS e820_table reports complete memory map to OS, including OS usable
> memory ranges and BIOS reserved memory ranges etc.
>
> x86 BIOS may not be trusted to include RMRR regions as reserved type
> of memory in its e820 memory map, hence validate every RMRR entry
> with the e820 memory map to make sure the RMRR regions will not be
> used by OS for any other purposes.
Are there real systems in the wild where this is a problem?
> +static inline int __init
> +arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
> +{
> + u64 start = rmrr->base_address;
> + u64 end = rmrr->end_address + 1;
> +
> + if (e820__mapped_all(start, end, 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);
> + return -EFAULT;
> +}
Why -EFAULT, there is no fault involved? Usibg -EINVAL seems to be a better choice.
On 10/16/2019 12:51 AM, Joerg Roedel wrote:
> Hi,
>
> On Tue, Oct 15, 2019 at 09:49:32AM -0700, Yian Chen wrote:
>> VT-d RMRR (Reserved Memory Region Reporting) regions are reserved
>> for device use only and should not be part of allocable memory pool of OS.
>>
>> BIOS e820_table reports complete memory map to OS, including OS usable
>> memory ranges and BIOS reserved memory ranges etc.
>>
>> x86 BIOS may not be trusted to include RMRR regions as reserved type
>> of memory in its e820 memory map, hence validate every RMRR entry
>> with the e820 memory map to make sure the RMRR regions will not be
>> used by OS for any other purposes.
> Are there real systems in the wild where this is a problem?
Firmware reports e820 and RMRR in separate structure. The system will
not work stably
if RMRR is not in the e820 table as reserved type mem and can be used
for other purposes.
In system engineering phase, I practiced with some kind bugs from BIOS,
but not yet exactly same as
the one here. Please consider this is a generic patch to avoid
subsequent failure at runtime.
>> +static inline int __init
>> +arch_rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
>> +{
>> + u64 start = rmrr->base_address;
>> + u64 end = rmrr->end_address + 1;
>> +
>> + if (e820__mapped_all(start, end, 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);
>> + return -EFAULT;
>> +}
> Why -EFAULT, there is no fault involved? Usibg -EINVAL seems to be a better choice.
-EFAULT could be used for address related errors.
For this case, I agree, -EINVAL seems better while
consider it as an input problem from firmware. I will make change.
Hi,
On 10/16/19 1:49 AM, Yian Chen wrote:
> VT-d RMRR (Reserved Memory Region Reporting) regions are reserved
> for device use only and should not be part of allocable memory pool of OS.
>
> BIOS e820_table reports complete memory map to OS, including OS usable
> memory ranges and BIOS reserved memory ranges etc.
>
> x86 BIOS may not be trusted to include RMRR regions as reserved type
> of memory in its e820 memory map, hence validate every RMRR entry
> with the e820 memory map to make sure the RMRR regions will not be
> used by OS for any other purposes.
I found "bad RMRR" warnings starting to appear on some x86 servers
since v5.5-rc1 and it gets even louder since v5.6-rc1. The "bad"
RMRR is for the area resides in ACPI NVS memory region. For example,
# dmesg|grep RMRR
DMAR: RMRR base: 0x000000a2290000 end: 0x000000a2292fff
DMAR: [Firmware Bug]: No firmware reserved region can cover this RMRR [0x00000000a2290000-0x00000000a2292fff], contact BIOS vendor for fixes
Your BIOS is broken; bad RMRR [0x00000000a2290000-0x00000000a2292fff]
# dmesg|grep NVS
BIOS-e820: [mem 0x00000000a067a000-0x00000000a2a79fff] ACPI NVS
reserve setup_data: [mem 0x00000000a067a000-0x00000000a2a79fff] ACPI NVS
PM: Registering ACPI NVS region [mem 0xa067a000-0xa2a79fff] (37748736 bytes)
The warnings come from arch_rmrr_sanity_check() since it checks whether
the region is E820_TYPE_RESERVED. However, if the purpose of the check
is to detect RMRR has regions that may be used by OS as free memory,
isn't E820_TYPE_NVS safe, too?
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.