On 06/12/19 11:42, Sam Eiderman wrote: > 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 fw_cfg entry is "bootdevices" and it serializes a struct. > At the moment the struct holds the values of logical CHS values but it > can be expanded easily due to the extendable ABI implemented. > > (In the future, we can pass the bootindex through "bootdevices" instead > "bootorder" - unifying all bootdevice information in one fw_cfg value)
I would disagree with that. UEFI guest firmware doesn't seem to have any use for this new type of information ("logical CHS values"), so the current interface (the "bootorder" fw_cfg file) should continue to work. The ArmVirtQemu and OVMF platform firmwares (built from the edk2 project, and bundled with QEMU 4.1+) implement some serious parsing and processing for "bootorder". Independently, another comment: > The PV interface through fw_cfg could have also been implemented using > device specific keys, e.g.: "/etc/bootdevice/%s/logical_geometry" where > %s is the device name QEMU produces - but this implementation would > require much more code refactoring, both in QEMU and SeaBIOS, so the > current implementation was chosen. > > Reviewed-by: Karl Heubaum <karl.heub...@oracle.com> > Reviewed-by: Arbel Moshe <arbel.mo...@oracle.com> > Signed-off-by: Sam Eiderman <shmuel.eider...@oracle.com> > --- > bootdevice.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > hw/nvram/fw_cfg.c | 14 +++++++++++--- > include/sysemu/sysemu.h | 1 + > 3 files changed, 54 insertions(+), 3 deletions(-) > > diff --git a/bootdevice.c b/bootdevice.c > index 2b12fb85a4..84c2a83f25 100644 > --- a/bootdevice.c > +++ b/bootdevice.c > @@ -405,3 +405,45 @@ void del_boot_device_lchs(DeviceState *dev, const char > *suffix) > } > } > } > + > +typedef struct QEMU_PACKED BootDeviceEntrySerialized { > + /* Do not change field order - add new fields below */ > + uint32_t lcyls; > + uint32_t lheads; > + uint32_t lsecs; > +} BootDeviceEntrySerialized; > + > +/* Serialized as: struct size (4) + (device name\0 + device struct) x > devices */ > +char *get_boot_devices_info(size_t *size) > +{ > + FWLCHSEntry *i; > + BootDeviceEntrySerialized s; > + size_t total = 0; > + char *list = NULL; > + > + list = g_malloc0(sizeof(uint32_t)); > + *((uint32_t *)list) = (uint32_t)sizeof(s); > + total = sizeof(uint32_t); > + > + QTAILQ_FOREACH(i, &fw_lchs, link) { > + char *bootpath; > + size_t len; > + > + bootpath = get_boot_device_path(i->dev, false, i->suffix); > + s.lcyls = i->lcyls; > + s.lheads = i->lheads; > + s.lsecs = i->lsecs; You should document the endianness of the fields in BootDeviceEntrySerialized, and then call byte order conversion functions here accordingly (most probably cpu_to_le32()). As written, this code would break if you ran qemu-system-x86_64 / qemu-system-i386 (with TCG acceleration) on a big endian host. Thanks Laszlo > + > + len = strlen(bootpath) + 1; > + list = g_realloc(list, total + len + sizeof(s)); > + memcpy(&list[total], bootpath, len); > + memcpy(&list[total + len], &s, sizeof(s)); > + total += len + sizeof(s); > + > + g_free(bootpath); > + } > + > + *size = total; > + > + return list; > +} > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 9f7b7789bc..008b21542f 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -916,13 +916,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_info(&len); > + ptr = fw_cfg_modify_file(s, "bootdevices", (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 173dfbb539..f0552006f4 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -174,6 +174,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_info(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, > _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org