Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values

[Gerd Hoffmann]

  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