[Qemu-devel] [SeaBIOS] [PATCH v3 0/4] Add Qemu to SeaBIOS LCHS interface

[Qemu-devel] [SeaBIOS] [PATCH v3 0/4] Add Qemu to SeaBIOS LCHS interface

[Sam Eiderman]

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(-)

-- 
2.13.3

[Qemu-devel] [SeaBIOS] [PATCH v3 1/4] geometry: Read LCHS from fw_cfg

[Sam Eiderman]

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();
 }
 
 
-- 
2.13.3

[Qemu-devel] [SeaBIOS] [PATCH v3 2/4] boot: Reorder functions in boot.c

[Sam Eiderman]

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)
-- 
2.13.3

[Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Sam Eiderman]

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);
-- 
2.13.3

[Qemu-devel] [SeaBIOS] [PATCH v3 4/4] geometry: Apply LCHS values for boot devices

[Sam Eiderman]

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)
-- 
2.13.3

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 4/4] geometry: Apply LCHS values for boot devices

[Gerd Hoffmann]

> +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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 4/4] geometry: Apply LCHS values for boot devices

[Sam Eiderman]

> 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
> 

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 4/4] geometry: Apply LCHS values for boot devices

[Gerd Hoffmann]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Kevin O'Connor]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Sam Eiderman]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Kevin O'Connor]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Sam Eiderman]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Kevin O'Connor]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Sam Eiderman]

> 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

Re: [Qemu-devel] [SeaBIOS] [PATCH v3 3/4] geometry: Add boot_lchs_find_*() utility functions

[Sam Eiderman]

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
>