From: Sam Eiderman <shmuel.eiderman@oracle.com>
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 new fw_cfg entry is "bios-geometry".
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 | 32 ++++++++++++++++++++++++++++++++
hw/nvram/fw_cfg.c | 14 +++++++++++---
include/sysemu/sysemu.h | 1 +
3 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/bootdevice.c b/bootdevice.c
index 2b12fb85a4..b034ad7bdc 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix)
}
}
}
+
+/* Serialized as: (device name\0 + lchs struct) x devices */
+char *get_boot_devices_lchs_list(size_t *size)
+{
+ FWLCHSEntry *i;
+ size_t total = 0;
+ char *list = NULL;
+
+ QTAILQ_FOREACH(i, &fw_lchs, link) {
+ char *bootpath;
+ char *chs_string;
+ size_t len;
+
+ bootpath = get_boot_device_path(i->dev, false, i->suffix);
+ chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
+ bootpath, i->lcyls, i->lheads, i->lsecs);
+
+ if (total) {
+ list[total - 1] = '\n';
+ }
+ len = strlen(chs_string) + 1;
+ list = g_realloc(list, total + len);
+ memcpy(&list[total], chs_string, len);
+ total += len;
+ g_free(chs_string);
+ g_free(bootpath);
+ }
+
+ *size = total;
+
+ return list;
+}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7dc3ac378e..18aff658c0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -920,13 +920,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_lchs_list(&len);
+ ptr = fw_cfg_modify_file(s, "bios-geometry", (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 5bc5c79cbc..80c57fdc4e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -106,6 +106,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_lchs_list(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,
--
2.23.0.351.gc4317032e6-goog
Hi Sam,
On 9/25/19 1:06 PM, Sam Eiderman wrote:
> From: Sam Eiderman <shmuel.eiderman@oracle.com>
>
> 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 new fw_cfg entry is "bios-geometry".
>
> 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 | 32 ++++++++++++++++++++++++++++++++
> hw/nvram/fw_cfg.c | 14 +++++++++++---
> include/sysemu/sysemu.h | 1 +
> 3 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 2b12fb85a4..b034ad7bdc 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix)
> }
> }
> }
> +
> +/* Serialized as: (device name\0 + lchs struct) x devices */
> +char *get_boot_devices_lchs_list(size_t *size)
> +{
> + FWLCHSEntry *i;
> + size_t total = 0;
> + char *list = NULL;
> +
> + QTAILQ_FOREACH(i, &fw_lchs, link) {
> + char *bootpath;
> + char *chs_string;
> + size_t len;
> +
> + bootpath = get_boot_device_path(i->dev, false, i->suffix);
> + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
> + bootpath, i->lcyls, i->lheads, i->lsecs);
Hmm maybe we can g_free(bootpath) directly here.
> +
> + if (total) {
> + list[total - 1] = '\n';
> + }
> + len = strlen(chs_string) + 1;
> + list = g_realloc(list, total + len);
> + memcpy(&list[total], chs_string, len);
> + total += len;
> + g_free(chs_string);
> + g_free(bootpath);
> + }
> +
> + *size = total;
> +
> + return list;
> +}
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378e..18aff658c0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -920,13 +920,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_lchs_list(&len);
> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);
OK. Can you add a test in tests/fw_cfg-test.c please?
> + 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 5bc5c79cbc..80c57fdc4e 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -106,6 +106,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_lchs_list(size_t *size);
Please add some documentation. At least 'size' must be non-NULL.
Ideally you should add doc for the other functions added in 3/8
"bootdevice: Add interface to gather LCHS" too.
John, what do you think about extracting the *boot_device* functions out
of "sysemu.h"?
Thanks,
Phil.
>
> /* handler to set the boot_device order for a specific type of MachineClass */
> typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
> Hi Sam,
>
> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>> From: Sam Eiderman <shmuel.eiderman@oracle.com>
>>
>> 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 new fw_cfg entry is "bios-geometry".
>>
>> 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 | 32 ++++++++++++++++++++++++++++++++
>> hw/nvram/fw_cfg.c | 14 +++++++++++---
>> include/sysemu/sysemu.h | 1 +
>> 3 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index 2b12fb85a4..b034ad7bdc 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix)
>> }
>> }
>> }
>> +
>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>> +char *get_boot_devices_lchs_list(size_t *size)
>> +{
>> + FWLCHSEntry *i;
>> + size_t total = 0;
>> + char *list = NULL;
>> +
>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
>> + char *bootpath;
>> + char *chs_string;
>> + size_t len;
>> +
>> + bootpath = get_boot_device_path(i->dev, false, i->suffix);
>> + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
>> + bootpath, i->lcyls, i->lheads, i->lsecs);
>
> Hmm maybe we can g_free(bootpath) directly here.
>
I think it's okay to do it at the bottom of the loop. No real benefit to
being that eager to free resources in my mind. I expect setup at the top
of a block and teardown at the bottom of a block.
Trying to do too much in the middle gets messy in my opinion, not that
it seems to matter here.
>> +
>> + if (total) {
>> + list[total - 1] = '\n';
>> + }
>> + len = strlen(chs_string) + 1;
>> + list = g_realloc(list, total + len);
>> + memcpy(&list[total], chs_string, len);
>> + total += len;
>> + g_free(chs_string);
>> + g_free(bootpath);
>> + }
>> +
>> + *size = total;
>> +
>> + return list;
>> +}
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 7dc3ac378e..18aff658c0 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -920,13 +920,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_lchs_list(&len);
>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);
>
> OK. Can you add a test in tests/fw_cfg-test.c please?
>
:D
>> + 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 5bc5c79cbc..80c57fdc4e 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -106,6 +106,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_lchs_list(size_t *size);
>
> Please add some documentation. At least 'size' must be non-NULL.
>
Sure; but I wasn't going to gate on it because this series went unloved
for so long. At this point, a follow-up patch is fine.
> Ideally you should add doc for the other functions added in 3/8
> "bootdevice: Add interface to gather LCHS" too.
>
Same thing here.
> John, what do you think about extracting the *boot_device* functions out
> of "sysemu.h"?
>
Potentially worthwhile; but not critical at the moment. The source tree
is not the best-organized thing as-is and I don't think it's fair to
hold this series up for much longer for nice-to-haves, ultimately.
More targeted improvements might avoid the "whose responsibility is it
to stage this?" hot potato we played with this one; so I'd rather have
smaller follow-up patches handled by the respective maintainers.
> Thanks,
>
> Phil.
>
Thanks for the reviews :)
--js
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On 9/26/19 8:26 PM, John Snow wrote:
> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
>> Hi Sam,
>>
>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>>> From: Sam Eiderman <shmuel.eiderman@oracle.com>
>>>
>>> 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 new fw_cfg entry is "bios-geometry".
>>>
>>> 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 | 32 ++++++++++++++++++++++++++++++++
>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
>>> include/sysemu/sysemu.h | 1 +
>>> 3 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bootdevice.c b/bootdevice.c
>>> index 2b12fb85a4..b034ad7bdc 100644
>>> --- a/bootdevice.c
>>> +++ b/bootdevice.c
>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix)
>>> }
>>> }
>>> }
>>> +
>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>>> +char *get_boot_devices_lchs_list(size_t *size)
>>> +{
>>> + FWLCHSEntry *i;
>>> + size_t total = 0;
>>> + char *list = NULL;
>>> +
>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
>>> + char *bootpath;
>>> + char *chs_string;
>>> + size_t len;
>>> +
>>> + bootpath = get_boot_device_path(i->dev, false, i->suffix);
>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
>>> + bootpath, i->lcyls, i->lheads, i->lsecs);
>>
>> Hmm maybe we can g_free(bootpath) directly here.
>>
>
> I think it's okay to do it at the bottom of the loop. No real benefit to
> being that eager to free resources in my mind. I expect setup at the top
> of a block and teardown at the bottom of a block.
>
> Trying to do too much in the middle gets messy in my opinion, not that
> it seems to matter here.
No problem.
>>> +
>>> + if (total) {
>>> + list[total - 1] = '\n';
>>> + }
>>> + len = strlen(chs_string) + 1;
>>> + list = g_realloc(list, total + len);
>>> + memcpy(&list[total], chs_string, len);
>>> + total += len;
>>> + g_free(chs_string);
>>> + g_free(bootpath);
>>> + }
>>> +
>>> + *size = total;
>>> +
>>> + return list;
>>> +}
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 7dc3ac378e..18aff658c0 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -920,13 +920,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_lchs_list(&len);
>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);
>>
>> OK. Can you add a test in tests/fw_cfg-test.c please?
>>
>
> :D
>
>>> + 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 5bc5c79cbc..80c57fdc4e 100644
>>> --- a/include/sysemu/sysemu.h
>>> +++ b/include/sysemu/sysemu.h
>>> @@ -106,6 +106,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_lchs_list(size_t *size);
>>
>> Please add some documentation. At least 'size' must be non-NULL.
>>
>
> Sure; but I wasn't going to gate on it because this series went unloved
> for so long. At this point, a follow-up patch is fine.
OK
>
>> Ideally you should add doc for the other functions added in 3/8
>> "bootdevice: Add interface to gather LCHS" too.
>>
>
> Same thing here.
>
>> John, what do you think about extracting the *boot_device* functions out
>> of "sysemu.h"?
>>
>
> Potentially worthwhile; but not critical at the moment. The source tree
> is not the best-organized thing as-is and I don't think it's fair to
> hold this series up for much longer for nice-to-haves, ultimately.
>
> More targeted improvements might avoid the "whose responsibility is it
> to stage this?" hot potato we played with this one; so I'd rather have
> smaller follow-up patches handled by the respective maintainers.
Sure, fair enough.
>
>> Thanks,
>>
>> Phil.
>>
> Thanks for the reviews :)
:)
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
> On 9/26/19 8:26 PM, John Snow wrote:
>> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Sam,
>>>
>>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>>>> From: Sam Eiderman <shmuel.eiderman@oracle.com>
>>>>
>>>> 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 new fw_cfg entry is "bios-geometry".
>>>>
>>>> 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 | 32 ++++++++++++++++++++++++++++++++
>>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
>>>> include/sysemu/sysemu.h | 1 +
>>>> 3 files changed, 44 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>> index 2b12fb85a4..b034ad7bdc 100644
>>>> --- a/bootdevice.c
>>>> +++ b/bootdevice.c
>>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix)
>>>> }
>>>> }
>>>> }
>>>> +
>>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>>>> +char *get_boot_devices_lchs_list(size_t *size)
>>>> +{
>>>> + FWLCHSEntry *i;
>>>> + size_t total = 0;
>>>> + char *list = NULL;
>>>> +
>>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
>>>> + char *bootpath;
>>>> + char *chs_string;
>>>> + size_t len;
>>>> +
>>>> + bootpath = get_boot_device_path(i->dev, false, i->suffix);
>>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32,
>>>> + bootpath, i->lcyls, i->lheads, i->lsecs);
>>>
>>> Hmm maybe we can g_free(bootpath) directly here.
>>>
>>
>> I think it's okay to do it at the bottom of the loop. No real benefit to
>> being that eager to free resources in my mind. I expect setup at the top
>> of a block and teardown at the bottom of a block.
>>
>> Trying to do too much in the middle gets messy in my opinion, not that
>> it seems to matter here.
>
> No problem.
>
>>>> +
>>>> + if (total) {
>>>> + list[total - 1] = '\n';
>>>> + }
>>>> + len = strlen(chs_string) + 1;
>>>> + list = g_realloc(list, total + len);
>>>> + memcpy(&list[total], chs_string, len);
>>>> + total += len;
>>>> + g_free(chs_string);
>>>> + g_free(bootpath);
>>>> + }
>>>> +
>>>> + *size = total;
>>>> +
>>>> + return list;
>>>> +}
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index 7dc3ac378e..18aff658c0 100644
>>>> --- a/hw/nvram/fw_cfg.c
>>>> +++ b/hw/nvram/fw_cfg.c
>>>> @@ -920,13 +920,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_lchs_list(&len);
>>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len);
>>>
>>> OK. Can you add a test in tests/fw_cfg-test.c please?
>>>
>>
>> :D
>>
>>>> + 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 5bc5c79cbc..80c57fdc4e 100644
>>>> --- a/include/sysemu/sysemu.h
>>>> +++ b/include/sysemu/sysemu.h
>>>> @@ -106,6 +106,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_lchs_list(size_t *size);
>>>
>>> Please add some documentation. At least 'size' must be non-NULL.
>>>
>>
>> Sure; but I wasn't going to gate on it because this series went unloved
>> for so long. At this point, a follow-up patch is fine.
>
> OK
>
>>
>>> Ideally you should add doc for the other functions added in 3/8
>>> "bootdevice: Add interface to gather LCHS" too.
>>>
>>
>> Same thing here.
>>
>>> John, what do you think about extracting the *boot_device* functions out
>>> of "sysemu.h"?
>>>
>>
>> Potentially worthwhile; but not critical at the moment. The source tree
>> is not the best-organized thing as-is and I don't think it's fair to
>> hold this series up for much longer for nice-to-haves, ultimately.
>>
>> More targeted improvements might avoid the "whose responsibility is it
>> to stage this?" hot potato we played with this one; so I'd rather have
>> smaller follow-up patches handled by the respective maintainers.
>
> Sure, fair enough.
I forgot:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
Philippe, thanks for the fast review,
John, thanks for picking up this hot potato :-)
Sam
On Thu, Sep 26, 2019 at 10:16 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:
> On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
> > On 9/26/19 8:26 PM, John Snow wrote:
> >> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
> >>> Hi Sam,
> >>>
> >>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
> >>>> From: Sam Eiderman <shmuel.eiderman@oracle.com>
> >>>>
> >>>> 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 new fw_cfg entry is "bios-geometry".
> >>>>
> >>>> 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 | 32 ++++++++++++++++++++++++++++++++
> >>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
> >>>> include/sysemu/sysemu.h | 1 +
> >>>> 3 files changed, 44 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/bootdevice.c b/bootdevice.c
> >>>> index 2b12fb85a4..b034ad7bdc 100644
> >>>> --- a/bootdevice.c
> >>>> +++ b/bootdevice.c
> >>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev,
> const char *suffix)
> >>>> }
> >>>> }
> >>>> }
> >>>> +
> >>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
> >>>> +char *get_boot_devices_lchs_list(size_t *size)
> >>>> +{
> >>>> + FWLCHSEntry *i;
> >>>> + size_t total = 0;
> >>>> + char *list = NULL;
> >>>> +
> >>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
> >>>> + char *bootpath;
> >>>> + char *chs_string;
> >>>> + size_t len;
> >>>> +
> >>>> + bootpath = get_boot_device_path(i->dev, false, i->suffix);
> >>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %"
> PRIu32,
> >>>> + bootpath, i->lcyls, i->lheads,
> i->lsecs);
> >>>
> >>> Hmm maybe we can g_free(bootpath) directly here.
> >>>
> >>
> >> I think it's okay to do it at the bottom of the loop. No real benefit to
> >> being that eager to free resources in my mind. I expect setup at the top
> >> of a block and teardown at the bottom of a block.
> >>
> >> Trying to do too much in the middle gets messy in my opinion, not that
> >> it seems to matter here.
> >
> > No problem.
> >
> >>>> +
> >>>> + if (total) {
> >>>> + list[total - 1] = '\n';
> >>>> + }
> >>>> + len = strlen(chs_string) + 1;
> >>>> + list = g_realloc(list, total + len);
> >>>> + memcpy(&list[total], chs_string, len);
> >>>> + total += len;
> >>>> + g_free(chs_string);
> >>>> + g_free(bootpath);
> >>>> + }
> >>>> +
> >>>> + *size = total;
> >>>> +
> >>>> + return list;
> >>>> +}
> >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >>>> index 7dc3ac378e..18aff658c0 100644
> >>>> --- a/hw/nvram/fw_cfg.c
> >>>> +++ b/hw/nvram/fw_cfg.c
> >>>> @@ -920,13 +920,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_lchs_list(&len);
> >>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf,
> len);
> >>>
> >>> OK. Can you add a test in tests/fw_cfg-test.c please?
> >>>
> >>
> >> :D
> >>
> >>>> + 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 5bc5c79cbc..80c57fdc4e 100644
> >>>> --- a/include/sysemu/sysemu.h
> >>>> +++ b/include/sysemu/sysemu.h
> >>>> @@ -106,6 +106,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_lchs_list(size_t *size);
> >>>
> >>> Please add some documentation. At least 'size' must be non-NULL.
> >>>
> >>
> >> Sure; but I wasn't going to gate on it because this series went unloved
> >> for so long. At this point, a follow-up patch is fine.
> >
> > OK
> >
> >>
> >>> Ideally you should add doc for the other functions added in 3/8
> >>> "bootdevice: Add interface to gather LCHS" too.
> >>>
> >>
> >> Same thing here.
> >>
> >>> John, what do you think about extracting the *boot_device* functions
> out
> >>> of "sysemu.h"?
> >>>
> >>
> >> Potentially worthwhile; but not critical at the moment. The source tree
> >> is not the best-organized thing as-is and I don't think it's fair to
> >> hold this series up for much longer for nice-to-haves, ultimately.
> >>
> >> More targeted improvements might avoid the "whose responsibility is it
> >> to stage this?" hot potato we played with this one; so I'd rather have
> >> smaller follow-up patches handled by the respective maintainers.
> >
> > Sure, fair enough.
>
> I forgot:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
Hi Sam,
On 9/29/19 12:13 PM, Sam Eiderman wrote:
> Philippe, thanks for the fast review,
Fast is not always the friend of careful.
>
> John, thanks for picking up this hot potato :-)
>
> Sam
>
> On Thu, Sep 26, 2019 at 10:16 PM Philippe Mathieu-Daudé
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>
> On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
> > On 9/26/19 8:26 PM, John Snow wrote:
> >> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
> >>> Hi Sam,
> >>>
> >>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
> >>>> From: Sam Eiderman <shmuel.eiderman@oracle.com
> <mailto:shmuel.eiderman@oracle.com>>
> >>>>
> >>>> 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 new fw_cfg entry is "bios-geometry".
> >>>>
> >>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com
> <mailto:karl.heubaum@oracle.com>>
> >>>> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com
> <mailto:arbel.moshe@oracle.com>>
> >>>> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com
> <mailto:shmuel.eiderman@oracle.com>>
> >>>> ---
> >>>> bootdevice.c | 32 ++++++++++++++++++++++++++++++++
> >>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
> >>>> include/sysemu/sysemu.h | 1 +
> >>>> 3 files changed, 44 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/bootdevice.c b/bootdevice.c
> >>>> index 2b12fb85a4..b034ad7bdc 100644
> >>>> --- a/bootdevice.c
> >>>> +++ b/bootdevice.c
> >>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState
> *dev, const char *suffix)
> >>>> }
> >>>> }
> >>>> }
> >>>> +
> >>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
I suppose the lchs struct is serialized in little-endian.
> >>>> +char *get_boot_devices_lchs_list(size_t *size)
> >>>> +{
> >>>> + FWLCHSEntry *i;
> >>>> + size_t total = 0;
> >>>> + char *list = NULL;
> >>>> +
> >>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
> >>>> + char *bootpath;
> >>>> + char *chs_string;
> >>>> + size_t len;
> >>>> +
> >>>> + bootpath = get_boot_device_path(i->dev, false,
> i->suffix);
> >>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %"
> PRIu32 " %" PRIu32,
> >>>> + bootpath, i->lcyls,
> i->lheads, i->lsecs);
Sam. can you check if you don't need endianness conversion here?
> >>>
> >>> Hmm maybe we can g_free(bootpath) directly here.
> >>>
> >>
> >> I think it's okay to do it at the bottom of the loop. No real
> benefit to
> >> being that eager to free resources in my mind. I expect setup at
> the top
> >> of a block and teardown at the bottom of a block.
> >>
> >> Trying to do too much in the middle gets messy in my opinion,
> not that
> >> it seems to matter here.
> >
> > No problem.
> >
> >>>> +
> >>>> + if (total) {
> >>>> + list[total - 1] = '\n';
> >>>> + }
> >>>> + len = strlen(chs_string) + 1;
> >>>> + list = g_realloc(list, total + len);
> >>>> + memcpy(&list[total], chs_string, len);
> >>>> + total += len;
> >>>> + g_free(chs_string);
> >>>> + g_free(bootpath);
> >>>> + }
> >>>> +
> >>>> + *size = total;
> >>>> +
> >>>> + return list;
> >>>> +}
> >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >>>> index 7dc3ac378e..18aff658c0 100644
> >>>> --- a/hw/nvram/fw_cfg.c
> >>>> +++ b/hw/nvram/fw_cfg.c
> >>>> @@ -920,13 +920,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_lchs_list(&len);
> >>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t
> *)buf, len);
> >>>
> >>> OK. Can you add a test in tests/fw_cfg-test.c please?
> >>>
> >>
> >> :D
> >>
> >>>> + 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 5bc5c79cbc..80c57fdc4e 100644
> >>>> --- a/include/sysemu/sysemu.h
> >>>> +++ b/include/sysemu/sysemu.h
> >>>> @@ -106,6 +106,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_lchs_list(size_t *size);
> >>>
> >>> Please add some documentation. At least 'size' must be non-NULL.
> >>>
> >>
> >> Sure; but I wasn't going to gate on it because this series went
> unloved
> >> for so long. At this point, a follow-up patch is fine.
> >
> > OK
> >
> >>
> >>> Ideally you should add doc for the other functions added in 3/8
> >>> "bootdevice: Add interface to gather LCHS" too.
> >>>
> >>
> >> Same thing here.
> >>
> >>> John, what do you think about extracting the *boot_device*
> functions out
> >>> of "sysemu.h"?
> >>>
> >>
> >> Potentially worthwhile; but not critical at the moment. The
> source tree
> >> is not the best-organized thing as-is and I don't think it's fair to
> >> hold this series up for much longer for nice-to-haves, ultimately.
> >>
> >> More targeted improvements might avoid the "whose responsibility
> is it
> >> to stage this?" hot potato we played with this one; so I'd
> rather have
> >> smaller follow-up patches handled by the respective maintainers.
> >
> > Sure, fair enough.
>
> I forgot:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>>
Meanwhile I withdraw my fast R-b :(
Regards,
Phil.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
On Tue, Oct 8, 2019, 13:34 Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Sam,
>
> On 9/29/19 12:13 PM, Sam Eiderman wrote:
> > Philippe, thanks for the fast review,
>
> Fast is not always the friend of careful.
>
> >
> > John, thanks for picking up this hot potato :-)
> >
> > Sam
> >
> > On Thu, Sep 26, 2019 at 10:16 PM Philippe Mathieu-Daudé
> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> >
> > On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
> > > On 9/26/19 8:26 PM, John Snow wrote:
> > >> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
> > >>> Hi Sam,
> > >>>
> > >>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
> > >>>> From: Sam Eiderman <shmuel.eiderman@oracle.com
> > <mailto:shmuel.eiderman@oracle.com>>
> > >>>>
> > >>>> 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 new fw_cfg entry is "bios-geometry".
> > >>>>
> > >>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com
> > <mailto:karl.heubaum@oracle.com>>
> > >>>> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com
> > <mailto:arbel.moshe@oracle.com>>
> > >>>> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com
> > <mailto:shmuel.eiderman@oracle.com>>
> > >>>> ---
> > >>>> bootdevice.c | 32 ++++++++++++++++++++++++++++++++
> > >>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
> > >>>> include/sysemu/sysemu.h | 1 +
> > >>>> 3 files changed, 44 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/bootdevice.c b/bootdevice.c
> > >>>> index 2b12fb85a4..b034ad7bdc 100644
> > >>>> --- a/bootdevice.c
> > >>>> +++ b/bootdevice.c
> > >>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState
> > *dev, const char *suffix)
> > >>>> }
> > >>>> }
> > >>>> }
> > >>>> +
> > >>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>
> I suppose the lchs struct is serialized in little-endian.
>
Nice catch, that's just a bad comment, should be removed.
There used to be a struct with 3 uint32_t values, Laszlo pointed out that
there is an endianess problem (this was fixed in v3) later Kevin suggested
to make it a textual interface and the struct was removed (in v4) but the
comment remained.
>
> > >>>> +char *get_boot_devices_lchs_list(size_t *size)
> > >>>> +{
> > >>>> + FWLCHSEntry *i;
> > >>>> + size_t total = 0;
> > >>>> + char *list = NULL;
> > >>>> +
> > >>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
> > >>>> + char *bootpath;
> > >>>> + char *chs_string;
> > >>>> + size_t len;
> > >>>> +
> > >>>> + bootpath = get_boot_device_path(i->dev, false,
> > i->suffix);
> > >>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %"
> > PRIu32 " %" PRIu32,
> > >>>> + bootpath, i->lcyls,
> > i->lheads, i->lsecs);
>
> Sam. can you check if you don't need endianness conversion here?
>
Hmm, since this is a textual interface, I believe this should work no?
uint32_t a = 4;
g_strdup_printf("%s" PRIu32, a);
Should return "4" no matter the endianess? (Taken care of by glib?)
> > >>>
> > >>> Hmm maybe we can g_free(bootpath) directly here.
> > >>>
> > >>
> > >> I think it's okay to do it at the bottom of the loop. No real
> > benefit to
> > >> being that eager to free resources in my mind. I expect setup at
> > the top
> > >> of a block and teardown at the bottom of a block.
> > >>
> > >> Trying to do too much in the middle gets messy in my opinion,
> > not that
> > >> it seems to matter here.
> > >
> > > No problem.
> > >
> > >>>> +
> > >>>> + if (total) {
> > >>>> + list[total - 1] = '\n';
> > >>>> + }
> > >>>> + len = strlen(chs_string) + 1;
> > >>>> + list = g_realloc(list, total + len);
> > >>>> + memcpy(&list[total], chs_string, len);
> > >>>> + total += len;
> > >>>> + g_free(chs_string);
> > >>>> + g_free(bootpath);
> > >>>> + }
> > >>>> +
> > >>>> + *size = total;
> > >>>> +
> > >>>> + return list;
> > >>>> +}
> > >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > >>>> index 7dc3ac378e..18aff658c0 100644
> > >>>> --- a/hw/nvram/fw_cfg.c
> > >>>> +++ b/hw/nvram/fw_cfg.c
> > >>>> @@ -920,13 +920,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_lchs_list(&len);
> > >>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t
> > *)buf, len);
> > >>>
> > >>> OK. Can you add a test in tests/fw_cfg-test.c please?
> > >>>
> > >>
> > >> :D
> > >>
> > >>>> + 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 5bc5c79cbc..80c57fdc4e 100644
> > >>>> --- a/include/sysemu/sysemu.h
> > >>>> +++ b/include/sysemu/sysemu.h
> > >>>> @@ -106,6 +106,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_lchs_list(size_t *size);
> > >>>
> > >>> Please add some documentation. At least 'size' must be non-NULL.
> > >>>
> > >>
> > >> Sure; but I wasn't going to gate on it because this series went
> > unloved
> > >> for so long. At this point, a follow-up patch is fine.
> > >
> > > OK
> > >
> > >>
> > >>> Ideally you should add doc for the other functions added in 3/8
> > >>> "bootdevice: Add interface to gather LCHS" too.
> > >>>
> > >>
> > >> Same thing here.
> > >>
> > >>> John, what do you think about extracting the *boot_device*
> > functions out
> > >>> of "sysemu.h"?
> > >>>
> > >>
> > >> Potentially worthwhile; but not critical at the moment. The
> > source tree
> > >> is not the best-organized thing as-is and I don't think it's
> fair to
> > >> hold this series up for much longer for nice-to-haves,
> ultimately.
> > >>
> > >> More targeted improvements might avoid the "whose responsibility
> > is it
> > >> to stage this?" hot potato we played with this one; so I'd
> > rather have
> > >> smaller follow-up patches handled by the respective maintainers.
> > >
> > > Sure, fair enough.
> >
> > I forgot:
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > <mailto:philmd@redhat.com>>
>
> Meanwhile I withdraw my fast R-b :(
>
> Regards,
>
> Phil.
>
Gentle Ping,
Philippe, John?
Just wondering if the series is okay, as Gerd pointed out this series
is a blocker for the corresponding changes in SeaBIOS for v 1.13
Sam
On Tue, Oct 8, 2019 at 2:51 PM Sam Eiderman <sameid@google.com> wrote:
>
>
>
> On Tue, Oct 8, 2019, 13:34 Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Sam,
>>
>> On 9/29/19 12:13 PM, Sam Eiderman wrote:
>> > Philippe, thanks for the fast review,
>>
>> Fast is not always the friend of careful.
>>
>> >
>> > John, thanks for picking up this hot potato :-)
>> >
>> > Sam
>> >
>> > On Thu, Sep 26, 2019 at 10:16 PM Philippe Mathieu-Daudé
>> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>> >
>> > On 9/26/19 9:09 PM, Philippe Mathieu-Daudé wrote:
>> > > On 9/26/19 8:26 PM, John Snow wrote:
>> > >> On 9/26/19 5:57 AM, Philippe Mathieu-Daudé wrote:
>> > >>> Hi Sam,
>> > >>>
>> > >>> On 9/25/19 1:06 PM, Sam Eiderman wrote:
>> > >>>> From: Sam Eiderman <shmuel.eiderman@oracle.com
>> > <mailto:shmuel.eiderman@oracle.com>>
>> > >>>>
>> > >>>> 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 new fw_cfg entry is "bios-geometry".
>> > >>>>
>> > >>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com
>> > <mailto:karl.heubaum@oracle.com>>
>> > >>>> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com
>> > <mailto:arbel.moshe@oracle.com>>
>> > >>>> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com
>> > <mailto:shmuel.eiderman@oracle.com>>
>> > >>>> ---
>> > >>>> bootdevice.c | 32 ++++++++++++++++++++++++++++++++
>> > >>>> hw/nvram/fw_cfg.c | 14 +++++++++++---
>> > >>>> include/sysemu/sysemu.h | 1 +
>> > >>>> 3 files changed, 44 insertions(+), 3 deletions(-)
>> > >>>>
>> > >>>> diff --git a/bootdevice.c b/bootdevice.c
>> > >>>> index 2b12fb85a4..b034ad7bdc 100644
>> > >>>> --- a/bootdevice.c
>> > >>>> +++ b/bootdevice.c
>> > >>>> @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState
>> > *dev, const char *suffix)
>> > >>>> }
>> > >>>> }
>> > >>>> }
>> > >>>> +
>> > >>>> +/* Serialized as: (device name\0 + lchs struct) x devices */
>>
>> I suppose the lchs struct is serialized in little-endian.
>
>
> Nice catch, that's just a bad comment, should be removed.
> There used to be a struct with 3 uint32_t values, Laszlo pointed out that there is an endianess problem (this was fixed in v3) later Kevin suggested to make it a textual interface and the struct was removed (in v4) but the comment remained.
>>
>>
>> > >>>> +char *get_boot_devices_lchs_list(size_t *size)
>> > >>>> +{
>> > >>>> + FWLCHSEntry *i;
>> > >>>> + size_t total = 0;
>> > >>>> + char *list = NULL;
>> > >>>> +
>> > >>>> + QTAILQ_FOREACH(i, &fw_lchs, link) {
>> > >>>> + char *bootpath;
>> > >>>> + char *chs_string;
>> > >>>> + size_t len;
>> > >>>> +
>> > >>>> + bootpath = get_boot_device_path(i->dev, false,
>> > i->suffix);
>> > >>>> + chs_string = g_strdup_printf("%s %" PRIu32 " %"
>> > PRIu32 " %" PRIu32,
>> > >>>> + bootpath, i->lcyls,
>> > i->lheads, i->lsecs);
>>
>> Sam. can you check if you don't need endianness conversion here?
>
>
> Hmm, since this is a textual interface, I believe this should work no?
>
> uint32_t a = 4;
> g_strdup_printf("%s" PRIu32, a);
>
> Should return "4" no matter the endianess? (Taken care of by glib?)
>
>>
>> > >>>
>> > >>> Hmm maybe we can g_free(bootpath) directly here.
>> > >>>
>> > >>
>> > >> I think it's okay to do it at the bottom of the loop. No real
>> > benefit to
>> > >> being that eager to free resources in my mind. I expect setup at
>> > the top
>> > >> of a block and teardown at the bottom of a block.
>> > >>
>> > >> Trying to do too much in the middle gets messy in my opinion,
>> > not that
>> > >> it seems to matter here.
>> > >
>> > > No problem.
>> > >
>> > >>>> +
>> > >>>> + if (total) {
>> > >>>> + list[total - 1] = '\n';
>> > >>>> + }
>> > >>>> + len = strlen(chs_string) + 1;
>> > >>>> + list = g_realloc(list, total + len);
>> > >>>> + memcpy(&list[total], chs_string, len);
>> > >>>> + total += len;
>> > >>>> + g_free(chs_string);
>> > >>>> + g_free(bootpath);
>> > >>>> + }
>> > >>>> +
>> > >>>> + *size = total;
>> > >>>> +
>> > >>>> + return list;
>> > >>>> +}
>> > >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> > >>>> index 7dc3ac378e..18aff658c0 100644
>> > >>>> --- a/hw/nvram/fw_cfg.c
>> > >>>> +++ b/hw/nvram/fw_cfg.c
>> > >>>> @@ -920,13 +920,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_lchs_list(&len);
>> > >>>> + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t
>> > *)buf, len);
>> > >>>
>> > >>> OK. Can you add a test in tests/fw_cfg-test.c please?
>> > >>>
>> > >>
>> > >> :D
>> > >>
>> > >>>> + 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 5bc5c79cbc..80c57fdc4e 100644
>> > >>>> --- a/include/sysemu/sysemu.h
>> > >>>> +++ b/include/sysemu/sysemu.h
>> > >>>> @@ -106,6 +106,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_lchs_list(size_t *size);
>> > >>>
>> > >>> Please add some documentation. At least 'size' must be non-NULL.
>> > >>>
>> > >>
>> > >> Sure; but I wasn't going to gate on it because this series went
>> > unloved
>> > >> for so long. At this point, a follow-up patch is fine.
>> > >
>> > > OK
>> > >
>> > >>
>> > >>> Ideally you should add doc for the other functions added in 3/8
>> > >>> "bootdevice: Add interface to gather LCHS" too.
>> > >>>
>> > >>
>> > >> Same thing here.
>> > >>
>> > >>> John, what do you think about extracting the *boot_device*
>> > functions out
>> > >>> of "sysemu.h"?
>> > >>>
>> > >>
>> > >> Potentially worthwhile; but not critical at the moment. The
>> > source tree
>> > >> is not the best-organized thing as-is and I don't think it's fair to
>> > >> hold this series up for much longer for nice-to-haves, ultimately.
>> > >>
>> > >> More targeted improvements might avoid the "whose responsibility
>> > is it
>> > >> to stage this?" hot potato we played with this one; so I'd
>> > rather have
>> > >> smaller follow-up patches handled by the respective maintainers.
>> > >
>> > > Sure, fair enough.
>> >
>> > I forgot:
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>> > <mailto:philmd@redhat.com>>
>>
>> Meanwhile I withdraw my fast R-b :(
>>
>> Regards,
>>
>> Phil.
Hi Sam,
On 10/16/19 13:02, Sam Eiderman wrote:
> Gentle Ping,
>
> Philippe, John?
>
> Just wondering if the series is okay, as Gerd pointed out this series
> is a blocker for the corresponding changes in SeaBIOS for v 1.13
The QEMU series is still not merged, due to a bug in the last patch
(namely, the test case, "hd-geo-test: Add tests for lchs override").
To my knowledge, SeaBIOS prefers to merge patches with the underlying
QEMU patches merged first, so you'll likely have to fix that QEMU issue
first.
I explained the bug in the QEMU test case here:
http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com
http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com
(Alternative links to the same:
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html
)
I've never received feedback to those messages, and I think you must
have missed them.
FWIW, when I hit "Reply All" in that thread, you were on the "To:" list
with:
Sam Eiderman <shmuel.eiderman@oracle.com>
but here you are present with
Sam Eiderman <sameid@google.com>
In addition, when I posted those messages, I got the following
auto-response ("Undelivered Mail Returned to Sender"):
> This is the mail system at host mx1.redhat.com.
>
> I'm sorry to have to inform you that your message could not
> be delivered to one or more recipients. It's attached below.
>
> For further assistance, please send mail to postmaster.
>
> If you do so, please include this problem report. You can
> delete your own text from the attached returned message.
>
> The mail system
>
> <shmuel.eiderman@oracle.com>: host
> aserp2030.oracle.com[141.146.126.74] said:
> 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command)
I didn't know your new address, so I could only hope you'd find my
feedback on qemu-devel.
Thanks
Laszlo
On 10/16/19 2:14 PM, Laszlo Ersek wrote:
> Hi Sam,
>
> On 10/16/19 13:02, Sam Eiderman wrote:
>> Gentle Ping,
>>
>> Philippe, John?
>>
>> Just wondering if the series is okay, as Gerd pointed out this series
>> is a blocker for the corresponding changes in SeaBIOS for v 1.13
>
> The QEMU series is still not merged, due to a bug in the last patch
> (namely, the test case, "hd-geo-test: Add tests for lchs override").
>
> To my knowledge, SeaBIOS prefers to merge patches with the underlying
> QEMU patches merged first, so you'll likely have to fix that QEMU issue
> first.
>
> I explained the bug in the QEMU test case here:
>
> http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com
> http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com
Yes, I was expecting a respin with find_fw_cfg_file() fixed per Laszlo
detailed review.
> (Alternative links to the same:
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html
> )
>
> I've never received feedback to those messages, and I think you must
> have missed them.
>
> FWIW, when I hit "Reply All" in that thread, you were on the "To:" list
> with:
>
> Sam Eiderman <shmuel.eiderman@oracle.com>
>
> but here you are present with
>
> Sam Eiderman <sameid@google.com>
>
> In addition, when I posted those messages, I got the following
> auto-response ("Undelivered Mail Returned to Sender"):
>
>> This is the mail system at host mx1.redhat.com.
>>
>> I'm sorry to have to inform you that your message could not
>> be delivered to one or more recipients. It's attached below.
>>
>> For further assistance, please send mail to postmaster.
>>
>> If you do so, please include this problem report. You can
>> delete your own text from the attached returned message.
>>
>> The mail system
>>
>> <shmuel.eiderman@oracle.com>: host
>> aserp2030.oracle.com[141.146.126.74] said:
>> 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command)
That explains it :)
>
> I didn't know your new address, so I could only hope you'd find my
> feedback on qemu-devel.
>
> Thanks
> Laszlo
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
Thanks for the detailed comment Laszlo,
Indeed my e-mail has changed and I only received replies to the
commits where I added this new mail in the S-o-b section, should of
added in all of them.
So as you said it, the problem was actually in using qfw_cfg_get_u32
which assumes the value is encoded LE and has an additional
le32_to_cpu, should have used qfw_cfg_get directly like
qfw_cfg_get_file does.
Regarding qfw_cfg_get_file - I wrote this code when this function did
not exist yet, I think it was added 6 months ago. In any case, I will
use it instead.
Thanks for this.
I will resubmit this entire commit series:
* I will only change code in the last commit (tests)
* I will remove a comment which is now not true anymore
* I will add my new email in S-o-b
Sam
On Wed, Oct 16, 2019 at 3:29 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 10/16/19 2:14 PM, Laszlo Ersek wrote:
> > Hi Sam,
> >
> > On 10/16/19 13:02, Sam Eiderman wrote:
> >> Gentle Ping,
> >>
> >> Philippe, John?
> >>
> >> Just wondering if the series is okay, as Gerd pointed out this series
> >> is a blocker for the corresponding changes in SeaBIOS for v 1.13
> >
> > The QEMU series is still not merged, due to a bug in the last patch
> > (namely, the test case, "hd-geo-test: Add tests for lchs override").
> >
> > To my knowledge, SeaBIOS prefers to merge patches with the underlying
> > QEMU patches merged first, so you'll likely have to fix that QEMU issue
> > first.
> >
> > I explained the bug in the QEMU test case here:
> >
> > http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com
> > http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com
>
> Yes, I was expecting a respin with find_fw_cfg_file() fixed per Laszlo
> detailed review.
>
> > (Alternative links to the same:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html
> > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html
> > )
> >
> > I've never received feedback to those messages, and I think you must
> > have missed them.
> >
> > FWIW, when I hit "Reply All" in that thread, you were on the "To:" list
> > with:
> >
> > Sam Eiderman <shmuel.eiderman@oracle.com>
> >
> > but here you are present with
> >
> > Sam Eiderman <sameid@google.com>
> >
> > In addition, when I posted those messages, I got the following
> > auto-response ("Undelivered Mail Returned to Sender"):
> >
> >> This is the mail system at host mx1.redhat.com.
> >>
> >> I'm sorry to have to inform you that your message could not
> >> be delivered to one or more recipients. It's attached below.
> >>
> >> For further assistance, please send mail to postmaster.
> >>
> >> If you do so, please include this problem report. You can
> >> delete your own text from the attached returned message.
> >>
> >> The mail system
> >>
> >> <shmuel.eiderman@oracle.com>: host
> >> aserp2030.oracle.com[141.146.126.74] said:
> >> 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command)
>
> That explains it :)
>
> >
> > I didn't know your new address, so I could only hope you'd find my
> > feedback on qemu-devel.
> >
> > Thanks
> > Laszlo
> >
On 10/16/19 10:55 AM, Sam Eiderman wrote: > Thanks for the detailed comment Laszlo, > > Indeed my e-mail has changed and I only received replies to the > commits where I added this new mail in the S-o-b section, should of > added in all of them. > > So as you said it, the problem was actually in using qfw_cfg_get_u32 > which assumes the value is encoded LE and has an additional > le32_to_cpu, should have used qfw_cfg_get directly like > qfw_cfg_get_file does. > > Regarding qfw_cfg_get_file - I wrote this code when this function did > not exist yet, I think it was added 6 months ago. In any case, I will > use it instead. > > Thanks for this. > > I will resubmit this entire commit series: > * I will only change code in the last commit (tests) > * I will remove a comment which is now not true anymore > * I will add my new email in S-o-b > > Sam > Philippe gave me a verbal tut-tut for not including his review tags in my last pull request; when you re-spin could you be so kind as to include any that still apply? --js _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
Sure! Philippe withdrew his R-b on 7/8, as I explained 7/8 is fine (only need to remove a bad comment) the problem was in the tests 8/8 - should I include the original R/b? I guess all other 1-6 are fine to add R/b... On Wed, Oct 16, 2019 at 6:07 PM John Snow <jsnow@redhat.com> wrote: > > > > On 10/16/19 10:55 AM, Sam Eiderman wrote: > > Thanks for the detailed comment Laszlo, > > > > Indeed my e-mail has changed and I only received replies to the > > commits where I added this new mail in the S-o-b section, should of > > added in all of them. > > > > So as you said it, the problem was actually in using qfw_cfg_get_u32 > > which assumes the value is encoded LE and has an additional > > le32_to_cpu, should have used qfw_cfg_get directly like > > qfw_cfg_get_file does. > > > > Regarding qfw_cfg_get_file - I wrote this code when this function did > > not exist yet, I think it was added 6 months ago. In any case, I will > > use it instead. > > > > Thanks for this. > > > > I will resubmit this entire commit series: > > * I will only change code in the last commit (tests) > > * I will remove a comment which is now not true anymore > > * I will add my new email in S-o-b > > > > Sam > > > > Philippe gave me a verbal tut-tut for not including his review tags in > my last pull request; when you re-spin could you be so kind as to > include any that still apply? > > --js
On 10/16/19 5:19 PM, Sam Eiderman wrote: > Sure! > > Philippe withdrew his R-b on 7/8, as I explained 7/8 is fine (only > need to remove a bad comment) the problem was in the tests 8/8 - > should I include the original R/b? I withdrew it because John was preparing his pull request, and I needed more time to review this again. But then Laszlo was quicker and figured out the problem is in the other patch, so please keep my original R-b. Thanks to all 3 of you :) > I guess all other 1-6 are fine to add R/b... > > On Wed, Oct 16, 2019 at 6:07 PM John Snow <jsnow@redhat.com> wrote: >> >> >> >> On 10/16/19 10:55 AM, Sam Eiderman wrote: >>> Thanks for the detailed comment Laszlo, >>> >>> Indeed my e-mail has changed and I only received replies to the >>> commits where I added this new mail in the S-o-b section, should of >>> added in all of them. >>> >>> So as you said it, the problem was actually in using qfw_cfg_get_u32 >>> which assumes the value is encoded LE and has an additional >>> le32_to_cpu, should have used qfw_cfg_get directly like >>> qfw_cfg_get_file does. >>> >>> Regarding qfw_cfg_get_file - I wrote this code when this function did >>> not exist yet, I think it was added 6 months ago. In any case, I will >>> use it instead. >>> >>> Thanks for this. >>> >>> I will resubmit this entire commit series: >>> * I will only change code in the last commit (tests) >>> * I will remove a comment which is now not true anymore >>> * I will add my new email in S-o-b >>> >>> Sam >>> >> >> Philippe gave me a verbal tut-tut for not including his review tags in >> my last pull request; when you re-spin could you be so kind as to >> include any that still apply? >> >> --js _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org
© 2016 - 2026 Red Hat, Inc.