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

[Sam Eiderman]

> On 17 Jun 2019, at 10:20, Gerd Hoffmann <address@hidden> 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
>