Syslinux - Sep 2013 - [PATCH 2/2] 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
significantly,
but it *will* introduce bugs if either of the above functions gets called before
disk_get_params.
---
 com32/include/syslinux/disk.h |   21 +++++
 com32/lib/syslinux/disk.c     |  170 +++++++++++++++++++++--------------------
 2 files changed, 108 insertions(+), 83 deletions(-)

diff --git a/com32/include/syslinux/disk.h b/com32/include/syslinux/disk.h
index f96ca68..e793514 100644
--- a/com32/include/syslinux/disk.h
+++ b/com32/include/syslinux/disk.h
@@ -41,6 +41,14 @@
 
 #define SECTOR 512u		/* bytes/sector */
 
+struct disk_info;
+struct disk_ops {
+    void *(*disk_op)(const struct disk_info *const, com32sys_t *, 
+		     uint64_t, uint8_t);
+    uint32_t read_code;
+    uint32_t write_code;
+};
+
 struct disk_info {
     int disk;
     int ebios;			/* EBIOS supported on this disk */
@@ -50,6 +58,14 @@ struct disk_info {
     uint32_t cyl;
     uint32_t head;
     uint32_t spt;
+    struct disk_ops ops;
+};
+
+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 {
@@ -177,4 +193,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 void *ebios_disk_op(const struct disk_info *const, 
+			   com32sys_t *, uint64_t, uint8_t);
+static 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..c80bfbb 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
@@ -92,6 +93,13 @@ int disk_get_params(int disk, struct disk_info *const
diskinfo)
     if (!(outreg.eflags.l & EFLAGS_CF) &&
 	outreg.ebx.w[0] == 0xaa55 && (outreg.ecx.b[0] & 1)) {
 	diskinfo->ebios = 1;
+	diskinfo->ops.disk_op = &ebios_disk_op;
+	diskinfo->ops.read_code = EBIOS_READ_CODE;
+	diskinfo->ops.write_code = EBIOS_WRITE_CODE;
+    } else {
+	diskinfo->ops.disk_op = &chs_disk_op;
+	diskinfo->ops.read_code = CHS_READ_CODE;
+	diskinfo->ops.write_code = CHS_WRITE_CODE;
     }
 
     /* Get extended disk parameters if ebios == 1 */
@@ -159,6 +167,79 @@ 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 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 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,7 +254,6 @@ 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;
@@ -185,48 +265,11 @@ void *disk_read_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
 
     memset(&inreg, 0, sizeof inreg);
 
-    buf = lmalloc(count * diskinfo->bps);
+    inreg.eax.b[1] = diskinfo->ops.read_code;
+    buf = diskinfo->ops.disk_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,7 +297,6 @@ 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 size = 65536;
@@ -264,58 +305,21 @@ int disk_write_sectors(const struct disk_info *const
diskinfo, uint64_t lba,
     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);
+    inreg.eax.b[1] = diskinfo->ops.write_code;
+    buf = diskinfo->ops.disk_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

I reread the entire patch, and figured out that it may be better to choose
what to do based on diskinfo->ebios (as earlier).
Function pointer is a good idea because it will avoid branching. However,
it would *only* work if the read/write functions
get called after disk_get_params setup the disk_info structure.

Probably, there is no way of assuring that, so I would recommend doing the
following instead:

if (diskinfo->ebios) {
    inreg.eax.b[1] = EBIOS_..._CODE;
    buf = ebios_disk_op(diskinfo, &inreg, lba, count);
} else {
    inreg.eax.b[1] = CHS_..._CODE;
    buf = chs_disk_op(diskinfo, &inreg, lba, count);
}

Regards,
Raphael S. Carvalho.

On Fri, Sep 6, 2013 at 1:03 AM, Raphael S.Carvalho
<raphael.scarv at gmail.com>wrote:
> This patch will improve the flow at disk_write_sectors and
> disk_read_sectors significantly,
> but it *will* introduce bugs if either of the above functions gets called
> before disk_get_params.
> ---
>  com32/include/syslinux/disk.h |   21 +++++
>  com32/lib/syslinux/disk.c     |  170
> +++++++++++++++++++++--------------------
>  2 files changed, 108 insertions(+), 83 deletions(-)
>
> diff --git a/com32/include/syslinux/disk.h b/com32/include/syslinux/disk.h
> index f96ca68..e793514 100644
> --- a/com32/include/syslinux/disk.h
> +++ b/com32/include/syslinux/disk.h
> @@ -41,6 +41,14 @@
>
>  #define SECTOR 512u            /* bytes/sector */
>
> +struct disk_info;
> +struct disk_ops {
> +    void *(*disk_op)(const struct disk_info *const, com32sys_t *,
> +                    uint64_t, uint8_t);
> +    uint32_t read_code;
> +    uint32_t write_code;
> +};
> +
>  struct disk_info {
>      int disk;
>      int ebios;                 /* EBIOS supported on this disk */
> @@ -50,6 +58,14 @@ struct disk_info {
>      uint32_t cyl;
>      uint32_t head;
>      uint32_t spt;
> +    struct disk_ops ops;
> +};
> +
> +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 {
> @@ -177,4 +193,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 void *ebios_disk_op(const struct disk_info *const,
> +                          com32sys_t *, uint64_t, uint8_t);
> +static 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..c80bfbb 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
> @@ -92,6 +93,13 @@ int disk_get_params(int disk, struct disk_info *const
> diskinfo)
>      if (!(outreg.eflags.l & EFLAGS_CF) &&
>         outreg.ebx.w[0] == 0xaa55 && (outreg.ecx.b[0] & 1)) {
>         diskinfo->ebios = 1;
> +       diskinfo->ops.disk_op = &ebios_disk_op;
> +       diskinfo->ops.read_code = EBIOS_READ_CODE;
> +       diskinfo->ops.write_code = EBIOS_WRITE_CODE;
> +    } else {
> +       diskinfo->ops.disk_op = &chs_disk_op;
> +       diskinfo->ops.read_code = CHS_READ_CODE;
> +       diskinfo->ops.write_code = CHS_WRITE_CODE;
>      }
>
>      /* Get extended disk parameters if ebios == 1 */
> @@ -159,6 +167,79 @@ 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 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 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,7 +254,6 @@ 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;
> @@ -185,48 +265,11 @@ void *disk_read_sectors(const struct disk_info
> *const diskinfo, uint64_t lba,
>
>      memset(&inreg, 0, sizeof inreg);
>
> -    buf = lmalloc(count * diskinfo->bps);
> +    inreg.eax.b[1] = diskinfo->ops.read_code;
> +    buf = diskinfo->ops.disk_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,7 +297,6 @@ 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 size = 65536;
> @@ -264,58 +305,21 @@ int disk_write_sectors(const struct disk_info *const
> diskinfo, uint64_t lba,
>      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);
> +    inreg.eax.b[1] = diskinfo->ops.write_code;
> +    buf = diskinfo->ops.disk_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
>
>