Syslinux - Sep 2013 - [PATCH 2/2 v2] com32/disk: Improve flow at disk_write_sectors and disk_read_sectors.

This patch will improve the flow at disk_write_sectors and disk_read_sectors.
It does that by creating a table of values respective to the operation.

Besides, read and write operations are pretty similar to each other,
so I redesigned the routines to avoid duplication.

Signed-off-by: Raphael S.Carvalho <raphael.scarv at gmail.com>
---
 com32/include/syslinux/disk.h |   18 ++++
 com32/lib/syslinux/disk.c     |  179 +++++++++++++++++++++-------------------
 2 files changed, 112 insertions(+), 85 deletions(-)

diff --git a/com32/include/syslinux/disk.h b/com32/include/syslinux/disk.h
index f96ca68..348d1ae 100644
--- a/com32/include/syslinux/disk.h
+++ b/com32/include/syslinux/disk.h
@@ -52,6 +52,19 @@ struct disk_info {
     uint32_t spt;
 };
 
+struct disk_ops {
+    uint8_t ah;
+    void *(*op)(const struct disk_info *const, com32sys_t *, 
+		uint64_t, uint8_t);
+};
+
+enum disk_op_codes {
+    EBIOS_READ_CODE = 0x42, /* Extended read */
+    EBIOS_WRITE_CODE = 0x43, /* Extended write */
+    CHS_READ_CODE = 0x02,
+    CHS_WRITE_CODE = 0x03,
+};
+
 struct disk_ebios_dapa {
     uint16_t len;
     uint16_t count;
@@ -177,4 +190,9 @@ extern void disk_gpt_part_dump(const struct
disk_gpt_part_entry *const
 			       gpt_part);
 extern void disk_gpt_header_dump(const struct disk_gpt_header *const gpt);
 
+static inline void *ebios_disk_op(const struct disk_info *const, 
+				  com32sys_t *, uint64_t, uint8_t);
+static inline void *chs_disk_op(const struct disk_info *const, 
+				com32sys_t *, uint64_t, uint8_t);
+
 #endif /* _SYSLINUX_DISK_H */
diff --git a/com32/lib/syslinux/disk.c b/com32/lib/syslinux/disk.c
index 554bed3..96e32e7 100644
--- a/com32/lib/syslinux/disk.c
+++ b/com32/lib/syslinux/disk.c
@@ -3,6 +3,7 @@
  *   Copyright 2003-2009 H. Peter Anvin - All Rights Reserved
  *   Copyright 2009-2010 Intel Corporation; author: H. Peter Anvin
  *   Copyright (C) 2010 Shao Miller
+ *   Copyright (C) 2013 Raphael S. Carvalho
  *
  *   Permission is hereby granted, free of charge, to any person
  *   obtaining a copy of this software and associated documentation
@@ -159,6 +160,81 @@ out:
 }
 
 /**
+ * Fill inreg based on EBIOS addressing properties.
+ *
+ * @v diskinfo			The disk drive to read from
+ * @v inreg			Register data structure to be filled.
+ * @v lba			The logical block address to begin reading at
+ * @v count			The number of sectors to read
+ * @ret 			lmalloc'd buf upon success, NULL upon failure
+ */
+static inline void *
+ebios_disk_op(const struct disk_info *const diskinfo, 
+	      com32sys_t *inreg, uint64_t lba, uint8_t count)
+{
+    static struct disk_ebios_dapa dapa;
+    void *buf;
+    
+    buf = lmalloc(count * diskinfo->bps);
+    if (!buf)
+	return NULL;
+    
+    dapa.len = sizeof(dapa);
+    dapa.count = count;
+    dapa.off = OFFS(buf);
+    dapa.seg = SEG(buf);
+    dapa.lba = lba;
+
+    inreg->esi.w[0] = OFFS(&dapa);
+    inreg->ds = SEG(&dapa);
+    inreg->edx.b[0] = diskinfo->disk;   
+    
+    return buf;
+}
+
+/**
+ * Fill inreg based on CHS addressing properties.
+ *
+ * @v diskinfo			The disk drive to read from
+ * @v inreg			Register data structure to be filled.
+ * @v lba			The logical block address to begin reading at
+ * @v count			The number of sectors to read
+ * @ret 			lmalloc'd buf upon success, NULL upon failure
+ */
+static inline void *
+chs_disk_op(const struct disk_info *const diskinfo, 
+	    com32sys_t *inreg, uint64_t lba, uint8_t count)
+{
+    unsigned int c, h, s, t;
+    void *buf;
+    
+    buf = lmalloc(count * diskinfo->bps);
+    if (!buf)
+	return NULL;
+    
+    /*
+     * if we passed lba + count check and we get here, that means that
+     * lbacnt was calculated from chs geometry (or faked from 1/1/1), thus
+     * 32bits are perfectly enough and lbacnt corresponds to cylinder
+     * boundary
+     */
+    s = lba % diskinfo->spt;
+    t = lba / diskinfo->spt;
+    h = t % diskinfo->head;
+    c = t / diskinfo->head;
+
+    inreg->eax.b[0] = count;
+    inreg->ecx.b[1] = c;
+    inreg->ecx.b[0] = ((c & 0x300) >> 2) | (s+1);
+    inreg->edx.b[1] = h;
+    inreg->edx.b[0] = diskinfo->disk;
+    inreg->ebx.w[0] = OFFS(buf);
+    inreg->es = SEG(buf);
+    
+    return buf;
+}
+
+/**
  * Get disk block(s) and return a malloc'd buffer.
  *
  * @v diskinfo			The disk drive to read from
@@ -173,11 +249,14 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
 			uint8_t count)
 {
     com32sys_t inreg;
-    struct disk_ebios_dapa *dapa;
     void *buf;
     void *data = NULL;
-    uint32_t maxcnt;
+    uint32_t idx, maxcnt;
     uint32_t size = 65536;
+    static struct disk_ops read_ops[2] = {
+	{ CHS_READ_CODE, &chs_disk_op },
+	{ EBIOS_READ_CODE, &ebios_disk_op }
+    };
 
     maxcnt = (size - diskinfo->bps) / diskinfo->bps;
     if (!count || count > maxcnt || lba + count > diskinfo->lbacnt)
@@ -185,48 +264,12 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
 
     memset(&inreg, 0, sizeof inreg);
 
-    buf = lmalloc(count * diskinfo->bps);
+    idx = diskinfo->ebios & 1; /* at most 1 */
+    inreg.eax.b[1] = read_ops[idx].ah;
+    buf = read_ops[idx].op(diskinfo, &inreg, lba, count);
     if (!buf)
 	return NULL;
 
-    dapa = lmalloc(sizeof(*dapa));
-    if (!dapa)
-	goto out;
-
-    if (diskinfo->ebios) {
-	dapa->len = sizeof(*dapa);
-	dapa->count = count;
-	dapa->off = OFFS(buf);
-	dapa->seg = SEG(buf);
-	dapa->lba = lba;
-
-	inreg.esi.w[0] = OFFS(dapa);
-	inreg.ds = SEG(dapa);
-	inreg.edx.b[0] = diskinfo->disk;
-	inreg.eax.b[1] = 0x42;	/* Extended read */
-    } else {
-	unsigned int c, h, s, t;
-	/*
-	 * if we passed lba + count check and we get here, that means that
-	 * lbacnt was calculated from chs geometry (or faked from 1/1/1), thus
-	 * 32bits are perfectly enough and lbacnt corresponds to cylinder
-	 * boundary
-	 */
-	s = lba % diskinfo->spt;
-	t = lba / diskinfo->spt;
-	h = t % diskinfo->head;
-	c = t / diskinfo->head;
-
-	inreg.eax.b[0] = count;
-	inreg.eax.b[1] = 0x02;	/* Read */
-	inreg.ecx.b[1] = c;
-	inreg.ecx.b[0] = ((c & 0x300) >> 2) | (s+1);
-	inreg.edx.b[1] = h;
-	inreg.edx.b[0] = diskinfo->disk;
-	inreg.ebx.w[0] = OFFS(buf);
-	inreg.es = SEG(buf);
-    }
-
     if (disk_int13_retry(&inreg, NULL))
 	goto out;
 
@@ -234,7 +277,6 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
     if (data)
 	memcpy(data, buf, count * diskinfo->bps);
 out:
-    lfree(dapa);
     lfree(buf);
     return data;
 }
@@ -255,67 +297,34 @@ int disk_write_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
 		       const void *data, uint8_t count)
 {
     com32sys_t inreg;
-    struct disk_ebios_dapa *dapa;
     void *buf;
-    uint32_t maxcnt;
+    uint32_t idx, maxcnt;
     uint32_t size = 65536;
     int rv = -1;
+    static struct disk_ops write_ops[2] = {
+	{ CHS_WRITE_CODE, &chs_disk_op },
+	{ EBIOS_WRITE_CODE, &ebios_disk_op }
+    };
 
     maxcnt = (size - diskinfo->bps) / diskinfo->bps;
     if (!count || count > maxcnt || lba + count > diskinfo->lbacnt)
 	return -1;
+    
+    memset(&inreg, 0, sizeof inreg);
 
-    buf = lmalloc(count * diskinfo->bps);
+    idx = diskinfo->ebios & 1; /* at most 1 */
+    inreg.eax.b[1] = write_ops[idx].ah;
+    buf = write_ops[idx].op(diskinfo, &inreg, lba, count);
     if (!buf)
 	return -1;
 
     memcpy(buf, data, count * diskinfo->bps);
-    memset(&inreg, 0, sizeof inreg);
-
-    dapa = lmalloc(sizeof(*dapa));
-    if (!dapa)
-	goto out;
-
-    if (diskinfo->ebios) {
-	dapa->len = sizeof(*dapa);
-	dapa->count = count;
-	dapa->off = OFFS(buf);
-	dapa->seg = SEG(buf);
-	dapa->lba = lba;
-
-	inreg.esi.w[0] = OFFS(dapa);
-	inreg.ds = SEG(dapa);
-	inreg.edx.b[0] = diskinfo->disk;
-	inreg.eax.b[1] = 0x43;	/* Extended write */
-    } else {
-	unsigned int c, h, s, t;
-	/*
-	 * if we passed lba + count check and we get here, that means that
-	 * lbacnt was calculated from chs geometry (or faked from 1/1/1), thus
-	 * 32bits are perfectly enough and lbacnt corresponds to cylinder
-	 * boundary
-	 */
-	s = lba % diskinfo->spt;
-	t = lba / diskinfo->spt;
-	h = t % diskinfo->head;
-	c = t / diskinfo->head;
-
-	inreg.eax.b[0] = count;
-	inreg.eax.b[1] = 0x03;	/* Write */
-	inreg.ecx.b[1] = c;
-	inreg.ecx.b[0] = ((c & 0x300) >> 2) | (s+1);
-	inreg.edx.b[1] = h;
-	inreg.edx.b[0] = diskinfo->disk;
-	inreg.ebx.w[0] = OFFS(buf);
-	inreg.es = SEG(buf);
-    }
 
     if (disk_int13_retry(&inreg, NULL))
 	goto out;
 
     rv = 0;			/* ok */
 out:
-    lfree(dapa);
     lfree(buf);
     return rv;
 }
-- 
1.7.2.5

On Fri, 06 Sep, at 04:00:16AM, Raphael S.Carvalho wrote:
> This patch will improve the flow at disk_write_sectors and
disk_read_sectors.
> It does that by creating a table of values respective to the operation.
> 
> Besides, read and write operations are pretty similar to each other,
> so I redesigned the routines to avoid duplication.
> 
> Signed-off-by: Raphael S.Carvalho <raphael.scarv at gmail.com>
> ---
>  com32/include/syslinux/disk.h |   18 ++++
>  com32/lib/syslinux/disk.c     |  179
+++++++++++++++++++++-------------------
>  2 files changed, 112 insertions(+), 85 deletions(-)
Again, this patch suffers from numerous whitespace errors.

If you apply this patch using git-am, you'll see what I'm talking about.
> diff --git a/com32/include/syslinux/disk.h b/com32/include/syslinux/disk.h
> index f96ca68..348d1ae 100644
> --- a/com32/include/syslinux/disk.h
> +++ b/com32/include/syslinux/disk.h
> @@ -52,6 +52,19 @@ struct disk_info {
>      uint32_t spt;
>  };
>  
> +struct disk_ops {
> +    uint8_t ah;
> +    void *(*op)(const struct disk_info *const, com32sys_t *, 
> +		uint64_t, uint8_t);
> +};
> +
> +enum disk_op_codes {
> +    EBIOS_READ_CODE = 0x42, /* Extended read */
> +    EBIOS_WRITE_CODE = 0x43, /* Extended write */
> +    CHS_READ_CODE = 0x02,
> +    CHS_WRITE_CODE = 0x03,
> +};
> +
>  struct disk_ebios_dapa {
>      uint16_t len;
>      uint16_t count;
> @@ -177,4 +190,9 @@ extern void disk_gpt_part_dump(const struct
disk_gpt_part_entry *const
>  			       gpt_part);
>  extern void disk_gpt_header_dump(const struct disk_gpt_header *const gpt);
>  
> +static inline void *ebios_disk_op(const struct disk_info *const, 
> +				  com32sys_t *, uint64_t, uint8_t);
> +static inline void *chs_disk_op(const struct disk_info *const, 
> +				com32sys_t *, uint64_t, uint8_t);
> +
>  #endif /* _SYSLINUX_DISK_H */
> diff --git a/com32/lib/syslinux/disk.c b/com32/lib/syslinux/disk.c
> index 554bed3..96e32e7 100644
> --- a/com32/lib/syslinux/disk.c
> +++ b/com32/lib/syslinux/disk.c
> @@ -3,6 +3,7 @@
>   *   Copyright 2003-2009 H. Peter Anvin - All Rights Reserved
>   *   Copyright 2009-2010 Intel Corporation; author: H. Peter Anvin
>   *   Copyright (C) 2010 Shao Miller
> + *   Copyright (C) 2013 Raphael S. Carvalho
>   *
>   *   Permission is hereby granted, free of charge, to any person
>   *   obtaining a copy of this software and associated documentation
> @@ -159,6 +160,81 @@ out:
>  }
>  
>  /**
> + * Fill inreg based on EBIOS addressing properties.
> + *
> + * @v diskinfo			The disk drive to read from
> + * @v inreg			Register data structure to be filled.
> + * @v lba			The logical block address to begin reading at
> + * @v count			The number of sectors to read
> + * @ret 			lmalloc'd buf upon success, NULL upon failure
> + */
> +static inline void *
> +ebios_disk_op(const struct disk_info *const diskinfo, 
> +	      com32sys_t *inreg, uint64_t lba, uint8_t count)
> +{
> +    static struct disk_ebios_dapa dapa;
> +    void *buf;
> +    
> +    buf = lmalloc(count * diskinfo->bps);
> +    if (!buf)
> +	return NULL;
> +    
> +    dapa.len = sizeof(dapa);
> +    dapa.count = count;
> +    dapa.off = OFFS(buf);
> +    dapa.seg = SEG(buf);
> +    dapa.lba = lba;
> +
> +    inreg->esi.w[0] = OFFS(&dapa);
> +    inreg->ds = SEG(&dapa);
> +    inreg->edx.b[0] = diskinfo->disk;   
> +    
'dapa' needs to be in low memory (addressable using SEG() and OFFS())
and the bss is not in low memory. You need to either tag it as
'__lowmem' or allocate the memory with lmalloc().
> +    return buf;
> +}
> +
> +/**
> + * Fill inreg based on CHS addressing properties.
> + *
> + * @v diskinfo			The disk drive to read from
> + * @v inreg			Register data structure to be filled.
> + * @v lba			The logical block address to begin reading at
> + * @v count			The number of sectors to read
> + * @ret 			lmalloc'd buf upon success, NULL upon failure
> + */
> +static inline void *
> +chs_disk_op(const struct disk_info *const diskinfo, 
> +	    com32sys_t *inreg, uint64_t lba, uint8_t count)
> +{
> +    unsigned int c, h, s, t;
> +    void *buf;
> +    
> +    buf = lmalloc(count * diskinfo->bps);
> +    if (!buf)
> +	return NULL;
> +    
> +    /*
> +     * if we passed lba + count check and we get here, that means that
> +     * lbacnt was calculated from chs geometry (or faked from 1/1/1), thus
> +     * 32bits are perfectly enough and lbacnt corresponds to cylinder
> +     * boundary
> +     */
> +    s = lba % diskinfo->spt;
> +    t = lba / diskinfo->spt;
> +    h = t % diskinfo->head;
> +    c = t / diskinfo->head;
> +
> +    inreg->eax.b[0] = count;
> +    inreg->ecx.b[1] = c;
> +    inreg->ecx.b[0] = ((c & 0x300) >> 2) | (s+1);
> +    inreg->edx.b[1] = h;
> +    inreg->edx.b[0] = diskinfo->disk;
> +    inreg->ebx.w[0] = OFFS(buf);
> +    inreg->es = SEG(buf);
> +    
> +    return buf;
> +}
> +
> +/**
>   * Get disk block(s) and return a malloc'd buffer.
>   *
>   * @v diskinfo			The disk drive to read from
> @@ -173,11 +249,14 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
>  			uint8_t count)
>  {
>      com32sys_t inreg;
> -    struct disk_ebios_dapa *dapa;
>      void *buf;
>      void *data = NULL;
> -    uint32_t maxcnt;
> +    uint32_t idx, maxcnt;
>      uint32_t size = 65536;
> +    static struct disk_ops read_ops[2] = {
> +	{ CHS_READ_CODE, &chs_disk_op },
> +	{ EBIOS_READ_CODE, &ebios_disk_op }
> +    };
>  
>      maxcnt = (size - diskinfo->bps) / diskinfo->bps;
>      if (!count || count > maxcnt || lba + count >
diskinfo->lbacnt)
> @@ -185,48 +264,12 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
>  
>      memset(&inreg, 0, sizeof inreg);
>  
> -    buf = lmalloc(count * diskinfo->bps);
> +    idx = diskinfo->ebios & 1; /* at most 1 */
> +    inreg.eax.b[1] = read_ops[idx].ah;
> +    buf = read_ops[idx].op(diskinfo, &inreg, lba, count);
>      if (!buf)
>  	return NULL;
>  
Hmm.. I don't think this improves the flow at all. You've pulled the
common code out into *disk_op() functions, which is a nice cleanup, but
this jumping through function pointers looks confusing.

Perhaps,

	if (diskinfo->ebios)
	    buf = ebios_disk_op(diskinfo, &inreg, lba, count, EBIOS_READ);
	else
	    buf = chs_disk_op(diskinfo, &inreg, lba, count, CHS_READ);

would be cleaner. You don't have to do any bit-twiddling to ensure you
don't go out of bounds in the disk_ops array this way, and it's not like
the abstraction of function pointers buys us anything, as we still need
to check diskinfo->ebios.


-- 
Matt Fleming, Intel Open Source Technology Center