[PATCH 1/2] x86/efi: Reserve SMBIOS table region when EFI booting

[PATCH 1/2] x86/efi: Reserve SMBIOS table region when EFI booting

[Ross Lagerwall]

Some EFI firmware implementations may place the SMBIOS table in RAM
marked as BootServicesData, which Xen does not consider as reserved.
When dom0 tries to access the SMBIOS, the region is not contained in the
initial P2M and it crashes with a page fault. To fix this, reserve the
SMBIOS region.

Also, fix the memcmp check for existence of the SMBIOS and the DMI
checksum calculation.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/dmi_scan.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index 187c01e..ec411e6 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -189,6 +189,8 @@ static inline bool_t __init dmi_checksum(const void __iomem *buf,
 
 static u32 __initdata efi_dmi_address;
 static u32 __initdata efi_dmi_size;
+static u32 __initdata efi_smbios_address;
+static u32 __initdata efi_smbios_size;
 static u64 __initdata efi_smbios3_address;
 static u32 __initdata efi_smbios3_size;
 
@@ -209,13 +211,18 @@ void __init dmi_efi_get_table(const void *smbios, const void *smbios3)
 		return;
 	}
 
-	if (eps && memcmp(eps->anchor, "_SM_", 4) &&
+	if (eps && memcmp(eps->anchor, "_SM_", 4) == 0 &&
 	    eps->length >= sizeof(*eps) &&
-	    dmi_checksum(eps, eps->length) &&
-	    memcmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
-	    dmi_checksum(&eps->dmi, sizeof(eps->dmi))) {
-		efi_dmi_address = eps->dmi.address;
-		efi_dmi_size = eps->dmi.size;
+	    dmi_checksum(eps, eps->length)) {
+		efi_smbios_address = (u32)(long)smbios;
+		efi_smbios_size = eps->length;
+
+		if ( memcmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
+		     dmi_checksum(&eps->dmi,
+				  eps->length - offsetof(struct smbios_eps, dmi)) ) {
+			efi_dmi_address = eps->dmi.address;
+			efi_dmi_size = eps->dmi.size;
+		}
 	}
 }
 
@@ -236,6 +243,12 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len)
 			instance |= 2;
 			return "DMI";
 		}
+		if (efi_smbios_size && !(instance & 4)) {
+			*base = efi_smbios_address;
+			*len = efi_smbios_size;
+			instance |= 4;
+			return "SMBIOS";
+		}
 	} else {
 		char __iomem *p = maddr_to_virt(0xF0000), *q;
 		union {
-- 
2.1.0

[PATCH 2/2] x86/efi: Fix memcmp check

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/dmi_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index ec411e6..3749688 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -203,7 +203,7 @@ void __init dmi_efi_get_table(const void *smbios, const void *smbios3)
 	const struct smbios_eps *eps = smbios;
 	const struct smbios3_eps *eps3 = smbios3;
 
-	if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) &&
+	if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) == 0 &&
 	    eps3->length >= sizeof(*eps3) &&
 	    dmi_checksum(eps3, eps3->length)) {
 		efi_smbios3_address = eps3->address;
-- 
2.1.0

Re: [PATCH 2/2] x86/efi: Fix memcmp check

[Andrew Cooper]

On 02/04/15 09:24, Ross Lagerwall wrote:
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/dmi_scan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index ec411e6..3749688 100644
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -203,7 +203,7 @@ void __init dmi_efi_get_table(const void *smbios, const void *smbios3)
>  	const struct smbios_eps *eps = smbios;
>  	const struct smbios3_eps *eps3 = smbios3;
>  
> -	if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) &&
> +	if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) == 0 &&
>  	    eps3->length >= sizeof(*eps3) &&
>  	    dmi_checksum(eps3, eps3->length)) {
>  		efi_smbios3_address = eps3->address;

Re: [PATCH 1/2] x86/efi: Reserve SMBIOS table region when EFI booting

[Andrew Cooper]

On 02/04/15 09:24, Ross Lagerwall wrote:
> Some EFI firmware implementations may place the SMBIOS table in RAM
> marked as BootServicesData, which Xen does not consider as reserved.
> When dom0 tries to access the SMBIOS, the region is not contained in the
> initial P2M and it crashes with a page fault. To fix this, reserve the
> SMBIOS region.
>
> Also, fix the memcmp check for existence of the SMBIOS and the DMI
> checksum calculation.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

(Personally, I would either have folded the two patches together, or put
the memcmp() one first, fixing both occurrences.  However, as both of
these are candidates for backports, this probably turns more into a
bikeshed activity.)

> ---
>  xen/arch/x86/dmi_scan.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index 187c01e..ec411e6 100644
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -189,6 +189,8 @@ static inline bool_t __init dmi_checksum(const void __iomem *buf,
>  
>  static u32 __initdata efi_dmi_address;
>  static u32 __initdata efi_dmi_size;
> +static u32 __initdata efi_smbios_address;
> +static u32 __initdata efi_smbios_size;
>  static u64 __initdata efi_smbios3_address;
>  static u32 __initdata efi_smbios3_size;
>  
> @@ -209,13 +211,18 @@ void __init dmi_efi_get_table(const void *smbios, const void *smbios3)
>  		return;
>  	}
>  
> -	if (eps && memcmp(eps->anchor, "_SM_", 4) &&
> +	if (eps && memcmp(eps->anchor, "_SM_", 4) == 0 &&
>  	    eps->length >= sizeof(*eps) &&
> -	    dmi_checksum(eps, eps->length) &&
> -	    memcmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
> -	    dmi_checksum(&eps->dmi, sizeof(eps->dmi))) {
> -		efi_dmi_address = eps->dmi.address;
> -		efi_dmi_size = eps->dmi.size;
> +	    dmi_checksum(eps, eps->length)) {
> +		efi_smbios_address = (u32)(long)smbios;
> +		efi_smbios_size = eps->length;
> +
> +		if ( memcmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
> +		     dmi_checksum(&eps->dmi,
> +				  eps->length - offsetof(struct smbios_eps, dmi)) ) {
> +			efi_dmi_address = eps->dmi.address;
> +			efi_dmi_size = eps->dmi.size;
> +		}
>  	}
>  }
>  
> @@ -236,6 +243,12 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len)
>  			instance |= 2;
>  			return "DMI";
>  		}
> +		if (efi_smbios_size && !(instance & 4)) {
> +			*base = efi_smbios_address;
> +			*len = efi_smbios_size;
> +			instance |= 4;
> +			return "SMBIOS";
> +		}
>  	} else {
>  		char __iomem *p = maddr_to_virt(0xF0000), *q;
>  		union {

Re: [PATCH 1/2] x86/efi: Reserve SMBIOS table region when EFI booting

[Jan Beulich]

>>> On 02.04.15 at 10:24, <ross.lagerwall@citrix.com> wrote:
> Some EFI firmware implementations may place the SMBIOS table in RAM
> marked as BootServicesData, which Xen does not consider as reserved.
> When dom0 tries to access the SMBIOS, the region is not contained in the
> initial P2M and it crashes with a page fault. To fix this, reserve the
> SMBIOS region.
> 
> Also, fix the memcmp check for existence of the SMBIOS

As Andrew said, put the memcmp() fixes all in a separate patch, or
fold the one remaining one in here.

> and the DMI
> checksum calculation.

This I don't agree with, and even less so without explanation. The
spec says

"15h
 Intermediate Checksum
 BYTE
 Checksum of Intermediate Entry Point Structure (IEPS). This value,
 when added to all other bytes in the IEPS, results in the value 00h
 (using 8-bit addition calculations). Values in the IEPS are summed
 starting at offset 10h, for 0Fh bytes."

I.e. I can't see why the sizeof() you replace would be wrong (other
than for broken firmware).

Jan