[PATCH] x86/DMI: use proper structures in favor of byte offsets

[PATCH] x86/DMI: use proper structures in favor of byte offsets

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3900 bytes --]

Besides being (in my eyes) desirable cleanup, this at once represents
another prerequisite for native EFI booting support.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -10,11 +10,31 @@
 #include <asm/system.h>
 #include <xen/dmi.h>
 
-#define bt_ioremap(b,l)  ((u8 *)__acpi_map_table(b,l))
+#define bt_ioremap(b,l)  ((void *)__acpi_map_table(b,l))
 #define bt_iounmap(b,l)  ((void)0)
 #define memcpy_fromio    memcpy
 #define alloc_bootmem(l) xmalloc_bytes(l)
 
+struct dmi_eps {
+	char anchor[5];			/* "_DMI_" */
+	u8 checksum;
+	u16 size;
+	u32 address;
+	u16 num_structures;
+	u8 revision;
+} __attribute__((packed));
+
+struct smbios_eps {
+	char anchor[4];			/* "_SM_" */
+	u8 checksum;
+	u8 length;
+	u8 major, minor;
+	u16 max_size;
+	u8 revision;
+	u8 _rsrvd_[5];
+	struct dmi_eps dmi;
+} __attribute__((packed));
+
 struct dmi_header
 {
 	u8	type;
@@ -90,62 +110,70 @@ static int __init dmi_table(u32 base, in
 }
 
 
-inline static int __init dmi_checksum(u8 *buf)
+static inline bool_t __init dmi_checksum(const void __iomem *buf,
+					 unsigned int len)
 {
-	u8 sum=0;
-	int a;
+	u8 sum = 0;
+	const u8 *p = buf;
+	unsigned int a;
 	
-	for(a=0; a<15; a++)
-		sum+=buf[a];
-	return (sum==0);
+	for (a = 0; a < len; a++)
+		sum += p[a];
+	return sum == 0;
 }
 
 int __init dmi_get_table(u32 *base, u32 *len)
 {
-	u8 buf[15];
+	struct dmi_eps eps;
 	char __iomem *p, *q;
 
 	p = maddr_to_virt(0xF0000);
 	for (q = p; q < p + 0x10000; q += 16) {
-		memcpy_fromio(buf, q, 15);
-		if (memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(buf)) {
-			*base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
-			*len=buf[7]<<8|buf[6];
+		memcpy_fromio(&eps, q, 15);
+		if (memcmp(eps.anchor, "_DMI_", 5) == 0 &&
+		    dmi_checksum(&eps, sizeof(eps))) {
+			*base = eps.address;
+			*len = eps.size;
 			return 0;
 		}
 	}
 	return -1;
 }
 
+static int __init _dmi_iterate(const struct dmi_eps *dmi,
+			       const struct smbios_eps __iomem *smbios,
+			       void (*decode)(struct dmi_header *))
+{
+	u16 num = dmi->num_structures;
+	u16 len = dmi->size;
+	u32 base = dmi->address;
+
+	/*
+	 * DMI version 0.0 means that the real version is taken from
+	 * the SMBIOS version, which we may not know at this point.
+	 */
+	if (dmi->revision)
+		printk(KERN_INFO "DMI %d.%d present.\n",
+		       dmi->revision >> 4,  dmi->revision & 0x0f);
+	else if (!smbios)
+		printk(KERN_INFO "DMI present.\n");
+	dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
+		    num, len));
+	dmi_printk((KERN_INFO "DMI table at 0x%08X.\n", base));
+	return dmi_table(base, len, num, decode);
+}
+
 static int __init dmi_iterate(void (*decode)(struct dmi_header *))
 {
-	u8 buf[15];
+	struct dmi_eps eps;
 	char __iomem *p, *q;
 
 	p = maddr_to_virt(0xF0000);
 	for (q = p; q < p + 0x10000; q += 16) {
-		memcpy_fromio(buf, q, 15);
-		if (memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(buf)) {
-			u16 num=buf[13]<<8|buf[12];
-			u16 len=buf[7]<<8|buf[6];
-			u32 base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
-
-			/*
-			 * DMI version 0.0 means that the real version is taken from
-			 * the SMBIOS version, which we don't know at this point.
-			 */
-			if(buf[14]!=0)
-				printk(KERN_INFO "DMI %d.%d present.\n",
-					buf[14]>>4, buf[14]&0x0F);
-			else
-				printk(KERN_INFO "DMI present.\n");
-			dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
-				num, len));
-			dmi_printk((KERN_INFO "DMI table at 0x%08X.\n",
-				base));
-			if(dmi_table(base,len, num, decode)==0)
-				return 0;
-		}
+		memcpy_fromio(&eps, q, sizeof(eps));
+		if (memcmp(eps.anchor, "_DMI_", 5) == 0 &&
+		    dmi_checksum(&eps, sizeof(eps)))
+			return _dmi_iterate(&eps, NULL, decode);
 	}
 	return -1;
 }




[-- Attachment #2: x86-DMI.patch --]
[-- Type: text/plain, Size: 3894 bytes --]

Besides being (in my eyes) desirable cleanup, this at once represents
another prerequisite for native EFI booting support.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -10,11 +10,31 @@
 #include <asm/system.h>
 #include <xen/dmi.h>
 
-#define bt_ioremap(b,l)  ((u8 *)__acpi_map_table(b,l))
+#define bt_ioremap(b,l)  ((void *)__acpi_map_table(b,l))
 #define bt_iounmap(b,l)  ((void)0)
 #define memcpy_fromio    memcpy
 #define alloc_bootmem(l) xmalloc_bytes(l)
 
+struct dmi_eps {
+	char anchor[5];			/* "_DMI_" */
+	u8 checksum;
+	u16 size;
+	u32 address;
+	u16 num_structures;
+	u8 revision;
+} __attribute__((packed));
+
+struct smbios_eps {
+	char anchor[4];			/* "_SM_" */
+	u8 checksum;
+	u8 length;
+	u8 major, minor;
+	u16 max_size;
+	u8 revision;
+	u8 _rsrvd_[5];
+	struct dmi_eps dmi;
+} __attribute__((packed));
+
 struct dmi_header
 {
 	u8	type;
@@ -90,62 +110,70 @@ static int __init dmi_table(u32 base, in
 }
 
 
-inline static int __init dmi_checksum(u8 *buf)
+static inline bool_t __init dmi_checksum(const void __iomem *buf,
+					 unsigned int len)
 {
-	u8 sum=0;
-	int a;
+	u8 sum = 0;
+	const u8 *p = buf;
+	unsigned int a;
 	
-	for(a=0; a<15; a++)
-		sum+=buf[a];
-	return (sum==0);
+	for (a = 0; a < len; a++)
+		sum += p[a];
+	return sum == 0;
 }
 
 int __init dmi_get_table(u32 *base, u32 *len)
 {
-	u8 buf[15];
+	struct dmi_eps eps;
 	char __iomem *p, *q;
 
 	p = maddr_to_virt(0xF0000);
 	for (q = p; q < p + 0x10000; q += 16) {
-		memcpy_fromio(buf, q, 15);
-		if (memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(buf)) {
-			*base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
-			*len=buf[7]<<8|buf[6];
+		memcpy_fromio(&eps, q, 15);
+		if (memcmp(eps.anchor, "_DMI_", 5) == 0 &&
+		    dmi_checksum(&eps, sizeof(eps))) {
+			*base = eps.address;
+			*len = eps.size;
 			return 0;
 		}
 	}
 	return -1;
 }
 
+static int __init _dmi_iterate(const struct dmi_eps *dmi,
+			       const struct smbios_eps __iomem *smbios,
+			       void (*decode)(struct dmi_header *))
+{
+	u16 num = dmi->num_structures;
+	u16 len = dmi->size;
+	u32 base = dmi->address;
+
+	/*
+	 * DMI version 0.0 means that the real version is taken from
+	 * the SMBIOS version, which we may not know at this point.
+	 */
+	if (dmi->revision)
+		printk(KERN_INFO "DMI %d.%d present.\n",
+		       dmi->revision >> 4,  dmi->revision & 0x0f);
+	else if (!smbios)
+		printk(KERN_INFO "DMI present.\n");
+	dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
+		    num, len));
+	dmi_printk((KERN_INFO "DMI table at 0x%08X.\n", base));
+	return dmi_table(base, len, num, decode);
+}
+
 static int __init dmi_iterate(void (*decode)(struct dmi_header *))
 {
-	u8 buf[15];
+	struct dmi_eps eps;
 	char __iomem *p, *q;
 
 	p = maddr_to_virt(0xF0000);
 	for (q = p; q < p + 0x10000; q += 16) {
-		memcpy_fromio(buf, q, 15);
-		if (memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(buf)) {
-			u16 num=buf[13]<<8|buf[12];
-			u16 len=buf[7]<<8|buf[6];
-			u32 base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
-
-			/*
-			 * DMI version 0.0 means that the real version is taken from
-			 * the SMBIOS version, which we don't know at this point.
-			 */
-			if(buf[14]!=0)
-				printk(KERN_INFO "DMI %d.%d present.\n",
-					buf[14]>>4, buf[14]&0x0F);
-			else
-				printk(KERN_INFO "DMI present.\n");
-			dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
-				num, len));
-			dmi_printk((KERN_INFO "DMI table at 0x%08X.\n",
-				base));
-			if(dmi_table(base,len, num, decode)==0)
-				return 0;
-		}
+		memcpy_fromio(&eps, q, sizeof(eps));
+		if (memcmp(eps.anchor, "_DMI_", 5) == 0 &&
+		    dmi_checksum(&eps, sizeof(eps)))
+			return _dmi_iterate(&eps, NULL, decode);
 	}
 	return -1;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86/DMI: use proper structures in favor of byte offsets

[Chris Dalton]

Hi Jan,

Not a comment on the patch changes directly but on the code covered by 
the patch: in my experience on some EFI bioses you can't assume that the 
SMBIOS tables are in a region beginning 0xF0000. The location needs to 
be retrieved from the EFI system table, but I guess you have this in hand.

Chris

On 21/06/11 09:01, Jan Beulich wrote:
> Besides being (in my eyes) desirable cleanup, this at once represents
> another prerequisite for native EFI booting support.
>
> Signed-off-by: Jan Beulich<jbeulich@novell.com>
>
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -10,11 +10,31 @@
>   #include<asm/system.h>
>   #include<xen/dmi.h>
>
> -#define bt_ioremap(b,l)  ((u8 *)__acpi_map_table(b,l))
> +#define bt_ioremap(b,l)  ((void *)__acpi_map_table(b,l))
>   #define bt_iounmap(b,l)  ((void)0)
>   #define memcpy_fromio    memcpy
>   #define alloc_bootmem(l) xmalloc_bytes(l)
>
> +struct dmi_eps {
> +	char anchor[5];			/* "_DMI_" */
> +	u8 checksum;
> +	u16 size;
> +	u32 address;
> +	u16 num_structures;
> +	u8 revision;
> +} __attribute__((packed));
> +
> +struct smbios_eps {
> +	char anchor[4];			/* "_SM_" */
> +	u8 checksum;
> +	u8 length;
> +	u8 major, minor;
> +	u16 max_size;
> +	u8 revision;
> +	u8 _rsrvd_[5];
> +	struct dmi_eps dmi;
> +} __attribute__((packed));
> +
>   struct dmi_header
>   {
>   	u8	type;
> @@ -90,62 +110,70 @@ static int __init dmi_table(u32 base, in
>   }
>
>
> -inline static int __init dmi_checksum(u8 *buf)
> +static inline bool_t __init dmi_checksum(const void __iomem *buf,
> +					 unsigned int len)
>   {
> -	u8 sum=0;
> -	int a;
> +	u8 sum = 0;
> +	const u8 *p = buf;
> +	unsigned int a;
>   	
> -	for(a=0; a<15; a++)
> -		sum+=buf[a];
> -	return (sum==0);
> +	for (a = 0; a<  len; a++)
> +		sum += p[a];
> +	return sum == 0;
>   }
>
>   int __init dmi_get_table(u32 *base, u32 *len)
>   {
> -	u8 buf[15];
> +	struct dmi_eps eps;
>   	char __iomem *p, *q;
>
>   	p = maddr_to_virt(0xF0000);
>   	for (q = p; q<  p + 0x10000; q += 16) {
> -		memcpy_fromio(buf, q, 15);
> -		if (memcmp(buf, "_DMI_", 5)==0&&  dmi_checksum(buf)) {
> -			*base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
> -			*len=buf[7]<<8|buf[6];
> +		memcpy_fromio(&eps, q, 15);
> +		if (memcmp(eps.anchor, "_DMI_", 5) == 0&&
> +		    dmi_checksum(&eps, sizeof(eps))) {
> +			*base = eps.address;
> +			*len = eps.size;
>   			return 0;
>   		}
>   	}
>   	return -1;
>   }
>
> +static int __init _dmi_iterate(const struct dmi_eps *dmi,
> +			       const struct smbios_eps __iomem *smbios,
> +			       void (*decode)(struct dmi_header *))
> +{
> +	u16 num = dmi->num_structures;
> +	u16 len = dmi->size;
> +	u32 base = dmi->address;
> +
> +	/*
> +	 * DMI version 0.0 means that the real version is taken from
> +	 * the SMBIOS version, which we may not know at this point.
> +	 */
> +	if (dmi->revision)
> +		printk(KERN_INFO "DMI %d.%d present.\n",
> +		       dmi->revision>>  4,  dmi->revision&  0x0f);
> +	else if (!smbios)
> +		printk(KERN_INFO "DMI present.\n");
> +	dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
> +		    num, len));
> +	dmi_printk((KERN_INFO "DMI table at 0x%08X.\n", base));
> +	return dmi_table(base, len, num, decode);
> +}
> +
>   static int __init dmi_iterate(void (*decode)(struct dmi_header *))
>   {
> -	u8 buf[15];
> +	struct dmi_eps eps;
>   	char __iomem *p, *q;
>
>   	p = maddr_to_virt(0xF0000);
>   	for (q = p; q<  p + 0x10000; q += 16) {
> -		memcpy_fromio(buf, q, 15);
> -		if (memcmp(buf, "_DMI_", 5)==0&&  dmi_checksum(buf)) {
> -			u16 num=buf[13]<<8|buf[12];
> -			u16 len=buf[7]<<8|buf[6];
> -			u32 base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
> -
> -			/*
> -			 * DMI version 0.0 means that the real version is taken from
> -			 * the SMBIOS version, which we don't know at this point.
> -			 */
> -			if(buf[14]!=0)
> -				printk(KERN_INFO "DMI %d.%d present.\n",
> -					buf[14]>>4, buf[14]&0x0F);
> -			else
> -				printk(KERN_INFO "DMI present.\n");
> -			dmi_printk((KERN_INFO "%d structures occupying %d bytes.\n",
> -				num, len));
> -			dmi_printk((KERN_INFO "DMI table at 0x%08X.\n",
> -				base));
> -			if(dmi_table(base,len, num, decode)==0)
> -				return 0;
> -		}
> +		memcpy_fromio(&eps, q, sizeof(eps));
> +		if (memcmp(eps.anchor, "_DMI_", 5) == 0&&
> +		    dmi_checksum(&eps, sizeof(eps)))
> +			return _dmi_iterate(&eps, NULL, decode);
>   	}
>   	return -1;
>   }
>
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


-- 
+44(0)7827 233109
Registered No: 690597 (Hewlett-Packard Ltd)
Registered Office: Cain Rd, Bracknell, Berks, RG12 1HN, UK

Re: [PATCH] x86/DMI: use proper structures in favor of byte offsets

[Jan Beulich]

>>> On 21.06.11 at 11:48, Chris Dalton <cid@hp.com> wrote:
> Not a comment on the patch changes directly but on the code covered by 
> the patch: in my experience on some EFI bioses you can't assume that the 
> SMBIOS tables are in a region beginning 0xF0000. The location needs to 
> be retrieved from the EFI system table, but I guess you have this in hand.

Yes, of course. The change here only is to not add further byte-offset
hacks in the code that's going to be added with the EFI patches I'm
preparing.

Jan