Some cleanups I came across while reviewing the boot code.
Gleb - patch 7 conflicts with your last patch - I hope it isn't a big deal to fixup though. (I'm still reviewing your last patch.)
-Kevin
Kevin O'Connor (9): Rename add_ordered_drive() to add_drive() and use in map_hd_drive(). Call setup_translation() from map_hd_drive(). Don't access drive_g->desc from boot_cdrom(). Remove Drives global struct in favor of independent global variables. Move IPL.checkfloppysig to a global (CheckFloppySig) in boot.c. Move IPL.bev to static variables in boot.c Move IPL.fw_bootorder to static variables in boot.c. Minor reorganization of some of the boot_xxx code in boot.c. Remove drive->desc field.
src/ahci.c | 40 +++++++--------- src/ata.c | 41 +++++++--------- src/block.c | 64 ++++++++++++------------- src/boot.c | 137 +++++++++++++++++++++++++++++++----------------------- src/boot.h | 36 +------------- src/cdrom.c | 2 +- src/coreboot.c | 5 +-- src/disk.c | 2 +- src/disk.h | 13 +---- src/floppy.c | 28 ++++++----- src/output.c | 26 ++++++++++ src/ramdisk.c | 10 +++- src/usb-msc.c | 32 ++++++------- src/util.h | 2 + src/virtio-blk.c | 11 ++--- 15 files changed, 221 insertions(+), 228 deletions(-)
--- src/block.c | 43 ++++++++++++++++++------------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/src/block.c b/src/block.c index 72b3081..6a3faa3 100644 --- a/src/block.c +++ b/src/block.c @@ -30,14 +30,13 @@ getDrive(u8 exttype, u8 extdriveoffset) int getDriveId(u8 exttype, struct drive_s *drive_g) { int i; - for (i = 0; i < ARRAY_SIZE(Drives.idmap[0]); i++) if (getDrive(exttype, i) == drive_g) return i; - return -1; }
+ /**************************************************************** * Disk geometry translation ****************************************************************/ @@ -200,27 +199,9 @@ fill_fdpt(struct drive_s *drive_g, int hdid) struct extended_bios_data_area_s, fdpt[1]))); }
-// Map a drive (that was registered via add_bcv_hd) -void -map_hd_drive(struct drive_s *drive_g) -{ - // fill hdidmap - u8 hdcount = GET_BDA(hdcount); - if (hdcount >= ARRAY_SIZE(Drives.idmap[0])) { - warn_noalloc(); - return; - } - dprintf(3, "Mapping hd drive %p to %d\n", drive_g, hdcount); - Drives.idmap[EXTTYPE_HD][hdcount] = drive_g; - SET_BDA(hdcount, hdcount + 1); - - // Fill "fdpt" structure. - fill_fdpt(drive_g, hdcount); -} - // Find spot to add a drive static void -add_ordered_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive_g) +add_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive_g) { if (*count >= ARRAY_SIZE(Drives.idmap[0])) { warn_noalloc(); @@ -230,22 +211,34 @@ add_ordered_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive_g) *count = *count + 1; }
+// Map a hard drive +void +map_hd_drive(struct drive_s *drive_g) +{ + ASSERT32FLAT(); + struct bios_data_area_s *bda = MAKE_FLATPTR(SEG_BDA, 0); + int hdid = bda->hdcount; + dprintf(3, "Mapping hd drive %p to %d\n", drive_g, hdid); + add_drive(Drives.idmap[EXTTYPE_HD], &bda->hdcount, drive_g); + + // Fill "fdpt" structure. + fill_fdpt(drive_g, hdid); +} + // Map a cd void map_cd_drive(struct drive_s *drive_g) { dprintf(3, "Mapping cd drive %p\n", drive_g); - add_ordered_drive(Drives.idmap[EXTTYPE_CD], &Drives.cdcount, drive_g); + add_drive(Drives.idmap[EXTTYPE_CD], &Drives.cdcount, drive_g); }
// Map a floppy void map_floppy_drive(struct drive_s *drive_g) { - // fill idmap dprintf(3, "Mapping floppy drive %p\n", drive_g); - add_ordered_drive(Drives.idmap[EXTTYPE_FLOPPY], &Drives.floppycount - , drive_g); + add_drive(Drives.idmap[EXTTYPE_FLOPPY], &Drives.floppycount, drive_g);
// Update equipment word bits for floppy if (Drives.floppycount == 1) {
Unify the calling of setup_translation(). --- src/ahci.c | 2 -- src/ata.c | 3 --- src/block.c | 5 ++++- src/disk.h | 1 - src/usb-msc.c | 3 --- src/virtio-blk.c | 1 - 6 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/ahci.c b/src/ahci.c index a7f458f..8fd2e14 100644 --- a/src/ahci.c +++ b/src/ahci.c @@ -379,8 +379,6 @@ ahci_port_init(struct ahci_ctrl_s *ctrl, u32 pnr) , ata_extract_version(buffer) , (u32)adjsize, adjprefix);
- // Setup disk geometry translation. - setup_translation(&port->drive); // Register with bcv system. boot_add_hd(&port->drive, -1); } else { diff --git a/src/ata.c b/src/ata.c index 872c5e3..86b89b5 100644 --- a/src/ata.c +++ b/src/ata.c @@ -834,9 +834,6 @@ init_drive_ata(struct atadrive_s *dummy, u16 *buffer) , (u32)adjsize, adjprefix); dprintf(1, "%s\n", adrive_g->drive.desc);
- // Setup disk geometry translation. - setup_translation(&adrive_g->drive); - int prio = bootprio_find_ata_device(adrive_g->chan_gf->pci_bdf, adrive_g->chan_gf->chanid, adrive_g->slave); diff --git a/src/block.c b/src/block.c index 6a3faa3..dafaaa6 100644 --- a/src/block.c +++ b/src/block.c @@ -72,7 +72,7 @@ get_translation(struct drive_s *drive_g) return TRANSLATION_LBA; }
-void +static void setup_translation(struct drive_s *drive_g) { u8 translation = get_translation(drive_g); @@ -221,6 +221,9 @@ map_hd_drive(struct drive_s *drive_g) dprintf(3, "Mapping hd drive %p to %d\n", drive_g, hdid); add_drive(Drives.idmap[EXTTYPE_HD], &bda->hdcount, drive_g);
+ // Setup disk geometry translation. + setup_translation(drive_g); + // Fill "fdpt" structure. fill_fdpt(drive_g, hdid); } diff --git a/src/disk.h b/src/disk.h index a598bb4..4802011 100644 --- a/src/disk.h +++ b/src/disk.h @@ -230,7 +230,6 @@ struct drives_s { extern struct drives_s Drives; struct drive_s *getDrive(u8 exttype, u8 extdriveoffset); int getDriveId(u8 exttype, struct drive_s *drive_g); -void setup_translation(struct drive_s *drive_g); void map_floppy_drive(struct drive_s *drive_g); void map_hd_drive(struct drive_s *drive_g); void map_cd_drive(struct drive_s *drive_g); diff --git a/src/usb-msc.c b/src/usb-msc.c index 2a6c31a..877ece4 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -167,9 +167,6 @@ setup_drive_hd(struct disk_op_s *op) op->drive_g->sectors = sectors; dprintf(1, "USB MSC blksize=%d sectors=%d\n", blksize, sectors);
- // Setup disk geometry translation. - setup_translation(op->drive_g); - // Register with bcv system. boot_add_hd(op->drive_g, -1);
diff --git a/src/virtio-blk.c b/src/virtio-blk.c index 834523a..6b49164 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -155,7 +155,6 @@ init_virtio_blk(u16 bdf) pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); vdrive_g->drive.desc = desc;
- setup_translation(&vdrive_g->drive); boot_add_hd(&vdrive_g->drive, bootprio_find_pci_device(bdf));
vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
The drive description is allocated with malloc_tmp() and is thus only available during the POST phase - boot_cdrom() is called during the boot phase. --- src/boot.c | 34 +++++++++++++++++----------------- 1 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/boot.c b/src/boot.c index d37e10f..8e33d53 100644 --- a/src/boot.c +++ b/src/boot.c @@ -275,27 +275,27 @@ interactive_bootmenu(void) pos = pos->next; }
+ // Get key press for (;;) { scan_code = get_keystroke(1000); - if (scan_code == 0x01) - // ESC + if (scan_code >= 1 && scan_code <= maxmenu+1) break; - if (scan_code < 1 || scan_code > maxmenu+1) - continue; - int choice = scan_code - 1; - - // Find entry and make top priority. - struct bootentry_s **pprev = &BootList; - while (--choice) - pprev = &(*pprev)->next; - pos = *pprev; - *pprev = pos->next; - pos->next = BootList; - BootList = pos; - pos->priority = 0; - break; } printf("\n"); + if (scan_code == 0x01) + // ESC + return; + + // Find entry and make top priority. + int choice = scan_code - 1; + struct bootentry_s **pprev = &BootList; + while (--choice) + pprev = &(*pprev)->next; + pos = *pprev; + *pprev = pos->next; + pos->next = BootList; + BootList = pos; + pos->priority = 0; }
static int HaveHDBoot, HaveFDBoot; @@ -431,7 +431,7 @@ boot_cdrom(struct ipl_entry_s *ie) struct drive_s *drive_g = (void*)ie->vector; int status = cdrom_boot(drive_g); if (status) { - printf("Boot failed: Could not read from CDROM %s (code %04x)\n", drive_g->desc, status); + printf("Boot failed: Could not read from CDROM (code %04x)\n", status); return; }
The "Drives" struct just held three global variables - declare the three global variables independently. --- src/block.c | 22 ++++++++++++---------- src/cdrom.c | 2 +- src/disk.c | 2 +- src/disk.h | 9 +-------- 4 files changed, 15 insertions(+), 20 deletions(-)
diff --git a/src/block.c b/src/block.c index dafaaa6..619db67 100644 --- a/src/block.c +++ b/src/block.c @@ -14,14 +14,16 @@ #include "usb-msc.h" // process_usb_op #include "virtio-blk.h" // process_virtio_op
-struct drives_s Drives VAR16VISIBLE; +u8 FloppyCount VAR16VISIBLE; +u8 CDCount; +struct drive_s *IDMap[3][CONFIG_MAX_EXTDRIVE] VAR16VISIBLE;
struct drive_s * getDrive(u8 exttype, u8 extdriveoffset) { - if (extdriveoffset >= ARRAY_SIZE(Drives.idmap[0])) + if (extdriveoffset >= ARRAY_SIZE(IDMap[0])) return NULL; - struct drive_s *drive_gf = GET_GLOBAL(Drives.idmap[exttype][extdriveoffset]); + struct drive_s *drive_gf = GET_GLOBAL(IDMap[exttype][extdriveoffset]); if (!drive_gf) return NULL; return GLOBALFLAT2GLOBAL(drive_gf); @@ -30,7 +32,7 @@ getDrive(u8 exttype, u8 extdriveoffset) int getDriveId(u8 exttype, struct drive_s *drive_g) { int i; - for (i = 0; i < ARRAY_SIZE(Drives.idmap[0]); i++) + for (i = 0; i < ARRAY_SIZE(IDMap[0]); i++) if (getDrive(exttype, i) == drive_g) return i; return -1; @@ -203,7 +205,7 @@ fill_fdpt(struct drive_s *drive_g, int hdid) static void add_drive(struct drive_s **idmap, u8 *count, struct drive_s *drive_g) { - if (*count >= ARRAY_SIZE(Drives.idmap[0])) { + if (*count >= ARRAY_SIZE(IDMap[0])) { warn_noalloc(); return; } @@ -219,7 +221,7 @@ map_hd_drive(struct drive_s *drive_g) struct bios_data_area_s *bda = MAKE_FLATPTR(SEG_BDA, 0); int hdid = bda->hdcount; dprintf(3, "Mapping hd drive %p to %d\n", drive_g, hdid); - add_drive(Drives.idmap[EXTTYPE_HD], &bda->hdcount, drive_g); + add_drive(IDMap[EXTTYPE_HD], &bda->hdcount, drive_g);
// Setup disk geometry translation. setup_translation(drive_g); @@ -233,7 +235,7 @@ void map_cd_drive(struct drive_s *drive_g) { dprintf(3, "Mapping cd drive %p\n", drive_g); - add_drive(Drives.idmap[EXTTYPE_CD], &Drives.cdcount, drive_g); + add_drive(IDMap[EXTTYPE_CD], &CDCount, drive_g); }
// Map a floppy @@ -241,14 +243,14 @@ void map_floppy_drive(struct drive_s *drive_g) { dprintf(3, "Mapping floppy drive %p\n", drive_g); - add_drive(Drives.idmap[EXTTYPE_FLOPPY], &Drives.floppycount, drive_g); + add_drive(IDMap[EXTTYPE_FLOPPY], &FloppyCount, drive_g);
// Update equipment word bits for floppy - if (Drives.floppycount == 1) { + if (FloppyCount == 1) { // 1 drive, ready for boot SETBITS_BDA(equipment_list_flags, 0x01); SET_BDA(floppy_harddisk_info, 0x07); - } else if (Drives.floppycount >= 2) { + } else if (FloppyCount >= 2) { // 2 drives, ready for boot SETBITS_BDA(equipment_list_flags, 0x41); SET_BDA(floppy_harddisk_info, 0x77); diff --git a/src/cdrom.c b/src/cdrom.c index 31ceaaa..3769dfa 100644 --- a/src/cdrom.c +++ b/src/cdrom.c @@ -109,7 +109,7 @@ cdemu_setup(void) { if (!CONFIG_CDROM_EMU) return; - if (!Drives.cdcount) + if (!CDCount) return;
struct drive_s *drive_g = malloc_fseg(sizeof(*drive_g)); diff --git a/src/disk.c b/src/disk.c index 56c369a..f7bfe9c 100644 --- a/src/disk.c +++ b/src/disk.c @@ -235,7 +235,7 @@ disk_1308(struct bregs *regs, struct drive_s *drive_g) u8 count; if (regs->dl < EXTSTART_HD) { // Floppy - count = GET_GLOBAL(Drives.floppycount); + count = GET_GLOBAL(FloppyCount);
if (CONFIG_CDROM_EMU && drive_g == GLOBALFLAT2GLOBAL(GET_GLOBAL(cdemu_drive_gf))) diff --git a/src/disk.h b/src/disk.h index 4802011..b85a62d 100644 --- a/src/disk.h +++ b/src/disk.h @@ -207,13 +207,6 @@ struct drive_s { #define TRANSLATION_LARGE 2 #define TRANSLATION_RECHS 3
-struct drives_s { - // map between bios floppy/hd/cd id and drive_s struct - u8 floppycount; - u8 cdcount; - struct drive_s *idmap[3][CONFIG_MAX_EXTDRIVE]; -}; - #define EXTTYPE_FLOPPY 0 #define EXTTYPE_HD 1 #define EXTTYPE_CD 2 @@ -227,7 +220,7 @@ struct drives_s { ****************************************************************/
// block.c -extern struct drives_s Drives; +extern u8 FloppyCount, CDCount; struct drive_s *getDrive(u8 exttype, u8 extdriveoffset); int getDriveId(u8 exttype, struct drive_s *drive_g); void map_floppy_drive(struct drive_s *drive_g);
--- src/boot.c | 7 ++++--- src/boot.h | 1 - 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/boot.c b/src/boot.c index 8e33d53..dacf639 100644 --- a/src/boot.c +++ b/src/boot.c @@ -86,6 +86,8 @@ int bootprio_find_named_rom(const char *name, int instance) * Boot setup ****************************************************************/
+static int CheckFloppySig = 1; + #define DEFAULT_PRIO 9999
static int DefaultFloppyPrio = 101; @@ -100,12 +102,11 @@ boot_setup(void) return;
SET_EBDA(boot_sequence, 0xffff); - IPL.checkfloppysig = 1;
if (!CONFIG_COREBOOT) { // On emulators, get boot order from nvram. if (inb_cmos(CMOS_BIOS_BOOTFLAG1) & 1) - IPL.checkfloppysig = 0; + CheckFloppySig = 0; u32 bootorder = (inb_cmos(CMOS_BIOS_BOOTFLAG2) | ((inb_cmos(CMOS_BIOS_BOOTFLAG1) & 0xf0) << 4)); DefaultFloppyPrio = DefaultCDPrio = DefaultHDPrio @@ -473,7 +474,7 @@ do_boot(u16 seq_nr) switch (ie->type) { case IPL_TYPE_FLOPPY: printf("Booting from Floppy...\n"); - boot_disk(0x00, IPL.checkfloppysig); + boot_disk(0x00, CheckFloppySig); break; case IPL_TYPE_HARDDISK: printf("Booting from Hard Disk...\n"); diff --git a/src/boot.h b/src/boot.h index fa455d5..7801ed5 100644 --- a/src/boot.h +++ b/src/boot.h @@ -15,7 +15,6 @@ struct ipl_entry_s { struct ipl_s { struct ipl_entry_s bev[8]; int bevcount; - int checkfloppysig; char **fw_bootorder; int fw_bootorder_count; };
Move the BEV storage to static variables in boot.c.
Also, increase the maximum number of BEV entries from 8 to 20. --- src/boot.c | 26 +++++++++++++++++++------- src/boot.h | 19 ------------------- 2 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/src/boot.c b/src/boot.c index dacf639..5ae418c 100644 --- a/src/boot.c +++ b/src/boot.c @@ -143,9 +143,15 @@ struct bootentry_s { const char *description; struct bootentry_s *next; }; - static struct bootentry_s *BootList;
+#define IPL_TYPE_FLOPPY 0x01 +#define IPL_TYPE_HARDDISK 0x02 +#define IPL_TYPE_CDROM 0x03 +#define IPL_TYPE_CBFS 0x20 +#define IPL_TYPE_BEV 0x80 +#define IPL_TYPE_BCV 0x81 + static void bootentry_add(int type, int prio, u32 data, const char *desc) { @@ -299,6 +305,12 @@ interactive_bootmenu(void) pos->priority = 0; }
+struct bev_s { + int type; + u32 vector; +}; +static struct bev_s BEV[20]; +static int BEVCount; static int HaveHDBoot, HaveFDBoot;
static void @@ -308,9 +320,9 @@ add_bev(int type, u32 vector) return; if (type == IPL_TYPE_FLOPPY && HaveFDBoot++) return; - if (IPL.bevcount >= ARRAY_SIZE(IPL.bev)) + if (BEVCount >= ARRAY_SIZE(BEV)) return; - struct ipl_entry_s *bev = &IPL.bev[IPL.bevcount++]; + struct bev_s *bev = &BEV[BEVCount++]; bev->type = type; bev->vector = vector; } @@ -420,7 +432,7 @@ boot_disk(u8 bootdrv, int checksig)
// Boot from a CD-ROM static void -boot_cdrom(struct ipl_entry_s *ie) +boot_cdrom(struct bev_s *ie) { if (! CONFIG_CDROM_BOOT) return; @@ -448,7 +460,7 @@ boot_cdrom(struct ipl_entry_s *ie)
// Boot from a CBFS payload static void -boot_cbfs(struct ipl_entry_s *ie) +boot_cbfs(struct bev_s *ie) { if (!CONFIG_COREBOOT || !CONFIG_COREBOOT_FLASH) return; @@ -462,7 +474,7 @@ do_boot(u16 seq_nr) if (! CONFIG_BOOT) panic("Boot support not compiled in.\n");
- if (seq_nr >= IPL.bevcount) { + if (seq_nr >= BEVCount) { printf("No bootable device.\n"); // Loop with irqs enabled - this allows ctrl+alt+delete to work. for (;;) @@ -470,7 +482,7 @@ do_boot(u16 seq_nr) }
// Boot the given BEV type. - struct ipl_entry_s *ie = &IPL.bev[seq_nr]; + struct bev_s *ie = &BEV[seq_nr]; switch (ie->type) { case IPL_TYPE_FLOPPY: printf("Booting from Floppy...\n"); diff --git a/src/boot.h b/src/boot.h index 7801ed5..94b175d 100644 --- a/src/boot.h +++ b/src/boot.h @@ -2,30 +2,11 @@ #ifndef __BOOT_H #define __BOOT_H
- -/**************************************************************** - * Initial Program Load (IPL) - ****************************************************************/ - -struct ipl_entry_s { - u16 type; - u32 vector; -}; - struct ipl_s { - struct ipl_entry_s bev[8]; - int bevcount; char **fw_bootorder; int fw_bootorder_count; };
-#define IPL_TYPE_FLOPPY 0x01 -#define IPL_TYPE_HARDDISK 0x02 -#define IPL_TYPE_CDROM 0x03 -#define IPL_TYPE_CBFS 0x20 -#define IPL_TYPE_BEV 0x80 -#define IPL_TYPE_BCV 0x81 -
/**************************************************************** * Function defs
--- src/boot.c | 17 +++++++++-------- src/boot.h | 10 ---------- 2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/boot.c b/src/boot.c index 5ae418c..9a395c6 100644 --- a/src/boot.c +++ b/src/boot.c @@ -14,13 +14,14 @@ #include "cmos.h" // inb_cmos #include "paravirt.h"
-struct ipl_s IPL; -
/**************************************************************** * Boot priority ordering ****************************************************************/
+static char **Bootorder; +static int BootorderCount; + static void loadBootOrder(void) { @@ -29,14 +30,14 @@ loadBootOrder(void) return;
int i; - IPL.fw_bootorder_count = 1; + BootorderCount = 1; while (f[i]) { if (f[i] == '\n') - IPL.fw_bootorder_count++; + BootorderCount++; i++; } - IPL.fw_bootorder = malloc_tmphigh(IPL.fw_bootorder_count*sizeof(char*)); - if (!IPL.fw_bootorder) { + Bootorder = malloc_tmphigh(BootorderCount*sizeof(char*)); + if (!Bootorder) { warn_noalloc(); free(f); return; @@ -45,12 +46,12 @@ loadBootOrder(void) dprintf(3, "boot order:\n"); i = 0; do { - IPL.fw_bootorder[i] = f; + Bootorder[i] = f; f = strchr(f, '\n'); if (f) { *f = '\0'; f++; - dprintf(3, "%d: %s\n", i, IPL.fw_bootorder[i]); + dprintf(3, "%d: %s\n", i, Bootorder[i]); i++; } } while(f); diff --git a/src/boot.h b/src/boot.h index 94b175d..107594b 100644 --- a/src/boot.h +++ b/src/boot.h @@ -2,16 +2,6 @@ #ifndef __BOOT_H #define __BOOT_H
-struct ipl_s { - char **fw_bootorder; - int fw_bootorder_count; -}; - - -/**************************************************************** - * Function defs - ****************************************************************/ - // boot.c extern struct ipl_s IPL; void boot_setup(void);
--- src/boot.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/src/boot.c b/src/boot.c index 9a395c6..5bb179d 100644 --- a/src/boot.c +++ b/src/boot.c @@ -10,9 +10,9 @@ #include "config.h" // CONFIG_* #include "disk.h" // cdrom_boot #include "bregs.h" // struct bregs -#include "boot.h" // struct ipl_s +#include "boot.h" // func defs #include "cmos.h" // inb_cmos -#include "paravirt.h" +#include "paravirt.h" // romfile_loadfile
/**************************************************************** @@ -306,6 +306,7 @@ interactive_bootmenu(void) pos->priority = 0; }
+// BEV (Boot Execution Vector) list struct bev_s { int type; u32 vector; @@ -381,13 +382,13 @@ boot_prep(void)
// Jump to a bootup entry point. static void -call_boot_entry(u16 bootseg, u16 bootip, u8 bootdrv) +call_boot_entry(struct segoff_s bootsegip, u8 bootdrv) { - dprintf(1, "Booting from %04x:%04x\n", bootseg, bootip); + dprintf(1, "Booting from %04x:%04x\n", bootsegip.seg, bootsegip.offset); struct bregs br; memset(&br, 0, sizeof(br)); br.flags = F_IF; - br.code = SEGOFF(bootseg, bootip); + br.code = bootsegip; // Set the magic number in ax and the boot drive in dl. br.dl = bootdrv; br.ax = 0xaa55; @@ -428,21 +429,17 @@ boot_disk(u8 bootdrv, int checksig) u16 bootip = (bootseg & 0x0fff) << 4; bootseg &= 0xf000;
- call_boot_entry(bootseg, bootip, bootdrv); + call_boot_entry(SEGOFF(bootseg, bootip), bootdrv); }
// Boot from a CD-ROM static void -boot_cdrom(struct bev_s *ie) +boot_cdrom(struct drive_s *drive_g) { if (! CONFIG_CDROM_BOOT) return; - - if (!ie->vector) - return; printf("Booting from DVD/CD...\n");
- struct drive_s *drive_g = (void*)ie->vector; int status = cdrom_boot(drive_g); if (status) { printf("Boot failed: Could not read from CDROM (code %04x)\n", status); @@ -456,19 +453,30 @@ boot_cdrom(struct bev_s *ie) u16 bootip = (bootseg & 0x0fff) << 4; bootseg &= 0xf000;
- call_boot_entry(bootseg, bootip, bootdrv); + call_boot_entry(SEGOFF(bootseg, bootip), bootdrv); }
// Boot from a CBFS payload static void -boot_cbfs(struct bev_s *ie) +boot_cbfs(struct cbfs_file *file) { if (!CONFIG_COREBOOT || !CONFIG_COREBOOT_FLASH) return; printf("Booting from CBFS...\n"); - cbfs_run_payload((void*)ie->vector); + cbfs_run_payload(file); +} + +// Boot from a BEV entry on an optionrom. +static void +boot_rom(u32 vector) +{ + printf("Booting from ROM...\n"); + struct segoff_s so; + so.segoff = vector; + call_boot_entry(so, 0); }
+// Determine next boot method and attempt a boot using it. static void do_boot(u16 seq_nr) { @@ -494,14 +502,13 @@ do_boot(u16 seq_nr) boot_disk(0x80, 1); break; case IPL_TYPE_CDROM: - boot_cdrom(ie); + boot_cdrom((void*)ie->vector); break; case IPL_TYPE_CBFS: - boot_cbfs(ie); + boot_cbfs((void*)ie->vector); break; case IPL_TYPE_BEV: - printf("Booting from ROM...\n"); - call_boot_entry(ie->vector >> 16, ie->vector & 0xffff, 0); + boot_rom(ie->vector); break; }
The description field is only available during the POST phase - it is confusing to have it live in a structure available through all phases.
The description was only used by the boot menu code - pass each drive description directly to the bootlist code.
Add a helper (znprintf) to automatically malloc_tmp the required space.
Also, fixup ramdisk handling - it was using an incorrect floppy priority. --- src/ahci.c | 36 ++++++++++++++++-------------------- src/ata.c | 38 +++++++++++++++++--------------------- src/boot.c | 14 +++++++------- src/boot.h | 6 +++--- src/coreboot.c | 5 +---- src/disk.h | 3 +-- src/floppy.c | 28 +++++++++++++++------------- src/output.c | 26 ++++++++++++++++++++++++++ src/ramdisk.c | 10 +++++++--- src/usb-msc.c | 29 +++++++++++++++-------------- src/util.h | 2 ++ src/virtio-blk.c | 10 ++++------ 12 files changed, 114 insertions(+), 93 deletions(-)
diff --git a/src/ahci.c b/src/ahci.c index 8fd2e14..b820e28 100644 --- a/src/ahci.c +++ b/src/ahci.c @@ -347,11 +347,6 @@ ahci_port_init(struct ahci_ctrl_s *ctrl, u32 pnr) port->drive.type = DTYPE_AHCI; port->drive.cntl_id = pnr; port->drive.removable = (buffer[0] & 0x80) ? 1 : 0; - port->drive.desc = malloc_tmp(MAXDESCSIZE); - if (!port->drive.desc) { - warn_noalloc(); - return NULL; - }
if (!port->atapi) { // found disk (ata) @@ -372,32 +367,33 @@ ahci_port_init(struct ahci_ctrl_s *ctrl, u32 pnr) adjsize >>= 10; adjprefix = 'G'; } - snprintf(port->drive.desc, MAXDESCSIZE - , "AHCI/%d: %s ATA-%d Hard-Disk (%u %ciBytes)" - , port->pnr - , ata_extract_model(model, MAXMODEL, buffer) - , ata_extract_version(buffer) - , (u32)adjsize, adjprefix); + char *desc = znprintf(MAXDESCSIZE + , "AHCI/%d: %s ATA-%d Hard-Disk (%u %ciBytes)" + , port->pnr + , ata_extract_model(model, MAXMODEL, buffer) + , ata_extract_version(buffer) + , (u32)adjsize, adjprefix); + dprintf(1, "%s\n", desc);
// Register with bcv system. - boot_add_hd(&port->drive, -1); + boot_add_hd(&port->drive, desc, -1); } else { // found cdrom (atapi) port->drive.blksize = CDROM_SECTOR_SIZE; port->drive.sectors = (u64)-1; u8 iscd = ((buffer[0] >> 8) & 0x1f) == 0x05; - snprintf(port->drive.desc, MAXDESCSIZE - , "DVD/CD [AHCI/%d: %s ATAPI-%d %s]" - , port->pnr - , ata_extract_model(model, MAXMODEL, buffer) - , ata_extract_version(buffer) - , (iscd ? "DVD/CD" : "Device")); + char *desc = znprintf(MAXDESCSIZE + , "DVD/CD [AHCI/%d: %s ATAPI-%d %s]" + , port->pnr + , ata_extract_model(model, MAXMODEL, buffer) + , ata_extract_version(buffer) + , (iscd ? "DVD/CD" : "Device")); + dprintf(1, "%s\n", desc);
// fill cdidmap if (iscd) - boot_add_cd(&port->drive, -1); + boot_add_cd(&port->drive, desc, -1); } - dprintf(1, "%s\n", port->drive.desc);
return port;
diff --git a/src/ata.c b/src/ata.c index 86b89b5..e9a2aef 100644 --- a/src/ata.c +++ b/src/ata.c @@ -739,16 +739,12 @@ ata_extract_model(char *model, u32 size, u16 *buffer) static struct atadrive_s * init_atadrive(struct atadrive_s *dummy, u16 *buffer) { - char *desc = malloc_tmp(MAXDESCSIZE); struct atadrive_s *adrive_g = malloc_fseg(sizeof(*adrive_g)); - if (!adrive_g || !desc) { + if (!adrive_g) { warn_noalloc(); - free(desc); - free(adrive_g); return NULL; } memset(adrive_g, 0, sizeof(*adrive_g)); - adrive_g->drive.desc = desc; adrive_g->chan_gf = dummy->chan_gf; adrive_g->slave = dummy->slave; adrive_g->drive.cntl_id = adrive_g->chan_gf->chanid * 2 + dummy->slave; @@ -774,20 +770,20 @@ init_drive_atapi(struct atadrive_s *dummy, u16 *buffer) adrive_g->drive.sectors = (u64)-1; u8 iscd = ((buffer[0] >> 8) & 0x1f) == 0x05; char model[MAXMODEL+1]; - snprintf(adrive_g->drive.desc, MAXDESCSIZE - , "DVD/CD [ata%d-%d: %s ATAPI-%d %s]" - , adrive_g->chan_gf->chanid, adrive_g->slave - , ata_extract_model(model, MAXMODEL, buffer) - , ata_extract_version(buffer) - , (iscd ? "DVD/CD" : "Device")); - dprintf(1, "%s\n", adrive_g->drive.desc); + char *desc = znprintf(MAXDESCSIZE + , "DVD/CD [ata%d-%d: %s ATAPI-%d %s]" + , adrive_g->chan_gf->chanid, adrive_g->slave + , ata_extract_model(model, MAXMODEL, buffer) + , ata_extract_version(buffer) + , (iscd ? "DVD/CD" : "Device")); + dprintf(1, "%s\n", desc);
// fill cdidmap if (iscd) { int prio = bootprio_find_ata_device(adrive_g->chan_gf->pci_bdf, adrive_g->chan_gf->chanid, adrive_g->slave); - boot_add_cd(&adrive_g->drive, prio); + boot_add_cd(&adrive_g->drive, desc, prio); }
return adrive_g; @@ -826,19 +822,19 @@ init_drive_ata(struct atadrive_s *dummy, u16 *buffer) adjprefix = 'G'; } char model[MAXMODEL+1]; - snprintf(adrive_g->drive.desc, MAXDESCSIZE - , "ata%d-%d: %s ATA-%d Hard-Disk (%u %ciBytes)" - , adrive_g->chan_gf->chanid, adrive_g->slave - , ata_extract_model(model, MAXMODEL, buffer) - , ata_extract_version(buffer) - , (u32)adjsize, adjprefix); - dprintf(1, "%s\n", adrive_g->drive.desc); + char *desc = znprintf(MAXDESCSIZE + , "ata%d-%d: %s ATA-%d Hard-Disk (%u %ciBytes)" + , adrive_g->chan_gf->chanid, adrive_g->slave + , ata_extract_model(model, MAXMODEL, buffer) + , ata_extract_version(buffer) + , (u32)adjsize, adjprefix); + dprintf(1, "%s\n", desc);
int prio = bootprio_find_ata_device(adrive_g->chan_gf->pci_bdf, adrive_g->chan_gf->chanid, adrive_g->slave); // Register with bcv system. - boot_add_hd(&adrive_g->drive, prio); + boot_add_hd(&adrive_g->drive, desc, prio);
return adrive_g; } diff --git a/src/boot.c b/src/boot.c index 5bb179d..e83dcdc 100644 --- a/src/boot.c +++ b/src/boot.c @@ -166,7 +166,7 @@ bootentry_add(int type, int prio, u32 data, const char *desc) be->type = type; be->priority = prio; be->data = data; - be->description = desc; + be->description = desc ?: "?";
// Add entry in sorted order. struct bootentry_s **pprev; @@ -215,24 +215,24 @@ boot_add_bcv(u16 seg, u16 ip, u16 desc, int prio) }
void -boot_add_floppy(struct drive_s *drive_g, int prio) +boot_add_floppy(struct drive_s *drive_g, const char *desc, int prio) { bootentry_add(IPL_TYPE_FLOPPY, defPrio(prio, DefaultFloppyPrio) - , (u32)drive_g, drive_g->desc); + , (u32)drive_g, desc); }
void -boot_add_hd(struct drive_s *drive_g, int prio) +boot_add_hd(struct drive_s *drive_g, const char *desc, int prio) { bootentry_add(IPL_TYPE_HARDDISK, defPrio(prio, DefaultHDPrio) - , (u32)drive_g, drive_g->desc); + , (u32)drive_g, desc); }
void -boot_add_cd(struct drive_s *drive_g, int prio) +boot_add_cd(struct drive_s *drive_g, const char *desc, int prio) { bootentry_add(IPL_TYPE_CDROM, defPrio(prio, DefaultCDPrio) - , (u32)drive_g, drive_g->desc); + , (u32)drive_g, desc); }
// Add a CBFS payload entry diff --git a/src/boot.h b/src/boot.h index 107594b..8f56ccf 100644 --- a/src/boot.h +++ b/src/boot.h @@ -8,9 +8,9 @@ void boot_setup(void); void boot_add_bev(u16 seg, u16 bev, u16 desc, int prio); void boot_add_bcv(u16 seg, u16 ip, u16 desc, int prio); struct drive_s; -void boot_add_floppy(struct drive_s *drive_g, int prio); -void boot_add_hd(struct drive_s *drive_g, int prio); -void boot_add_cd(struct drive_s *drive_g, int prio); +void boot_add_floppy(struct drive_s *drive_g, const char *desc, int prio); +void boot_add_hd(struct drive_s *drive_g, const char *desc, int prio); +void boot_add_cd(struct drive_s *drive_g, const char *desc, int prio); void boot_add_cbfs(void *data, const char *desc, int prio); void boot_prep(void); int bootprio_find_pci_device(int bdf); diff --git a/src/coreboot.c b/src/coreboot.c index 503a6d2..b4dfd8a 100644 --- a/src/coreboot.c +++ b/src/coreboot.c @@ -604,10 +604,7 @@ register_cbfs_payload(void) if (!file) break; const char *filename = cbfs_filename(file); - char *desc = malloc_tmp(MAXDESCSIZE); - if (!desc) - break; - snprintf(desc, MAXDESCSIZE, "Payload [%s]", &filename[4]); + char *desc = znprintf(MAXDESCSIZE, "Payload [%s]", &filename[4]); boot_add_cbfs(file, desc, -1); } } diff --git a/src/disk.h b/src/disk.h index b85a62d..f31de73 100644 --- a/src/disk.h +++ b/src/disk.h @@ -177,7 +177,6 @@ struct drive_s { u8 floppy_type; // Type of floppy (only for floppy drives). struct chs_s lchs; // Logical CHS u64 sectors; // Total sectors count - char *desc; // Drive description (only available during POST) u32 cntl_id; // Unique id for a given driver type. u8 removable; // Is media removable (currently unused)
@@ -232,7 +231,7 @@ int send_disk_op(struct disk_op_s *op); // floppy.c extern struct floppy_ext_dbt_s diskette_param_table2; void floppy_setup(void); -struct drive_s *addFloppy(int floppyid, int ftype, int driver); +struct drive_s *init_floppy(int floppyid, int ftype); int find_floppy_type(u32 size); int process_floppy_op(struct disk_op_s *op); void floppy_tick(void); diff --git a/src/floppy.c b/src/floppy.c index bfca63f..edc675d 100644 --- a/src/floppy.c +++ b/src/floppy.c @@ -92,38 +92,40 @@ struct floppyinfo_s FloppyInfo[] VAR16VISIBLE = { };
struct drive_s * -addFloppy(int floppyid, int ftype, int driver) +init_floppy(int floppyid, int ftype) { if (ftype <= 0 || ftype >= ARRAY_SIZE(FloppyInfo)) { dprintf(1, "Bad floppy type %d\n", ftype); return NULL; }
- char *desc = malloc_tmp(MAXDESCSIZE); struct drive_s *drive_g = malloc_fseg(sizeof(*drive_g)); - if (!drive_g || !desc) { + if (!drive_g) { warn_noalloc(); - free(desc); - free(drive_g); return NULL; } memset(drive_g, 0, sizeof(*drive_g)); drive_g->cntl_id = floppyid; - drive_g->type = driver; + drive_g->type = DTYPE_FLOPPY; drive_g->blksize = DISK_SECTOR_SIZE; drive_g->floppy_type = ftype; drive_g->sectors = (u64)-1; - drive_g->desc = desc; - snprintf(desc, MAXDESCSIZE, "Floppy [drive %c]", 'A' + floppyid);
memcpy(&drive_g->lchs, &FloppyInfo[ftype].chs , sizeof(FloppyInfo[ftype].chs)); + return drive_g; +}
+static void +addFloppy(int floppyid, int ftype) +{ + struct drive_s *drive_g = init_floppy(floppyid, ftype); + if (!drive_g) + return; + char *desc = znprintf(MAXDESCSIZE, "Floppy [drive %c]", 'A' + floppyid); int bdf = pci_find_class(PCI_CLASS_BRIDGE_ISA); /* isa-to-pci bridge */ int prio = bootprio_find_fdc_device(bdf, PORT_FD_BASE, floppyid); - - boot_add_floppy(drive_g, prio); - return drive_g; + boot_add_floppy(drive_g, desc, prio); }
void @@ -138,9 +140,9 @@ floppy_setup(void) } else { u8 type = inb_cmos(CMOS_FLOPPY_DRIVE_TYPE); if (type & 0xf0) - addFloppy(0, type >> 4, DTYPE_FLOPPY); + addFloppy(0, type >> 4); if (type & 0x0f) - addFloppy(1, type & 0x0f, DTYPE_FLOPPY); + addFloppy(1, type & 0x0f); }
outb(0x02, PORT_DMA1_MASK_REG); diff --git a/src/output.c b/src/output.c index 936d3d8..4c9f95b 100644 --- a/src/output.c +++ b/src/output.c @@ -148,6 +148,8 @@ putc(struct putcinfo *action, char c) static void puts(struct putcinfo *action, const char *s) { + if (!MODESEGMENT && !s) + s = "(NULL)"; for (; *s; s++) putc(action, *s); } @@ -407,6 +409,30 @@ snprintf(char *str, size_t size, const char *fmt, ...) return end - str; }
+// Build a formatted string - malloc'ing the memory. +char * +znprintf(size_t size, const char *fmt, ...) +{ + ASSERT32FLAT(); + if (!size) + return NULL; + char *str = malloc_tmp(size); + if (!str) { + warn_noalloc(); + return NULL; + } + struct snprintfinfo sinfo = { { putc_str }, str, str + size }; + va_list args; + va_start(args, fmt); + bvprintf(&sinfo.info, fmt, args); + va_end(args); + char *end = sinfo.str; + if (end >= sinfo.end) + end = sinfo.end - 1; + *end = '\0'; + return str; +} +
/**************************************************************** * Misc helpers diff --git a/src/ramdisk.c b/src/ramdisk.c index be1de56..5391376 100644 --- a/src/ramdisk.c +++ b/src/ramdisk.c @@ -9,6 +9,7 @@ #include "memmap.h" // add_e820 #include "biosvar.h" // GET_GLOBAL #include "bregs.h" // struct bregs +#include "boot.h" // boot_add_floppy
void ramdisk_setup(void) @@ -40,10 +41,13 @@ ramdisk_setup(void) cbfs_copyfile(file, pos, size);
// Setup driver. - dprintf(1, "Mapping CBFS floppy %s to addr %p\n", cbfs_filename(file), pos); - struct drive_s *drive_g = addFloppy((u32)pos, ftype, DTYPE_RAMDISK); + struct drive_s *drive_g = init_floppy((u32)pos, ftype); if (!drive_g) - strtcpy(drive_g->desc, cbfs_filename(file), MAXDESCSIZE); + return; + drive_g->type = DTYPE_RAMDISK; + dprintf(1, "Mapping CBFS floppy %s to addr %p\n", cbfs_filename(file), pos); + char *desc = znprintf(MAXDESCSIZE, "Ramdisk [%s]", cbfs_filename(file)); + boot_add_floppy(drive_g, desc, -1); }
static int diff --git a/src/usb-msc.c b/src/usb-msc.c index 877ece4..d5fe7ba 100644 --- a/src/usb-msc.c +++ b/src/usb-msc.c @@ -139,16 +139,16 @@ process_usb_op(struct disk_op_s *op) ****************************************************************/
static int -setup_drive_cdrom(struct disk_op_s *op) +setup_drive_cdrom(struct disk_op_s *op, char *desc) { op->drive_g->blksize = CDROM_SECTOR_SIZE; op->drive_g->sectors = (u64)-1; - boot_add_cd(op->drive_g, -1); + boot_add_cd(op->drive_g, desc, -1); return 0; }
static int -setup_drive_hd(struct disk_op_s *op) +setup_drive_hd(struct disk_op_s *op, char *desc) { struct cdbres_read_capacity info; int ret = cdb_read_capacity(op, &info); @@ -159,7 +159,7 @@ setup_drive_hd(struct disk_op_s *op) u32 blksize = ntohl(info.blksize), sectors = ntohl(info.sectors); if (blksize != DISK_SECTOR_SIZE) { if (blksize == CDROM_SECTOR_SIZE) - return setup_drive_cdrom(op); + return setup_drive_cdrom(op, desc); dprintf(1, "Unsupported USB MSC block size %d\n", blksize); return -1; } @@ -168,7 +168,7 @@ setup_drive_hd(struct disk_op_s *op) dprintf(1, "USB MSC blksize=%d sectors=%d\n", blksize, sectors);
// Register with bcv system. - boot_add_hd(op->drive_g, -1); + boot_add_hd(op->drive_g, desc, -1);
return 0; } @@ -192,9 +192,8 @@ usb_msc_init(struct usb_pipe *pipe }
// Allocate drive structure. - char *desc = malloc_tmphigh(MAXDESCSIZE); struct usbdrive_s *udrive_g = malloc_fseg(sizeof(*udrive_g)); - if (!udrive_g || !desc) { + if (!udrive_g) { warn_noalloc(); goto fail; } @@ -232,20 +231,22 @@ usb_msc_init(struct usb_pipe *pipe , strtcpy(rev, data.rev, sizeof(rev)) , pdt, removable); udrive_g->drive.removable = removable; - snprintf(desc, MAXDESCSIZE, "USB Drive %s %s %s", vendor, product, rev); - udrive_g->drive.desc = desc;
- if (pdt == USB_MSC_TYPE_CDROM) - ret = setup_drive_cdrom(&dop); - else - ret = setup_drive_hd(&dop); + if (pdt == USB_MSC_TYPE_CDROM) { + char *desc = znprintf(MAXDESCSIZE, "DVD/CD [USB Drive %s %s %s]" + , vendor, product, rev); + ret = setup_drive_cdrom(&dop, desc); + } else { + char *desc = znprintf(MAXDESCSIZE, "USB Drive %s %s %s" + , vendor, product, rev); + ret = setup_drive_hd(&dop, desc); + } if (ret) goto fail;
return 0; fail: dprintf(1, "Unable to configure USB MSC device.\n"); - free(desc); free(udrive_g); return -1; } diff --git a/src/util.h b/src/util.h index f5b9446..7102c27 100644 --- a/src/util.h +++ b/src/util.h @@ -237,6 +237,8 @@ void printf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2))); int snprintf(char *str, size_t size, const char *fmt, ...) __attribute__ ((format (printf, 3, 4))); +char * znprintf(size_t size, const char *fmt, ...) + __attribute__ ((format (printf, 2, 3))); void __dprintf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2))); void __debug_enter(struct bregs *regs, const char *fname); diff --git a/src/virtio-blk.c b/src/virtio-blk.c index 6b49164..153399d 100644 --- a/src/virtio-blk.c +++ b/src/virtio-blk.c @@ -101,10 +101,9 @@ init_virtio_blk(u16 bdf) { dprintf(1, "found virtio-blk at %x:%x\n", pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); - char *desc = malloc_tmphigh(MAXDESCSIZE); struct virtiodrive_s *vdrive_g = malloc_fseg(sizeof(*vdrive_g)); struct vring_virtqueue *vq = memalign_low(PAGE_SIZE, sizeof(*vq)); - if (!vdrive_g || !desc || !vq) { + if (!vdrive_g || !vq) { warn_noalloc(); goto fail; } @@ -151,11 +150,10 @@ init_virtio_blk(u16 bdf) vdrive_g->drive.pchs.cylinders = cfg.cylinders; vdrive_g->drive.pchs.heads = cfg.heads; vdrive_g->drive.pchs.spt = cfg.sectors; - snprintf(desc, MAXDESCSIZE, "Virtio disk PCI:%x:%x", - pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf)); - vdrive_g->drive.desc = desc; + char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%x:%x", + pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf));
- boot_add_hd(&vdrive_g->drive, bootprio_find_pci_device(bdf)); + boot_add_hd(&vdrive_g->drive, desc, bootprio_find_pci_device(bdf));
vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
On Wed, Dec 29, 2010 at 12:46:45PM -0500, Kevin O'Connor wrote:
Some cleanups I came across while reviewing the boot code.
Gleb - patch 7 conflicts with your last patch - I hope it isn't a big deal to fixup though. (I'm still reviewing your last patch.)
The remaining patch is pretty independent. Should be very easy to rebase. Do you want me to do it?
-Kevin
Kevin O'Connor (9): Rename add_ordered_drive() to add_drive() and use in map_hd_drive(). Call setup_translation() from map_hd_drive(). Don't access drive_g->desc from boot_cdrom(). Remove Drives global struct in favor of independent global variables. Move IPL.checkfloppysig to a global (CheckFloppySig) in boot.c. Move IPL.bev to static variables in boot.c Move IPL.fw_bootorder to static variables in boot.c. Minor reorganization of some of the boot_xxx code in boot.c. Remove drive->desc field.
src/ahci.c | 40 +++++++--------- src/ata.c | 41 +++++++--------- src/block.c | 64 ++++++++++++------------- src/boot.c | 137 +++++++++++++++++++++++++++++++----------------------- src/boot.h | 36 +------------- src/cdrom.c | 2 +- src/coreboot.c | 5 +-- src/disk.c | 2 +- src/disk.h | 13 +---- src/floppy.c | 28 ++++++----- src/output.c | 26 ++++++++++ src/ramdisk.c | 10 +++- src/usb-msc.c | 32 ++++++------- src/util.h | 2 + src/virtio-blk.c | 11 ++--- 15 files changed, 221 insertions(+), 228 deletions(-)
-- 1.7.3.4
-- Gleb.
On Wed, Dec 29, 2010 at 09:00:36PM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 12:46:45PM -0500, Kevin O'Connor wrote:
Some cleanups I came across while reviewing the boot code.
Gleb - patch 7 conflicts with your last patch - I hope it isn't a big deal to fixup though. (I'm still reviewing your last patch.)
The remaining patch is pretty independent. Should be very easy to rebase. Do you want me to do it?
It wouldn't hurt.
BTW, I was thinking of ways to try and simplify this code. What if we tried doing an exact string match for the device, but if that fails and a BDF is known for the device then do a prefix string search for the PCI device?
-Kevin
On Wed, Dec 29, 2010 at 02:10:16PM -0500, Kevin O'Connor wrote:
On Wed, Dec 29, 2010 at 09:00:36PM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 12:46:45PM -0500, Kevin O'Connor wrote:
Some cleanups I came across while reviewing the boot code.
Gleb - patch 7 conflicts with your last patch - I hope it isn't a big deal to fixup though. (I'm still reviewing your last patch.)
The remaining patch is pretty independent. Should be very easy to rebase. Do you want me to do it?
It wouldn't hurt.
BTW, I was thinking of ways to try and simplify this code. What if we tried doing an exact string match for the device, but if that fails and a BDF is known for the device then do a prefix string search for the PCI device?
I don't see why this would be simpler. Also how can you do exact string match if BDF is not knows? You can't create device path without knowing BDF (well DF). My code works like OpenFirmware device path was designed to work: match device path one node at a time. This also allows for greater flexibility in qemu<->seabios interface.
-- Gleb.
On Wed, Dec 29, 2010 at 10:29:57PM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 02:10:16PM -0500, Kevin O'Connor wrote:
BTW, I was thinking of ways to try and simplify this code. What if we tried doing an exact string match for the device, but if that fails and a BDF is known for the device then do a prefix string search for the PCI device?
I don't see why this would be simpler. Also how can you do exact string match if BDF is not knows? You can't create device path without knowing BDF (well DF). My code works like OpenFirmware device path was designed to work: match device path one node at a time. This also allows for greater flexibility in qemu<->seabios interface.
Playing around with this a little further, I came up with the below. I'm not sure if it's better.
I added a "magic" syntax to support rom instances (eg, /rom2@genroms/linuxboot.bin, /pci@i0cf8/scsi@3/rom2). I also stubbed out some (admittedly wrong) pci bus path support.
Do you know what the magic qemu command-line options are to enable bootindex for roms, floppies, scsi, etc is?
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..836a8b2 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i; + int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n') @@ -51,35 +52,122 @@ loadBootOrder(void) if (f) { *f = '\0'; f++; - dprintf(3, "%d: %s\n", i, Bootorder[i]); i++; } + dprintf(3, "%d: %s\n", i, Bootorder[i]); } while(f); }
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) { + for (;;) { + if (!*glob && (!*str || *str == '/')) + return (char*)str; + if (*glob == *str) { + glob++; + str++; + continue; + } + if (*glob != '*') + return NULL; + if (*str && *str != '/' && *str != glob[1]) + str++; + else + glob++; + } +} + +// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) +{ + int i; + for (i = 0; i < BootorderCount; i++) + if (glob_prefix(glob, Bootorder[i])) + return i; return -1; }
+#define FW_PCI_DOMAIN "/pci@i0cf8" + +static char * +build_pci_path(char *buf, int max, int bdf) +{ + // Build a glob string for a bdf - for example: /pci@i0cf8/*@1,2 + char *p = buf; + int bus = pci_bdf_to_bus(bdf); + if (bus) + // XXX - this isn't the correct path syntax + p += snprintf(p, buf+max-p, "/bus%x", bus); + + int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf); + if (!fn) + snprintf(buf, buf+max-p, "%s/*@%x", FW_PCI_DOMAIN, dev); + else + snprintf(buf, buf+max-p, "%s/*@%x,%x", FW_PCI_DOMAIN, dev, fn); + return buf; +} + +int bootprio_find_pci_device(int bdf) +{ + // Find pci device - for example: /pci@i0cf8/ethernet@5 + char pci[256]; + return find_prio(build_pci_path(pci, sizeof(pci), bdf)); +} + int bootprio_find_ata_device(int bdf, int chanid, int slave) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0 + char pci[256], desc[256]; + snprintf(desc, sizeof(desc), "%s/drive@%x/disk@%x" + , build_pci_path(pci, sizeof(pci), bdf), chanid, slave); + return find_prio(desc); }
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0 + char pci[256], desc[256]; + snprintf(desc, sizeof(desc), "%s/fdc@%04x/floppy@%x" + , build_pci_path(pci, sizeof(pci), bdf), port, fdid); + return find_prio(desc); }
int bootprio_find_pci_rom(int bdf, int instance) { - return -1; + // Find pci rom - for example: /pci@i0cf8/scsi@3/rom2 + char pci[256], desc[256]; + build_pci_path(pci, sizeof(pci), bdf); + if (instance) + snprintf(desc, sizeof(desc), "%s/rom%x", pci, instance); + else + snprintf(desc, sizeof(desc), "%s/rom", pci); + int prio = find_prio(desc); + if (prio >= 0) + return prio; + // Didn't find rom string - try to match just the pci device. + return find_prio(pci); }
int bootprio_find_named_rom(const char *name, int instance) { - return -1; + // Find named rom - for example: /rom@genroms/linuxboot.bin + char desc[256]; + if (instance) + snprintf(desc, sizeof(desc), "/rom@%s", name); + else + snprintf(desc, sizeof(desc), "/rom%d@%s", instance, name); + return find_prio(desc); }
On Wed, Dec 29, 2010 at 08:23:13PM -0500, Kevin O'Connor wrote:
On Wed, Dec 29, 2010 at 10:29:57PM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 02:10:16PM -0500, Kevin O'Connor wrote:
BTW, I was thinking of ways to try and simplify this code. What if we tried doing an exact string match for the device, but if that fails and a BDF is known for the device then do a prefix string search for the PCI device?
I don't see why this would be simpler. Also how can you do exact string match if BDF is not knows? You can't create device path without knowing BDF (well DF). My code works like OpenFirmware device path was designed to work: match device path one node at a time. This also allows for greater flexibility in qemu<->seabios interface.
Playing around with this a little further, I came up with the below. I'm not sure if it's better.
For me it looks more complicated (may be because I am more familiar with my own code ;)) I see that you are starting to add regular expressions engine!
I added a "magic" syntax to support rom instances (eg, /rom2@genroms/linuxboot.bin, /pci@i0cf8/scsi@3/rom2). I also stubbed out some (admittedly wrong) pci bus path support.
For supporting multiple pci buses we need to keep track of pci bus topology in upper layers. Bus number in bdf is meaningless for device path purposes. Just drop this code for good.
To boot from a certain file on a device OpenFirmware uses following syntax: /path/to/device:filename i.e parameter goes after ':'. To stay with OpenFirmware spirit I would suggest to use the following syntax for rom instances: /rom@genroms/linuxboot.bin:rom2 /pci@i0cf8/scsi@3:rom2
I can add support for such syntax to my patch too.
Do you know what the magic qemu command-line options are to enable bootindex for roms, floppies, scsi, etc is?
You need latest qemu for that (not qemu-kvm):
qemu -drive file=/dev/null,if=none,id=d1,media=cdrom -device ide-drive,drive=d1,bootindex=0 \ -drive file=fedora.iso,if=none,id=d2,media=cdrom -device ide-drive,drive=d2,bootindex=110 \ -drive if=none,file=/tmp/a,id=fda \ -drive if=none,file=/tmp/a,id=fdb \ -global isa-fdc.bootindexB=30 \ -global isa-fdc.bootindexA=40 \ -global isa-fdc.driveA=fda \ -global isa-fdc.driveB=fdb \ -netdev type=user,id=net1 -device virtio-net-pci,netdev=net1,bootindex=20 \ -netdev type=user,id=net2 -device e1000,netdev=net2,bootindex=10 \ -drive file=rhel6.qcow2,if=none,id=d3 -device ide-drive,drive=d3,bus=ide.0,bootindex=100 \ -drive file=/dev/null,if=none,id=d4 -device virtio-blk-pci,drive=d4,bootindex=95 \ -boot menu=on
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..836a8b2 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i;
- int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n')
@@ -51,35 +52,122 @@ loadBootOrder(void) if (f) { *f = '\0'; f++;
dprintf(3, "%d: %s\n", i, Bootorder[i]); i++; }
dprintf(3, "%d: %s\n", i, Bootorder[i]);}
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) {
- for (;;) {
if (!*glob && (!*str || *str == '/'))return (char*)str;if (*glob == *str) {glob++;str++;continue;}if (*glob != '*')return NULL;if (*str && *str != '/' && *str != glob[1])str++;elseglob++;- }
+}
+// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) +{
- int i;
- for (i = 0; i < BootorderCount; i++)
if (glob_prefix(glob, Bootorder[i]))return i;}
+#define FW_PCI_DOMAIN "/pci@i0cf8"
+static char * +build_pci_path(char *buf, int max, int bdf) +{
- // Build a glob string for a bdf - for example: /pci@i0cf8/*@1,2
- char *p = buf;
- int bus = pci_bdf_to_bus(bdf);
- if (bus)
// XXX - this isn't the correct path syntaxp += snprintf(p, buf+max-p, "/bus%x", bus);- int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf);
- if (!fn)
snprintf(buf, buf+max-p, "%s/*@%x", FW_PCI_DOMAIN, dev);- else
snprintf(buf, buf+max-p, "%s/*@%x,%x", FW_PCI_DOMAIN, dev, fn);- return buf;
+}
+int bootprio_find_pci_device(int bdf) +{
- // Find pci device - for example: /pci@i0cf8/ethernet@5
- char pci[256];
- return find_prio(build_pci_path(pci, sizeof(pci), bdf));
+}
int bootprio_find_ata_device(int bdf, int chanid, int slave) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0
- char pci[256], desc[256];
- snprintf(desc, sizeof(desc), "%s/drive@%x/disk@%x"
, build_pci_path(pci, sizeof(pci), bdf), chanid, slave);- return find_prio(desc);
}
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0
- char pci[256], desc[256];
- snprintf(desc, sizeof(desc), "%s/fdc@%04x/floppy@%x"
, build_pci_path(pci, sizeof(pci), bdf), port, fdid);- return find_prio(desc);
}
int bootprio_find_pci_rom(int bdf, int instance) {
- return -1;
- // Find pci rom - for example: /pci@i0cf8/scsi@3/rom2
- char pci[256], desc[256];
- build_pci_path(pci, sizeof(pci), bdf);
- if (instance)
snprintf(desc, sizeof(desc), "%s/rom%x", pci, instance);- else
snprintf(desc, sizeof(desc), "%s/rom", pci);- int prio = find_prio(desc);
- if (prio >= 0)
return prio;- // Didn't find rom string - try to match just the pci device.
- return find_prio(pci);
}
int bootprio_find_named_rom(const char *name, int instance) {
- return -1;
- // Find named rom - for example: /rom@genroms/linuxboot.bin
- char desc[256];
- if (instance)
snprintf(desc, sizeof(desc), "/rom@%s", name);- else
snprintf(desc, sizeof(desc), "/rom%d@%s", instance, name);- return find_prio(desc);
}
-- Gleb.
On Thu, Dec 30, 2010 at 08:01:15AM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 08:23:13PM -0500, Kevin O'Connor wrote:
Playing around with this a little further, I came up with the below. I'm not sure if it's better.
For me it looks more complicated (may be because I am more familiar with my own code ;))
The basic idea is to completely build the search string in the bootprio_find_X() function and then have it call find_prio() to return the priority.
I see that you are starting to add regular expressions engine!
Yes - I'm not happy about that. Ultimately, rom loading needs some "fuzzy" matching capability, though.
I added a "magic" syntax to support rom instances (eg, /rom2@genroms/linuxboot.bin, /pci@i0cf8/scsi@3/rom2). I also stubbed out some (admittedly wrong) pci bus path support.
For supporting multiple pci buses we need to keep track of pci bus topology in upper layers. Bus number in bdf is meaningless for device path purposes.
Agreed. I just need some way to make sure a bus2 device doesn't grab the priority of a bus0 request.
To boot from a certain file on a device OpenFirmware uses following syntax: /path/to/device:filename i.e parameter goes after ':'.
Thanks.
qemu -drive file=/dev/null,if=none,id=d1,media=cdrom -device ide-drive,drive=d1,bootindex=0 \
Thanks.
I've updated the patch - see below.
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..b47f0fe 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i; + int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n') @@ -48,38 +49,119 @@ loadBootOrder(void) do { Bootorder[i] = f; f = strchr(f, '\n'); - if (f) { - *f = '\0'; - f++; - dprintf(3, "%d: %s\n", i, Bootorder[i]); - i++; - } + if (f) + *(f++) = '\0'; + dprintf(3, "%d: %s\n", i+1, Bootorder[i]); + i++; } while(f); }
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) { + for (;;) { + if (!*glob && (!*str || *str == '/')) + return (char*)str; + if (*glob == *str) { + glob++; + str++; + continue; + } + if (*glob != '*') + return NULL; + if (*str && *str != '/' && *str != glob[1]) + str++; + else + glob++; + } +} + +// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) +{ + int i; + for (i = 0; i < BootorderCount; i++) + if (glob_prefix(glob, Bootorder[i])) + return i+1; return -1; }
+#define FW_PCI_DOMAIN "/pci@i0cf8" + +static char * +build_pci_path(char *buf, int max, const char *devname, int bdf) +{ + // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2 + char *p = buf; + int bus = pci_bdf_to_bus(bdf); + if (bus) + // XXX - this isn't the correct path syntax + p += snprintf(p, max, "/bus%x", bus); + + int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf); + if (fn) + snprintf(p, buf+max-p, "%s/%s@%x,%x", FW_PCI_DOMAIN, devname, dev, fn); + else + snprintf(p, buf+max-p, "%s/%s@%x", FW_PCI_DOMAIN, devname, dev); + return buf; +} + +int bootprio_find_pci_device(int bdf) +{ + // Find pci device - for example: /pci@i0cf8/ethernet@5 + char pci[256]; + return find_prio(build_pci_path(pci, sizeof(pci), "*", bdf)); +} + int bootprio_find_ata_device(int bdf, int chanid, int slave) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0 + char pci[256], desc[256]; + build_pci_path(pci, sizeof(pci), "*", bdf); + snprintf(desc, sizeof(desc), "%s/drive@%x/disk@%x", pci, chanid, slave); + return find_prio(desc); }
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0 + char pci[256], desc[256]; + build_pci_path(pci, sizeof(pci), "isa", bdf); + snprintf(desc, sizeof(desc), "%s/fdc@%04x/floppy@%x", pci, port, fdid); + return find_prio(desc); }
int bootprio_find_pci_rom(int bdf, int instance) { - return -1; + // Find pci rom - for example: /pci@i0cf8/scsi@3:rom2 + char pci[256], desc[256]; + build_pci_path(pci, sizeof(pci), "*", bdf); + if (instance) { + snprintf(desc, sizeof(desc), "%s:rom%d", pci, instance); + return find_prio(desc); + } + return find_prio(pci); }
int bootprio_find_named_rom(const char *name, int instance) { - return -1; + // Find named rom - for example: /rom@genroms/linuxboot.bin + char desc[256]; + if (instance) + snprintf(desc, sizeof(desc), "/rom@%s:rom%d", name, instance); + else + snprintf(desc, sizeof(desc), "/rom@%s", name); + return find_prio(desc); }
@@ -167,6 +249,8 @@ bootentry_add(int type, int prio, u32 data, const char *desc) be->priority = prio; be->data = data; be->description = desc ?: "?"; + dprintf(3, "Registering bootable: %s (type:%d prio:%d data:%x)\n" + , be->description, type, prio, data);
// Add entry in sorted order. struct bootentry_s **pprev;
On Thu, Dec 30, 2010 at 12:20:17PM -0500, Kevin O'Connor wrote:
On Thu, Dec 30, 2010 at 08:01:15AM +0200, Gleb Natapov wrote:
On Wed, Dec 29, 2010 at 08:23:13PM -0500, Kevin O'Connor wrote:
Playing around with this a little further, I came up with the below. I'm not sure if it's better.
For me it looks more complicated (may be because I am more familiar with my own code ;))
The basic idea is to completely build the search string in the bootprio_find_X() function and then have it call find_prio() to return the priority.
I understand the code, I just don't see why this is easier. Your code also uses a lot of stack is this OK? It also assumes that device path is no greater then 256 bytes long. With multiple bridges we can go over this limit.
I see that you are starting to add regular expressions engine!
Yes - I'm not happy about that. Ultimately, rom loading needs some "fuzzy" matching capability, though.
I added a "magic" syntax to support rom instances (eg, /rom2@genroms/linuxboot.bin, /pci@i0cf8/scsi@3/rom2). I also stubbed out some (admittedly wrong) pci bus path support.
For supporting multiple pci buses we need to keep track of pci bus topology in upper layers. Bus number in bdf is meaningless for device path purposes.
Agreed. I just need some way to make sure a bus2 device doesn't grab the priority of a bus0 request.
Ah yeah. This will do the trick.
To boot from a certain file on a device OpenFirmware uses following syntax: /path/to/device:filename i.e parameter goes after ':'.
Thanks.
qemu -drive file=/dev/null,if=none,id=d1,media=cdrom -device ide-drive,drive=d1,bootindex=0 \
Thanks.
I've updated the patch - see below.
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..b47f0fe 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i;
- int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n')
@@ -48,38 +49,119 @@ loadBootOrder(void) do { Bootorder[i] = f; f = strchr(f, '\n');
if (f) {*f = '\0';f++;dprintf(3, "%d: %s\n", i, Bootorder[i]);i++;}
if (f)*(f++) = '\0';dprintf(3, "%d: %s\n", i+1, Bootorder[i]);i++;}
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) {
- for (;;) {
if (!*glob && (!*str || *str == '/'))return (char*)str;if (*glob == *str) {glob++;str++;continue;}if (*glob != '*')return NULL;if (*str && *str != '/' && *str != glob[1])str++;elseglob++;- }
+}
+// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) +{
- int i;
- for (i = 0; i < BootorderCount; i++)
if (glob_prefix(glob, Bootorder[i]))return i+1;}
+#define FW_PCI_DOMAIN "/pci@i0cf8"
+static char * +build_pci_path(char *buf, int max, const char *devname, int bdf) +{
- // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2
- char *p = buf;
- int bus = pci_bdf_to_bus(bdf);
- if (bus)
// XXX - this isn't the correct path syntaxp += snprintf(p, max, "/bus%x", bus);- int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf);
- if (fn)
snprintf(p, buf+max-p, "%s/%s@%x,%x", FW_PCI_DOMAIN, devname, dev, fn);- else
snprintf(p, buf+max-p, "%s/%s@%x", FW_PCI_DOMAIN, devname, dev);- return buf;
+}
+int bootprio_find_pci_device(int bdf) +{
- // Find pci device - for example: /pci@i0cf8/ethernet@5
- char pci[256];
- return find_prio(build_pci_path(pci, sizeof(pci), "*", bdf));
+}
int bootprio_find_ata_device(int bdf, int chanid, int slave) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0
- char pci[256], desc[256];
- build_pci_path(pci, sizeof(pci), "*", bdf);
- snprintf(desc, sizeof(desc), "%s/drive@%x/disk@%x", pci, chanid, slave);
- return find_prio(desc);
}
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0
- char pci[256], desc[256];
- build_pci_path(pci, sizeof(pci), "isa", bdf);
- snprintf(desc, sizeof(desc), "%s/fdc@%04x/floppy@%x", pci, port, fdid);
- return find_prio(desc);
}
int bootprio_find_pci_rom(int bdf, int instance) {
- return -1;
- // Find pci rom - for example: /pci@i0cf8/scsi@3:rom2
- char pci[256], desc[256];
- build_pci_path(pci, sizeof(pci), "*", bdf);
- if (instance) {
snprintf(desc, sizeof(desc), "%s:rom%d", pci, instance);return find_prio(desc);- }
- return find_prio(pci);
}
int bootprio_find_named_rom(const char *name, int instance) {
- return -1;
- // Find named rom - for example: /rom@genroms/linuxboot.bin
- char desc[256];
- if (instance)
snprintf(desc, sizeof(desc), "/rom@%s:rom%d", name, instance);- else
snprintf(desc, sizeof(desc), "/rom@%s", name);- return find_prio(desc);
}
@@ -167,6 +249,8 @@ bootentry_add(int type, int prio, u32 data, const char *desc) be->priority = prio; be->data = data; be->description = desc ?: "?";
dprintf(3, "Registering bootable: %s (type:%d prio:%d data:%x)\n"
, be->description, type, prio, data);// Add entry in sorted order. struct bootentry_s **pprev;
-- Gleb.
On Thu, Dec 30, 2010 at 07:55:27PM +0200, Gleb Natapov wrote:
On Thu, Dec 30, 2010 at 12:20:17PM -0500, Kevin O'Connor wrote:
On Thu, Dec 30, 2010 at 08:01:15AM +0200, Gleb Natapov wrote:
For me it looks more complicated (may be because I am more familiar with my own code ;))
The basic idea is to completely build the search string in the bootprio_find_X() function and then have it call find_prio() to return the priority.
I understand the code, I just don't see why this is easier.
It does seem a little more compact to me. Shrug - it's probably because I'm more familiar with my code. :-)
Your code also uses a lot of stack is this OK?
It's ~600 bytes - since the code runs in 32-bit flat mode, it shouldn't be a problem. (The closest limit would be each "thread" stack of 4K.)
There's a tool to try and scan stack usage (it's for the 16-bit code, but it can also run on the 32bit code):
objdump -m i386 -M suffix -d out/rom.o | tools/checkstack.py | less
Nothing is close to 4k.
It also assumes that device path is no greater then 256 bytes long. With multiple bridges we can go over this limit.
Assuming each bridge takes 15 bytes to describe, one would need around 12 layers of bridges before this was an issue. I don't think that's realistic.
-Kevin
On Thu, Dec 30, 2010 at 01:56:28PM -0500, Kevin O'Connor wrote:
On Thu, Dec 30, 2010 at 07:55:27PM +0200, Gleb Natapov wrote:
On Thu, Dec 30, 2010 at 12:20:17PM -0500, Kevin O'Connor wrote:
On Thu, Dec 30, 2010 at 08:01:15AM +0200, Gleb Natapov wrote:
For me it looks more complicated (may be because I am more familiar with my own code ;))
The basic idea is to completely build the search string in the bootprio_find_X() function and then have it call find_prio() to return the priority.
I understand the code, I just don't see why this is easier.
It does seem a little more compact to me. Shrug - it's probably because I'm more familiar with my code. :-)
Probably :) FWIW I ran a guest with your patch and it works for me.
Your code also uses a lot of stack is this OK?
It's ~600 bytes - since the code runs in 32-bit flat mode, it shouldn't be a problem. (The closest limit would be each "thread" stack of 4K.)
There's a tool to try and scan stack usage (it's for the 16-bit code, but it can also run on the 32bit code):
objdump -m i386 -M suffix -d out/rom.o | tools/checkstack.py | less
Nothing is close to 4k.
OK.
It also assumes that device path is no greater then 256 bytes long. With multiple bridges we can go over this limit.
Assuming each bridge takes 15 bytes to describe, one would need around 12 layers of bridges before this was an issue. I don't think that's realistic.
Each node consist of name@address. IIRC name can be max 32 bytes. Address is unlimited but the longest address I can think of is 64bit integer written in hex, this is 16 bites more. On a PC we have very limited number of possible devices and none with such long names and deep nesting, so probably this is not the big issue.
-- Gleb.
On Thu, Dec 30, 2010 at 09:31:56PM +0200, Gleb Natapov wrote:
On Thu, Dec 30, 2010 at 01:56:28PM -0500, Kevin O'Connor wrote:
Assuming each bridge takes 15 bytes to describe, one would need around 12 layers of bridges before this was an issue. I don't think that's realistic.
Each node consist of name@address. IIRC name can be max 32 bytes. Address is unlimited but the longest address I can think of is 64bit integer written in hex, this is 16 bites more. On a PC we have very limited number of possible devices and none with such long names and deep nesting, so probably this is not the big issue.
Well, it was pointless for my patch to store the pci path on the stack twice, so I redid that. As you say, though, it's unlikely to be an issue - the size can always be increased if it does become an issue.
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..dd719f3 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i; + int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n') @@ -48,38 +49,117 @@ loadBootOrder(void) do { Bootorder[i] = f; f = strchr(f, '\n'); - if (f) { - *f = '\0'; - f++; - dprintf(3, "%d: %s\n", i, Bootorder[i]); - i++; - } + if (f) + *(f++) = '\0'; + dprintf(3, "%d: %s\n", i+1, Bootorder[i]); + i++; } while(f); }
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) +{ + for (;;) { + if (!*glob && (!*str || *str == '/')) + return (char*)str; + if (*glob == *str) { + glob++; + str++; + continue; + } + if (*glob != '*') + return NULL; + if (*str && *str != '/' && *str != glob[1]) + str++; + else + glob++; + } +} + +// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) { + dprintf(1, "Searching bootorder for: %s\n", glob); + int i; + for (i = 0; i < BootorderCount; i++) + if (glob_prefix(glob, Bootorder[i])) + return i+1; return -1; }
+#define FW_PCI_DOMAIN "/pci@i0cf8" + +static char * +build_pci_path(char *buf, int max, const char *devname, int bdf) +{ + // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2 + char *p = buf; + int bus = pci_bdf_to_bus(bdf); + if (bus) + // XXX - this isn't the correct path syntax + p += snprintf(p, max, "/bus%x", bus); + + int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf); + p += snprintf(p, buf+max-p, "%s/%s@%x", FW_PCI_DOMAIN, devname, dev); + if (fn) + p += snprintf(p, buf+max-p, ",%x", fn); + return p; +} + +int bootprio_find_pci_device(int bdf) +{ + // Find pci device - for example: /pci@i0cf8/ethernet@5 + char desc[256]; + build_pci_path(desc, sizeof(desc), "*", bdf); + return find_prio(desc); +} + int bootprio_find_ata_device(int bdf, int chanid, int slave) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "*", bdf); + snprintf(p, desc+sizeof(desc)-p, "/drive@%x/disk@%x", chanid, slave); + return find_prio(desc); }
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) { - return -1; + if (bdf == -1) + // support only pci machine for now + return -1; + // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "isa", bdf); + snprintf(p, desc+sizeof(desc)-p, "/fdc@%04x/floppy@%x", port, fdid); + return find_prio(desc); }
int bootprio_find_pci_rom(int bdf, int instance) { - return -1; + // Find pci rom - for example: /pci@i0cf8/scsi@3:rom2 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "*", bdf); + if (instance) + snprintf(p, desc+sizeof(desc)-p, ":rom%d", instance); + return find_prio(desc); }
int bootprio_find_named_rom(const char *name, int instance) { - return -1; + // Find named rom - for example: /rom@genroms/linuxboot.bin + char desc[256], *p; + p = desc + snprintf(desc, sizeof(desc), "/rom@%s", name); + if (instance) + snprintf(p, desc+sizeof(desc)-p, ":rom%d", instance); + return find_prio(desc); }
@@ -167,6 +247,8 @@ bootentry_add(int type, int prio, u32 data, const char *desc) be->priority = prio; be->data = data; be->description = desc ?: "?"; + dprintf(3, "Registering bootable: %s (type:%d prio:%d data:%x)\n" + , be->description, type, prio, data);
// Add entry in sorted order. struct bootentry_s **pprev;
On Thu, Dec 30, 2010 at 07:02:34PM -0500, Kevin O'Connor wrote:
On Thu, Dec 30, 2010 at 09:31:56PM +0200, Gleb Natapov wrote:
On Thu, Dec 30, 2010 at 01:56:28PM -0500, Kevin O'Connor wrote:
Assuming each bridge takes 15 bytes to describe, one would need around 12 layers of bridges before this was an issue. I don't think that's realistic.
Each node consist of name@address. IIRC name can be max 32 bytes. Address is unlimited but the longest address I can think of is 64bit integer written in hex, this is 16 bites more. On a PC we have very limited number of possible devices and none with such long names and deep nesting, so probably this is not the big issue.
Well, it was pointless for my patch to store the pci path on the stack twice, so I redid that. As you say, though, it's unlikely to be an issue - the size can always be increased if it does become an issue.
Works in my testing. Are you going to commit it?
-Kevin
diff --git a/src/boot.c b/src/boot.c index e83dcdc..dd719f3 100644 --- a/src/boot.c +++ b/src/boot.c @@ -13,6 +13,7 @@ #include "boot.h" // func defs #include "cmos.h" // inb_cmos #include "paravirt.h" // romfile_loadfile +#include "pci.h" //pci_bdf_to_*
/**************************************************************** @@ -29,7 +30,7 @@ loadBootOrder(void) if (!f) return;
- int i;
- int i = 0; BootorderCount = 1; while (f[i]) { if (f[i] == '\n')
@@ -48,38 +49,117 @@ loadBootOrder(void) do { Bootorder[i] = f; f = strchr(f, '\n');
if (f) {*f = '\0';f++;dprintf(3, "%d: %s\n", i, Bootorder[i]);i++;}
if (f)*(f++) = '\0';dprintf(3, "%d: %s\n", i+1, Bootorder[i]);i++;}
-int bootprio_find_pci_device(int bdf) +// See if 'str' matches 'glob' - if glob contains an '*' character it +// will match any number of characters in str that aren't a '/' or the +// next glob character. +static char * +glob_prefix(const char *glob, const char *str) +{
- for (;;) {
if (!*glob && (!*str || *str == '/'))return (char*)str;if (*glob == *str) {glob++;str++;continue;}if (*glob != '*')return NULL;if (*str && *str != '/' && *str != glob[1])str++;elseglob++;- }
+}
+// Search the bootorder list for the given glob pattern. +static int +find_prio(const char *glob) {
- dprintf(1, "Searching bootorder for: %s\n", glob);
- int i;
- for (i = 0; i < BootorderCount; i++)
if (glob_prefix(glob, Bootorder[i]))return i+1;}
+#define FW_PCI_DOMAIN "/pci@i0cf8"
+static char * +build_pci_path(char *buf, int max, const char *devname, int bdf) +{
- // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2
- char *p = buf;
- int bus = pci_bdf_to_bus(bdf);
- if (bus)
// XXX - this isn't the correct path syntaxp += snprintf(p, max, "/bus%x", bus);- int dev = pci_bdf_to_dev(bdf), fn = pci_bdf_to_fn(bdf);
- p += snprintf(p, buf+max-p, "%s/%s@%x", FW_PCI_DOMAIN, devname, dev);
- if (fn)
p += snprintf(p, buf+max-p, ",%x", fn);- return p;
+}
+int bootprio_find_pci_device(int bdf) +{
- // Find pci device - for example: /pci@i0cf8/ethernet@5
- char desc[256];
- build_pci_path(desc, sizeof(desc), "*", bdf);
- return find_prio(desc);
+}
int bootprio_find_ata_device(int bdf, int chanid, int slave) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find ata drive - for example: /pci@i0cf8/ide@1,1/drive@1/disk@0
- char desc[256], *p;
- p = build_pci_path(desc, sizeof(desc), "*", bdf);
- snprintf(p, desc+sizeof(desc)-p, "/drive@%x/disk@%x", chanid, slave);
- return find_prio(desc);
}
-int bootprio_find_fdc_device(int bfd, int port, int fdid) +int bootprio_find_fdc_device(int bdf, int port, int fdid) {
- return -1;
- if (bdf == -1)
// support only pci machine for nowreturn -1;- // Find floppy - for example: /pci@i0cf8/isa@1/fdc@03f1/floppy@0
- char desc[256], *p;
- p = build_pci_path(desc, sizeof(desc), "isa", bdf);
- snprintf(p, desc+sizeof(desc)-p, "/fdc@%04x/floppy@%x", port, fdid);
- return find_prio(desc);
}
int bootprio_find_pci_rom(int bdf, int instance) {
- return -1;
- // Find pci rom - for example: /pci@i0cf8/scsi@3:rom2
- char desc[256], *p;
- p = build_pci_path(desc, sizeof(desc), "*", bdf);
- if (instance)
snprintf(p, desc+sizeof(desc)-p, ":rom%d", instance);- return find_prio(desc);
}
int bootprio_find_named_rom(const char *name, int instance) {
- return -1;
- // Find named rom - for example: /rom@genroms/linuxboot.bin
- char desc[256], *p;
- p = desc + snprintf(desc, sizeof(desc), "/rom@%s", name);
- if (instance)
snprintf(p, desc+sizeof(desc)-p, ":rom%d", instance);- return find_prio(desc);
}
@@ -167,6 +247,8 @@ bootentry_add(int type, int prio, u32 data, const char *desc) be->priority = prio; be->data = data; be->description = desc ?: "?";
dprintf(3, "Registering bootable: %s (type:%d prio:%d data:%x)\n"
, be->description, type, prio, data);// Add entry in sorted order. struct bootentry_s **pprev;
-- Gleb.
On Wed, Jan 05, 2011 at 05:19:21PM +0200, Gleb Natapov wrote:
On Thu, Dec 30, 2010 at 07:02:34PM -0500, Kevin O'Connor wrote:
Well, it was pointless for my patch to store the pci path on the stack twice, so I redid that. As you say, though, it's unlikely to be an issue - the size can always be increased if it does become an issue.
Works in my testing. Are you going to commit it?
I pushed my latest tree.
-Kevin
-
Gleb Natapov -
Kevin O'Connor