[syslinux] [PATCH 2/2] com32/disk: Improve flow at disk_write_sectors and disk_read_sectors.
Raphael S Carvalho
raphael.scarv at gmail.com
Thu Sep 5 21:23:15 PDT 2013
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
>
>
More information about the Syslinux
mailing list