v1:
Non-standard logical geometries break under QEMU.
A virtual disk which contains an operating system which depends on logical geometries (consistent values being reported from BIOS INT13 AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard logical geometries - for example 56 SPT (sectors per track). No matter what QEMU will guess - SeaBIOS, for large enough disks - will use LBA translation, which will report 63 SPT instead.
In addition we can not enforce SeaBIOS to rely on phyiscal geometries at all. A virtio-blk-pci virtual disk with 255 phyiscal heads can not report more than 16 physical heads when moved to an IDE controller, the ATA spec allows a maximum of 16 heads - this is an artifact of virtualization.
By supplying the logical geometies directly we are able to support such "exotic" disks.
We will use fw_cfg to do just that.
v2:
Rename bootdevices fw_cfg key to bios-geoemtry
v3:
Change fw_cfg interface from mixed binary/textual to textual only Squash commit "config: Add toggle for bootdevice information"
Sam Eiderman (4): geometry: Read LCHS from fw_cfg boot: Reorder functions in boot.c geometry: Add boot_lchs_find_*() utility functions geometry: Apply LCHS values for boot devices
src/Kconfig | 7 ++ src/block.c | 21 ++++- src/block.h | 1 + src/boot.c | 239 +++++++++++++++++++++++++++++++++++++++++---------- src/hw/ahci.c | 1 + src/hw/ata.c | 8 ++ src/hw/esp-scsi.c | 2 + src/hw/lsi-scsi.c | 2 + src/hw/megasas.c | 1 + src/hw/mpt-scsi.c | 2 + src/hw/pvscsi.c | 1 + src/hw/virtio-blk.c | 2 + src/hw/virtio-scsi.c | 2 + src/util.h | 6 ++ 14 files changed, 249 insertions(+), 46 deletions(-)
Read bios geometry for boot devices from fw_cfg.
By receiving LCHS values directly from QEMU through fw_cfg we will be able to support logical geometries which can not be inferred by SeaBIOS itself. (For instance: A 8GB virtio-blk hard drive which was originally created as an IDE and must report LCHS of */32/63 for its operating system to function will always break under SeaBIOS since a LARGE/LBA translation will be used, causing the number of reported logical heads to be > 32.)
The only LCHS paravirtual interface available at the moment is for IDE disks (rtc_read() in get_translation()) and it's limited to a maximum of 4 disks (this code existed in SeaBIOS's translation function before SCSI and VirtIO were even introduced). This is why we create a new interface which allows passing LCHS information per hdd.
Boot device information is serialized in the following way: * device_path lcyls lheads lsecs\n ... * device_path lcyls lheads lsecs\0
Device path is a null terminated string in the "Open Firmware" device path format, the same path as used in bootorder.
Reviewed-by: Karl Heubaum karl.heubaum@oracle.com Reviewed-by: Arbel Moshe arbel.moshe@oracle.com Signed-off-by: Sam Eiderman shmuel.eiderman@oracle.com --- src/boot.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
diff --git a/src/boot.c b/src/boot.c index 5acf94fe..a2cb167c 100644 --- a/src/boot.c +++ b/src/boot.c @@ -24,6 +24,79 @@
/**************************************************************** + * Boot device logical geometry + ****************************************************************/ + +typedef struct BootDeviceLCHS { + char *name; + u32 lcyls; + u32 lheads; + u32 lsecs; +} BootDeviceLCHS; + +static BootDeviceLCHS *BiosGeometry VARVERIFY32INIT; +static int BiosGeometryCount; + +static char * +parse_u32(char *cur, u32 *n) +{ + u32 m = 0; + if (cur) { + while ('0' <= *cur && *cur <= '9') { + m = 10 * m + (*cur - '0'); + cur++; + } + if (*cur != '\0') + cur++; + } + *n = m; + return cur; +} + +static void +loadBiosGeometry(void) +{ + char *f = romfile_loadfile("bios-geometry", NULL); + if (!f) + return; + + int i = 0; + BiosGeometryCount = 1; + while (f[i]) { + if (f[i] == '\n') + BiosGeometryCount++; + i++; + } + BiosGeometry = malloc_tmphigh(BiosGeometryCount * sizeof(BootDeviceLCHS)); + if (!BiosGeometry) { + warn_noalloc(); + free(f); + BiosGeometryCount = 0; + return; + } + + dprintf(1, "bios geometry:\n"); + i = 0; + do { + BootDeviceLCHS *d = &BiosGeometry[i]; + d->name = f; + f = strchr(f, '\n'); + if (f) + *(f++) = '\0'; + char *chs_values = strchr(d->name, ' '); + if (chs_values) + *(chs_values++) = '\0'; + chs_values = parse_u32(chs_values, &d->lcyls); + chs_values = parse_u32(chs_values, &d->lheads); + chs_values = parse_u32(chs_values, &d->lsecs); + dprintf(1, "%s: (%u, %u, %u)\n", + d->name, d->lcyls, d->lheads, d->lsecs); + i++; + } while (f); +} + + +/**************************************************************** * Boot priority ordering ****************************************************************/
@@ -288,6 +361,7 @@ boot_init(void) BootRetryTime = romfile_loadint("etc/boot-fail-wait", 60*1000);
loadBootOrder(); + loadBiosGeometry(); }
Currently glob_prefix() and build_pci_path() are under the "Boot priority ordering" section. Move them to a new "Helper search functions" section since we will reuse them in the next commit.
Reviewed-by: Karl Heubaum karl.heubaum@oracle.com Reviewed-by: Arbel Moshe arbel.moshe@oracle.com Signed-off-by: Sam Eiderman shmuel.eiderman@oracle.com --- src/boot.c | 94 ++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/boot.c b/src/boot.c index a2cb167c..70f639f4 100644 --- a/src/boot.c +++ b/src/boot.c @@ -22,6 +22,55 @@ #include "util.h" // irqtimer_calc #include "tcgbios.h" // tpm_*
+/**************************************************************** + * Helper search functions + ****************************************************************/ + +// See if 'str' starts with '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 == '*') { + if (!*str || *str == '/' || *str == glob[1]) + glob++; + else + str++; + continue; + } + if (*glob != *str) + return NULL; + glob++; + str++; + } +} + +#define FW_PCI_DOMAIN "/pci@i0cf8" + +static char * +build_pci_path(char *buf, int max, const char *devname, struct pci_device *pci) +{ + // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2 + char *p = buf; + if (pci->parent) { + p = build_pci_path(p, max, "pci-bridge", pci->parent); + } else { + p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN); + if (pci->rootbus) + p += snprintf(p, buf+max-p, ",%x", pci->rootbus); + } + + int dev = pci_bdf_to_dev(pci->bdf), fn = pci_bdf_to_fn(pci->bdf); + p += snprintf(p, buf+max-p, "/%s@%x", devname, dev); + if (fn) + p += snprintf(p, buf+max-p, ",%x", fn); + return p; +} +
/**************************************************************** * Boot device logical geometry @@ -141,29 +190,6 @@ loadBootOrder(void) } while (f); }
-// See if 'str' starts with '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 == '*') { - if (!*str || *str == '/' || *str == glob[1]) - glob++; - else - str++; - continue; - } - if (*glob != *str) - return NULL; - glob++; - str++; - } -} - // Search the bootorder list for the given glob pattern. static int find_prio(const char *glob) @@ -176,28 +202,6 @@ find_prio(const char *glob) return -1; }
-#define FW_PCI_DOMAIN "/pci@i0cf8" - -static char * -build_pci_path(char *buf, int max, const char *devname, struct pci_device *pci) -{ - // Build the string path of a bdf - for example: /pci@i0cf8/isa@1,2 - char *p = buf; - if (pci->parent) { - p = build_pci_path(p, max, "pci-bridge", pci->parent); - } else { - p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN); - if (pci->rootbus) - p += snprintf(p, buf+max-p, ",%x", pci->rootbus); - } - - int dev = pci_bdf_to_dev(pci->bdf), fn = pci_bdf_to_fn(pci->bdf); - p += snprintf(p, buf+max-p, "/%s@%x", devname, dev); - if (fn) - p += snprintf(p, buf+max-p, ",%x", fn); - return p; -} - int bootprio_find_pci_device(struct pci_device *pci) { if (CONFIG_CSM)
Adding the following utility functions:
* boot_lchs_find_pci_device * boot_lchs_find_scsi_device * boot_lchs_find_ata_device
These will be used to apply LCHS values received through fw_cfg.
Reviewed-by: Karl Heubaum karl.heubaum@oracle.com Reviewed-by: Arbel Moshe arbel.moshe@oracle.com Signed-off-by: Sam Eiderman shmuel.eiderman@oracle.com --- src/Kconfig | 7 ++++++ src/boot.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util.h | 6 ++++++ 3 files changed, 84 insertions(+)
diff --git a/src/Kconfig b/src/Kconfig index 55a87cb7..0b4c1c0d 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -72,6 +72,13 @@ endchoice help Support controlling of the boot order via the fw_cfg/CBFS "bootorder" file. + config BIOS_GEOMETRY + depends on BOOT + bool "Boot device bios geometry override" + default y + help + Support overriding bios (logical) geometry of boot devices via the + fw_cfg/CBFS "bios-geometry" file.
config COREBOOT_FLASH depends on COREBOOT diff --git a/src/boot.c b/src/boot.c index 70f639f4..308d9559 100644 --- a/src/boot.c +++ b/src/boot.c @@ -105,6 +105,8 @@ parse_u32(char *cur, u32 *n) static void loadBiosGeometry(void) { + if (!CONFIG_BIOS_GEOMETRY) + return; char *f = romfile_loadfile("bios-geometry", NULL); if (!f) return; @@ -144,6 +146,75 @@ loadBiosGeometry(void) } while (f); }
+// Search the bios-geometry list for the given glob pattern. +static BootDeviceLCHS * +boot_lchs_find(const char *glob) +{ + dprintf(1, "Searching bios-geometry for: %s\n", glob); + int i; + for (i = 0; i < BiosGeometryCount; i++) + if (glob_prefix(glob, BiosGeometry[i].name)) + return &BiosGeometry[i]; + return NULL; +} + +int boot_lchs_find_pci_device(struct pci_device *pci, struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + char desc[256]; + build_pci_path(desc, sizeof(desc), "*", pci); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} + +int boot_lchs_find_scsi_device(struct pci_device *pci, int target, int lun, + struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + if (!pci) + // support only pci machine for now + return -1; + // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 + char desc[256], *p; + p = build_pci_path(desc, sizeof(desc), "*", pci); + snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%x,%x", target, lun); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} + +int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave, + struct chs_s *chs) +{ + if (!CONFIG_BIOS_GEOMETRY) + return -1; + if (!pci) + // 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), "*", pci); + snprintf(p, desc+sizeof(desc)-p, "/drive@%x/disk@%x", chanid, slave); + BootDeviceLCHS *b = boot_lchs_find(desc); + if (!b) + return -1; + chs->cylinder = (u16)b->lcyls; + chs->head = (u16)b->lheads; + chs->sector = (u16)b->lsecs; + return 0; +} +
/**************************************************************** * Boot priority ordering diff --git a/src/util.h b/src/util.h index e2afc80c..b173fa88 100644 --- a/src/util.h +++ b/src/util.h @@ -38,6 +38,12 @@ struct usbdevice_s; int bootprio_find_usb(struct usbdevice_s *usbdev, int lun); int get_keystroke_full(int msec); int get_keystroke(int msec); +struct chs_s; +int boot_lchs_find_pci_device(struct pci_device *pci, struct chs_s *chs); +int boot_lchs_find_scsi_device(struct pci_device *pci, int target, int lun, + struct chs_s *chs); +int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave, + struct chs_s *chs);
// bootsplash.c void enable_vga_console(void);
Boot devices which use overriden LCHS values are:
* ata * ahci * scsi * esp * lsi * megasas * mpt * pvscsi * virtio * virtio-blk
We use these values in get_translation() and setup_translation() by introducing a new translation type: "TRANSLATION_MACHINE".
We treat this translation as TRANSLATION_NONE in fill_ata_edd(), although this does not really matter since now the translation between physical and logical geometry does not exist.
Reviewed-by: Karl Heubaum karl.heubaum@oracle.com Reviewed-by: Arbel Moshe arbel.moshe@oracle.com Signed-off-by: Sam Eiderman shmuel.eiderman@oracle.com --- src/block.c | 21 ++++++++++++++++++++- src/block.h | 1 + src/hw/ahci.c | 1 + src/hw/ata.c | 8 ++++++++ src/hw/esp-scsi.c | 2 ++ src/hw/lsi-scsi.c | 2 ++ src/hw/megasas.c | 1 + src/hw/mpt-scsi.c | 2 ++ src/hw/pvscsi.c | 1 + src/hw/virtio-blk.c | 2 ++ src/hw/virtio-scsi.c | 2 ++ 11 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/src/block.c b/src/block.c index f73ec18c..ca23a83a 100644 --- a/src/block.c +++ b/src/block.c @@ -69,9 +69,17 @@ int create_bounce_buf(void) * Disk geometry translation ****************************************************************/
+static int +overriden_lchs_supplied(struct drive_s *drive) +{ + return drive->lchs.cylinder || drive->lchs.head || drive->lchs.sector; +} + static u8 get_translation(struct drive_s *drive) { + if (overriden_lchs_supplied(drive)) + return TRANSLATION_MACHINE; u8 type = drive->type; if (CONFIG_QEMU && type == DTYPE_ATA) { // Emulators pass in the translation info via nvram. @@ -159,6 +167,16 @@ setup_translation(struct drive_s *drive) break; } break; + case TRANSLATION_MACHINE: + desc = "overriden"; + cylinders = drive->lchs.cylinder; + heads = drive->lchs.head; + if (heads > 255) + heads = 255; + spt = drive->lchs.sector; + if (spt > 63) + spt = 63; + break; } // clip to 1024 cylinders in lchs if (cylinders > 1024) @@ -423,7 +441,8 @@ fill_ata_edd(struct segoff_s edd, struct drive_s *drive_gf) u16 options = 0; if (GET_GLOBALFLAT(drive_gf->type) == DTYPE_ATA) { u8 translation = GET_GLOBALFLAT(drive_gf->translation); - if (translation != TRANSLATION_NONE) { + if ((translation != TRANSLATION_NONE) && + (translation != TRANSLATION_MACHINE)) { options |= 1<<3; // CHS translation if (translation == TRANSLATION_LBA) options |= 1<<9; diff --git a/src/block.h b/src/block.h index f64e8807..12f27eee 100644 --- a/src/block.h +++ b/src/block.h @@ -90,6 +90,7 @@ struct drive_s { #define TRANSLATION_LBA 1 #define TRANSLATION_LARGE 2 #define TRANSLATION_RECHS 3 +#define TRANSLATION_MACHINE 4
#define EXTTYPE_FLOPPY 0 #define EXTTYPE_HD 1 diff --git a/src/hw/ahci.c b/src/hw/ahci.c index 1746e7a1..97a072a1 100644 --- a/src/hw/ahci.c +++ b/src/hw/ahci.c @@ -593,6 +593,7 @@ static int ahci_port_setup(struct ahci_port_s *port) , ata_extract_version(buffer)); port->prio = bootprio_find_ata_device(ctrl->pci_tmp, pnr, 0); } + boot_lchs_find_ata_device(ctrl->pci_tmp, pnr, 0, &(port->drive.lchs)); return 0; }
diff --git a/src/hw/ata.c b/src/hw/ata.c index b6e073cf..f788ce71 100644 --- a/src/hw/ata.c +++ b/src/hw/ata.c @@ -755,6 +755,10 @@ init_drive_atapi(struct atadrive_s *dummy, u16 *buffer) int prio = bootprio_find_ata_device(adrive->chan_gf->pci_tmp, adrive->chan_gf->chanid, adrive->slave); + boot_lchs_find_ata_device(adrive->chan_gf->pci_tmp, + adrive->chan_gf->chanid, + adrive->slave, + &(adrive->drive.lchs)); boot_add_cd(&adrive->drive, desc, prio); }
@@ -805,6 +809,10 @@ init_drive_ata(struct atadrive_s *dummy, u16 *buffer) int prio = bootprio_find_ata_device(adrive->chan_gf->pci_tmp, adrive->chan_gf->chanid, adrive->slave); + boot_lchs_find_ata_device(adrive->chan_gf->pci_tmp, + adrive->chan_gf->chanid, + adrive->slave, + &(adrive->drive.lchs)); // Register with bcv system. boot_add_hd(&adrive->drive, desc, prio);
diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c index ffd86d0f..cc25f227 100644 --- a/src/hw/esp-scsi.c +++ b/src/hw/esp-scsi.c @@ -181,6 +181,8 @@ esp_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv)
char *name = znprintf(MAXDESCSIZE, "esp %pP %d:%d", llun->pci, llun->target, llun->lun); + boot_lchs_find_scsi_device(llun->pci, llun->target, llun->lun, + &(llun->drive.lchs)); int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); int ret = scsi_drive_setup(&llun->drive, name, prio); free(name); diff --git a/src/hw/lsi-scsi.c b/src/hw/lsi-scsi.c index d5fc3e45..cbaa2acd 100644 --- a/src/hw/lsi-scsi.c +++ b/src/hw/lsi-scsi.c @@ -158,6 +158,8 @@ lsi_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) lsi_scsi_init_lun(llun, tmpl_llun->pci, tmpl_llun->iobase, tmpl_llun->target, lun);
+ boot_lchs_find_scsi_device(llun->pci, llun->target, llun->lun, + &(llun->drive.lchs)); char *name = znprintf(MAXDESCSIZE, "lsi %pP %d:%d", llun->pci, llun->target, llun->lun); int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); diff --git a/src/hw/megasas.c b/src/hw/megasas.c index d2675804..87b8beec 100644 --- a/src/hw/megasas.c +++ b/src/hw/megasas.c @@ -225,6 +225,7 @@ megasas_add_lun(struct pci_device *pci, u32 iobase, u8 target, u8 lun) free(mlun); return -1; } + boot_lchs_find_scsi_device(pci, target, lun, &(mlun->drive.lchs)); name = znprintf(MAXDESCSIZE, "MegaRAID SAS (PCI %pP) LD %d:%d" , pci, target, lun); prio = bootprio_find_scsi_device(pci, target, lun); diff --git a/src/hw/mpt-scsi.c b/src/hw/mpt-scsi.c index 1faede6a..570b2126 100644 --- a/src/hw/mpt-scsi.c +++ b/src/hw/mpt-scsi.c @@ -221,6 +221,8 @@ mpt_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) mpt_scsi_init_lun(llun, tmpl_llun->pci, tmpl_llun->iobase, tmpl_llun->target, lun);
+ boot_lchs_find_scsi_device(llun->pci, llun->target, llun->lun, + &(llun->drive.lchs)); char *name = znprintf(MAXDESCSIZE, "mpt %pP %d:%d", llun->pci, llun->target, llun->lun); int prio = bootprio_find_scsi_device(llun->pci, llun->target, llun->lun); diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c index 9d7d68d8..3e5171ad 100644 --- a/src/hw/pvscsi.c +++ b/src/hw/pvscsi.c @@ -273,6 +273,7 @@ pvscsi_add_lun(struct pci_device *pci, void *iobase, plun->iobase = iobase; plun->ring_dsc = ring_dsc;
+ boot_lchs_find_scsi_device(pci, target, lun, &(plun->drive.lchs)); char *name = znprintf(MAXDESCSIZE, "pvscsi %pP %d:%d", pci, target, lun); int prio = bootprio_find_scsi_device(pci, target, lun); int ret = scsi_drive_setup(&plun->drive, name, prio); diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 88d7e54a..3e615b26 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -183,6 +183,8 @@ init_virtio_blk(void *data)
status |= VIRTIO_CONFIG_S_DRIVER_OK; vp_set_status(&vdrive->vp, status); + + boot_lchs_find_pci_device(pci, &vdrive->drive.lchs); return;
fail: diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c index a87cad88..e1e2f5d4 100644 --- a/src/hw/virtio-scsi.c +++ b/src/hw/virtio-scsi.c @@ -121,6 +121,8 @@ virtio_scsi_add_lun(u32 lun, struct drive_s *tmpl_drv) virtio_scsi_init_lun(vlun, tmpl_vlun->pci, tmpl_vlun->vp, tmpl_vlun->vq, tmpl_vlun->target, lun);
+ boot_lchs_find_scsi_device(vlun->pci, vlun->target, vlun->lun, + &(vlun->drive.lchs)); int prio = bootprio_find_scsi_device(vlun->pci, vlun->target, vlun->lun); int ret = scsi_drive_setup(&vlun->drive, "virtio-scsi", prio); if (ret)
+static int +overriden_lchs_supplied(struct drive_s *drive) +{
- return drive->lchs.cylinder || drive->lchs.head || drive->lchs.sector;
+}
- case TRANSLATION_MACHINE:
Hmm, why this name? Doesn't look intuitive to me.
desc = "overriden";
I'd name that "host-supplied" or "fw-cfg".
cylinders = drive->lchs.cylinder;heads = drive->lchs.head;if (heads > 255)heads = 255;
I suggest to move these sanity checks to overriden_lchs_supplied(), then ignore the override altogether when heads or sectors is out of range instead of trying to fixup things.
The other patches look all sane to me.
cheers, Gerd
On 20 Jun 2019, at 8:42, Gerd Hoffmann kraxel@redhat.com wrote:
+static int +overriden_lchs_supplied(struct drive_s *drive) +{
- return drive->lchs.cylinder || drive->lchs.head || drive->lchs.sector;
+}
- case TRANSLATION_MACHINE:
Hmm, why this name? Doesn't look intuitive to me.
TRANSLATION_HOST?
desc = "overriden";I'd name that "host-supplied" or "fw-cfg”.
“host-supplied”?
cylinders = drive->lchs.cylinder;heads = drive->lchs.head;if (heads > 255)heads = 255;I suggest to move these sanity checks to overriden_lchs_supplied(), then ignore the override altogether when heads or sectors is out of range instead of trying to fixup things.
Sounds reasonable. I’ll rename to host_lchs_supplied()?
WDYT?
The other patches look all sane to me.
cheers, Gerd
On Thu, Jun 20, 2019 at 11:52:01AM +0300, Sam Eiderman wrote:
On 20 Jun 2019, at 8:42, Gerd Hoffmann kraxel@redhat.com wrote:
+static int +overriden_lchs_supplied(struct drive_s *drive) +{
- return drive->lchs.cylinder || drive->lchs.head || drive->lchs.sector;
+}
- case TRANSLATION_MACHINE:
Hmm, why this name? Doesn't look intuitive to me.
TRANSLATION_HOST?
desc = "overriden";I'd name that "host-supplied" or "fw-cfg”.
“host-supplied”?
cylinders = drive->lchs.cylinder;heads = drive->lchs.head;if (heads > 255)heads = 255;I suggest to move these sanity checks to overriden_lchs_supplied(), then ignore the override altogether when heads or sectors is out of range instead of trying to fixup things.
Sounds reasonable. I’ll rename to host_lchs_supplied()?
WDYT?
looks all good to me.
cheers, Gerd
On Wed, Jun 19, 2019 at 12:23:51PM +0300, Sam Eiderman wrote:
Adding the following utility functions:
* boot_lchs_find_pci_device * boot_lchs_find_scsi_device * boot_lchs_find_ata_device
FWIW, this leads to a bit of code duplication. I think it would be preferable to refactor the bootprio_find_XYZ() calls. Instead of returning an 'int prio' they could return a znprintf'd 'char *devpath' instead. Then the boot_add_XYZ() calls could directly call find_prio(devpath). The boot_add_hd() could then directly populate drive->lchs or call setup_translation().
-Kevin
Sounds reasonable, how do purpose to deal with:
config BIOS_GEOMETRY config BOOTORDER
precompiler optouts?
If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”.
I’ll see what can be done.
On 20 Jun 2019, at 17:37, Kevin O'Connor kevin@koconnor.net wrote:
On Wed, Jun 19, 2019 at 12:23:51PM +0300, Sam Eiderman wrote:
Adding the following utility functions:
- boot_lchs_find_pci_device
- boot_lchs_find_scsi_device
- boot_lchs_find_ata_device
FWIW, this leads to a bit of code duplication. I think it would be preferable to refactor the bootprio_find_XYZ() calls. Instead of returning an 'int prio' they could return a znprintf'd 'char *devpath' instead. Then the boot_add_XYZ() calls could directly call find_prio(devpath). The boot_add_hd() could then directly populate drive->lchs or call setup_translation().
-Kevin
On Fri, Jun 21, 2019 at 08:42:28PM +0300, Sam Eiderman wrote:
Sounds reasonable, how do purpose to deal with:
config BIOS_GEOMETRY config BOOTORDER
precompiler optouts?
I think you can stick them both under BOOTORDER. That option is only there in case someone wants to reduce the size of the SeaBIOS binary. I can't think of a reasonable situation where one cares that much about binary size, yet wants to override legacy disk translations..
If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”.
I’ll see what can be done.
Thanks. -Kevin
But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
I’m thinking of maybe adding
if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) return NULL;
In each of the get_*_devpath functions (which will normally return an allocated string, not on stack).
Another approach can be make CONFIG_BIOS_GEOMETRY depend on CONFIG_BOOTORDER. Then we should only keep:
if (!CONFIG_BOOTORDER) return NULL;
In the get_*_devpath functions.
I think the first approach will look better when reading the code - will not require the reader to analize dependancies in the Kconfig file.
Sam
On 21 Jun 2019, at 21:59, Kevin O'Connor kevin@koconnor.net wrote:
On Fri, Jun 21, 2019 at 08:42:28PM +0300, Sam Eiderman wrote:
Sounds reasonable, how do purpose to deal with:
config BIOS_GEOMETRY config BOOTORDER
precompiler optouts?
I think you can stick them both under BOOTORDER. That option is only there in case someone wants to reduce the size of the SeaBIOS binary. I can't think of a reasonable situation where one cares that much about binary size, yet wants to override legacy disk translations..
If we don’t need any of them we also don’t need to call “get_scsi_devpath", “get_ata_devpath”, “get_pci_dev_path”.
I’ll see what can be done.
Thanks. -Kevin
On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote:
But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
I’m thinking of maybe adding
if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) return NULL;
That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY).
FYI, I think BIOS_GEOMETRY is a little confusing - maybe CUSTOM_DISK_GEOMETRY.
-Kevin
On 22 Jun 2019, at 18:27, Kevin O'Connor kevin@koconnor.net wrote:
On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote:
But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
I’m thinking of maybe adding
if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) return NULL;
That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY).
Yes of course, my bad
FYI, I think BIOS_GEOMETRY is a little confusing - maybe CUSTOM_DISK_GEOMETRY.
The thing is that disk geometry is actually (physical geometry, reported by the disk controller) and here bios geometry stands for the geometry reported from bios int13. Also “bios geometry” === “logical geometry” === “lchs”. So following previous discussion with Gerd, maybe CONFIG_HOST_BIOS_GEOMETRY is better?
Sam
-Kevin
Kevin,
Rethinking this change (where we construct the device path from outside and call boot_prio_find()), this is pretty tricky to implement since we need to take care of csm_bootprio_ata() and csm_bootprio_pci() which do not work with device path. In addition, bootprio_find_fdc_device bootprio_find_pci_rom bootprio_find_named_rom bootprio_find_usb Will not require this change and this will just result in too much refactoring.
Maybe simply introduce build_scsi_path() and build_ata_path() functions and then, for instance, make booprio_find_scsi_device() and boot_lchs_find_scsi_device() call the same build_scsi_path() function, resulting in less code duplication.
Sam
On 22 Jun 2019, at 20:33, Sam Eiderman shmuel.eiderman@oracle.com wrote:
On 22 Jun 2019, at 18:27, Kevin O'Connor kevin@koconnor.net wrote:
On Sat, Jun 22, 2019 at 11:51:48AM +0300, Sam Eiderman wrote:
But maybe someone wants bootorder but doesn’t want to override legacy disk translations…
I’m thinking of maybe adding
if (!CONFIG_BOOTORDER || !CONFIG_BIOS_GEOMETRY) return NULL;
That's fine - though it's (!CONFIG_BOOTORDER && !CONFIG_BIOS_GEOMETRY).
Yes of course, my bad
FYI, I think BIOS_GEOMETRY is a little confusing - maybe CUSTOM_DISK_GEOMETRY.
The thing is that disk geometry is actually (physical geometry, reported by the disk controller) and here bios geometry stands for the geometry reported from bios int13. Also “bios geometry” === “logical geometry” === “lchs”. So following previous discussion with Gerd, maybe CONFIG_HOST_BIOS_GEOMETRY is better?
Sam
-Kevin
-
Gerd Hoffmann -
Kevin O'Connor -
Sam Eiderman