| Message ID | 20190925110639.100699-8-sameid@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Qemu to SeaBIOS LCHS interface | expand |
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, >
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
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 :) :)
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>
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.
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 >
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
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
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,