Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
> On 17 Jun 2019, at 17:48, Kevin O'Connor 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: We can go pure ascii. I meanwhile sent a v3 QEMU and v2 SeaBIOS patches for more comments. Sam > > -Kevin
Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
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: -Kevin
Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
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 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 >
Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
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
Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
> On 17 Jun 2019, at 10:20, Gerd Hoffmann 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(_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
>
Re: [Qemu-devel] [QEMU] [PATCH v2 7/8] bootdevice: FW_CFG interface for LCHS values
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(_lchs)) {
return NULL;
}
> +if (!mc->legacy_fw_cfg_order) {
^^^
Hmm?
cheers,
Gerd
