diff mbox series

[QEMU,v2,7/8] bootdevice: FW_CFG interface for LCHS values

Message ID 20190612115939.23825-8-shmuel.eiderman@oracle.com
State New
Headers show
Series Add Qemu to SeaBIOS LCHS interface | expand

Commit Message

Sam Eiderman June 12, 2019, 11:59 a.m. UTC 
Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS.

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 report - SeaBIOS, for large enough disks - will
use LBA translation, which will report 63 SPT instead.

In addition we cannot force SeaBIOS to rely on physical geometries at
all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot
report more than 16 physical heads when moved to an IDE controller,
since the ATA spec allows a maximum of 16 heads - this is an artifact of
virtualization.

By supplying the logical geometries directly we are able to support such
"exotic" disks.

We serialize this information in a similar way to the "bootorder"
interface.
The fw_cfg entry is "bootdevices" and it serializes a struct.
At the moment the struct holds the values of logical CHS values but it
can be expanded easily due to the extendable ABI implemented.

(In the future, we can pass the bootindex through "bootdevices" instead
"bootorder" - unifying all bootdevice information in one fw_cfg value)

The PV interface through fw_cfg could have also been implemented using
device specific keys, e.g.: "/etc/bootdevice/%s/logical_geometry" where
%s is the device name QEMU produces - but this implementation would
require much more code refactoring, both in QEMU and SeaBIOS, so the
current implementation was chosen.

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>
---
 bootdevice.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
 hw/nvram/fw_cfg.c       | 14 +++++++++++---
 include/sysemu/sysemu.h |  1 +
 3 files changed, 54 insertions(+), 3 deletions(-)

Comments

Gerd Hoffmann June 17, 2019, 7:20 a.m. UTC | #1 
Hi,

> We serialize this information in a similar way to the "bootorder"
> interface.
> The fw_cfg entry is "bootdevices" and it serializes a struct.

Why "bootdevices"?  I'd suggest to use "geometry" or "lchs" instead.

> At the moment the struct holds the values of logical CHS values but it
> can be expanded easily due to the extendable ABI implemented.
> 
> (In the future, we can pass the bootindex through "bootdevices" instead
> "bootorder" - unifying all bootdevice information in one fw_cfg value)

I don't think deprecating bootorder is useful.  Nobody cares about the
disk geometry, except some legacy x86 bios guests.  So seabios will be
the only firmware using this new interface.  Switching all firmware to a
new fw_cfg file is pointless churn.

Why make this extendable?  What possible extensions do you have in mind?

Also note that with a possible extension you might end up in a situation
where you have info A for device 1 and info B for device 2 and info A+B
for device 3 while with your current patch there is no way to signal
whenever info A or B is available for a given device.

> +/* Serialized as: struct size (4) + (device name\0 + device struct) x devices */
> +char *get_boot_devices_info(size_t *size)
> +{
> +    FWLCHSEntry *i;
> +    BootDeviceEntrySerialized s;
> +    size_t total = 0;
> +    char *list = NULL;

if (QTAILQ_EMPTY(&fw_lchs)) {
	return NULL;
}

> +    if (!mc->legacy_fw_cfg_order) {
                ^^^^^^^^^^^^^^^^^^^
Hmm?

cheers,
  Gerd
Sam Eiderman June 17, 2019, 7:36 a.m. UTC | #2 
> On 17 Jun 2019, at 10:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>> We serialize this information in a similar way to the "bootorder"
>> interface.
>> The fw_cfg entry is "bootdevices" and it serializes a struct.
> 
> Why "bootdevices"?  I'd suggest to use "geometry" or "lchs" instead.

True, if we don’t think an extension will be required in the future we might
as well call it “lchs" or "bios-geometry”.

> 
>> At the moment the struct holds the values of logical CHS values but it
>> can be expanded easily due to the extendable ABI implemented.
>> 
>> (In the future, we can pass the bootindex through "bootdevices" instead
>> "bootorder" - unifying all bootdevice information in one fw_cfg value)
> 
> I don't think deprecating bootorder is useful.  Nobody cares about the
> disk geometry, except some legacy x86 bios guests.  So seabios will be
> the only firmware using this new interface.  Switching all firmware to a
> new fw_cfg file is pointless churn.
> 
> Why make this extendable?  What possible extensions do you have in mind?

I’m not sure about this but if “bootorder” was written in the first place using
such an extension this could have been useful.
I don’t have anything specific in mind.
I don’t think deprecating bootorder is useful either, just mentioned that it
will be possible if we would like to unify all disk values someday.

> 
> Also note that with a possible extension you might end up in a situation
> where you have info A for device 1 and info B for device 2 and info A+B
> for device 3 while with your current patch there is no way to signal
> whenever info A or B is available for a given device.

Well for lchs (A) a non-existing value is 0, 0, 0 (uint32).
So at the moment we’re good.
We can signal other values with other magic numbers (such as -1 for bootorder)
or prefix the value with an additional boolean value “is signaled”.

> 
>> +/* Serialized as: struct size (4) + (device name\0 + device struct) x devices */
>> +char *get_boot_devices_info(size_t *size)
>> +{
>> +    FWLCHSEntry *i;
>> +    BootDeviceEntrySerialized s;
>> +    size_t total = 0;
>> +    char *list = NULL;
> 
> if (QTAILQ_EMPTY(&fw_lchs)) {
> 	return NULL;
> }
> 
>> +    if (!mc->legacy_fw_cfg_order) {
>                ^^^^^^^^^^^^^^^^^^^
> Hmm?

Only making this available in non-legacy mode.
Qemu complains in get_fw_cfg_order() (fw_cfg.c):

	warn_report("Unknown firmware file in legacy mode: %s", name);

Detected during qtests.


So overall, WDYT?
Keep it extendible for a low price of ABI + “bootdevices” name.
Or go strict and rename to “bios-geometries”?

(The ABI will not change too much anyway, the struct_size will disappear
and sizeof(12) struct of LCHS will be assumed)

Sam

> 
> cheers,
>  Gerd
>
Gerd Hoffmann June 17, 2019, 8:38 a.m. UTC | #3 
Hi,

> Keep it extendible for a low price of ABI + “bootdevices” name.
> Or go strict and rename to “bios-geometries”?

The name should reflect what is in there, so "bios-geometries" looks
better to me.  I'd also keep it strict, unless we have at least a vague
idea what might be a useful future extension.  I don't have any.

cheers,
  Gerd
Sam Eiderman June 17, 2019, 10:08 a.m. UTC | #4 
Ok,

I’ll resubmit this patch series in v3, as well as v2 for SeaBIOS soon enough.

* Change “bootdevices” to “bios-geometry”, and remove the struct size
* Add cpu_to_le32 fix as Laszlo suggested or big endian hosts
* Fix last qtest commit - automatic docker tester for some reason does not have qemu-img set

Sam

> On 17 Jun 2019, at 11:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>> Keep it extendible for a low price of ABI + “bootdevices” name.
>> Or go strict and rename to “bios-geometries”?
> 
> The name should reflect what is in there, so "bios-geometries" looks
> better to me.  I'd also keep it strict, unless we have at least a vague
> idea what might be a useful future extension.  I don't have any.
> 
> cheers,
>  Gerd
>
Kevin O'Connor June 17, 2019, 2:48 p.m. UTC | #5 
On Mon, Jun 17, 2019 at 10:36:54AM +0300, Sam Eiderman wrote:
> So overall, WDYT?
> Keep it extendible for a low price of ABI + “bootdevices” name.
> Or go strict and rename to “bios-geometries”?

If we add another qemu to firmware interface I think the interface
should be documented.  I also think that a mix of an ascii and binary
interface is going to be difficult to describe and document.  I'd
prefer a pure ascii interface - for example a newline separated list
of four space separted fields: <device name> <cylinders> <heads> <spt>

-Kevin
Sam Eiderman June 17, 2019, 3:13 p.m. UTC | #6 
> On 17 Jun 2019, at 17:48, Kevin O'Connor <kevin@koconnor.net> wrote:
> 
> On Mon, Jun 17, 2019 at 10:36:54AM +0300, Sam Eiderman wrote:
>> So overall, WDYT?
>> Keep it extendible for a low price of ABI + “bootdevices” name.
>> Or go strict and rename to “bios-geometries”?
> 
> If we add another qemu to firmware interface I think the interface
> should be documented.  I also think that a mix of an ascii and binary
> interface is going to be difficult to describe and document.  I'd
> prefer a pure ascii interface - for example a newline separated list
> of four space separted fields: <device name> <cylinders> <heads> <spt>

We can go pure ascii.
I meanwhile sent a v3 QEMU and v2 SeaBIOS patches for more comments.

Sam

> 
> -Kevin
diff mbox series

Patch

diff --git a/bootdevice.c b/bootdevice.c
index 2b12fb85a4..84c2a83f25 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -405,3 +405,45 @@  void del_boot_device_lchs(DeviceState *dev, const char *suffix)
         }
     }
 }
+
+typedef struct QEMU_PACKED BootDeviceEntrySerialized {
+    /* Do not change field order - add new fields below */
+    uint32_t lcyls;
+    uint32_t lheads;
+    uint32_t lsecs;
+} BootDeviceEntrySerialized;
+
+/* Serialized as: struct size (4) + (device name\0 + device struct) x devices */
+char *get_boot_devices_info(size_t *size)
+{
+    FWLCHSEntry *i;
+    BootDeviceEntrySerialized s;
+    size_t total = 0;
+    char *list = NULL;
+
+    list = g_malloc0(sizeof(uint32_t));
+    *((uint32_t *)list) = (uint32_t)sizeof(s);
+    total = sizeof(uint32_t);
+
+    QTAILQ_FOREACH(i, &fw_lchs, link) {
+        char *bootpath;
+        size_t len;
+
+        bootpath = get_boot_device_path(i->dev, false, i->suffix);
+        s.lcyls = i->lcyls;
+        s.lheads = i->lheads;
+        s.lsecs = i->lsecs;
+
+        len = strlen(bootpath) + 1;
+        list = g_realloc(list, total + len + sizeof(s));
+        memcpy(&list[total], bootpath, len);
+        memcpy(&list[total + len], &s, sizeof(s));
+        total += len + sizeof(s);
+
+        g_free(bootpath);
+    }
+
+    *size = total;
+
+    return list;
+}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 9f7b7789bc..008b21542f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -916,13 +916,21 @@  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
 
 static void fw_cfg_machine_reset(void *opaque)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    FWCfgState *s = opaque;
     void *ptr;
     size_t len;
-    FWCfgState *s = opaque;
-    char *bootindex = get_boot_devices_list(&len);
+    char *buf;
 
-    ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
+    buf = get_boot_devices_list(&len);
+    ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)buf, len);
     g_free(ptr);
+
+    if (!mc->legacy_fw_cfg_order) {
+        buf = get_boot_devices_info(&len);
+        ptr = fw_cfg_modify_file(s, "bootdevices", (uint8_t *)buf, len);
+        g_free(ptr);
+    }
 }
 
 static void fw_cfg_machine_ready(struct Notifier *n, void *data)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 173dfbb539..f0552006f4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -174,6 +174,7 @@  void validate_bootdevices(const char *devices, Error **errp);
 void add_boot_device_lchs(DeviceState *dev, const char *suffix,
                           uint32_t lcyls, uint32_t lheads, uint32_t lsecs);
 void del_boot_device_lchs(DeviceState *dev, const char *suffix);
+char *get_boot_devices_info(size_t *size);
 
 /* handler to set the boot_device order for a specific type of MachineClass */
 typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,