[syslinux] [PATCH 2/2 v2] com32/disk: Improve flow at disk_write_sectors and disk_read_sectors.
Matt Fleming
matt at console-pimps.org
Tue Sep 17 07:37:10 PDT 2013
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
More information about the Syslinux
mailing list