v1:
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 guess - SeaBIOS, for large enough disks - will use LBA translation, which will report 63 SPT instead.
In addition we can not enforce SeaBIOS to rely on phyiscal geometries at all. A virtio-blk-pci virtual disk with 255 phyiscal heads can not report more than 16 physical heads when moved to an IDE controller, the ATA spec allows a maximum of 16 heads - this is an artifact of virtualization.
By supplying the logical geometies directly we are able to support such "exotic" disks.
We will use fw_cfg to do just that.
v2:
Fix missing parenthesis check in "hd-geo-test: Add tests for lchs override"
v3:
* Rename fw_cfg key to "bios-geometry". * Remove "extendible" interface. * Add cpu_to_le32 fix as Laszlo suggested or big endian hosts * Fix last qtest commit - automatic docker tester for some reason does not have qemu-img set
v4:
* Change fw_cfg interface from mixed textual/binary to textual only
v5:
* Fix line > 80 chars in tests/hd-geo-test.c
v6:
* Small fixes for issues pointed by Max * (&conf->conf)->lcyls to conf->conf.lcyls and so on * Remove scsi_unrealize from everything other than scsi-hd * Add proper include to sysemu.h * scsi_device_unrealize() after scsi_device_purge_requests()
v7:
* Adapted last commit (tests) to changes in qtest
Sam Eiderman (8): block: Refactor macros - fix tabbing block: Support providing LCHS from user bootdevice: Add interface to gather LCHS scsi: Propagate unrealize() callback to scsi-hd bootdevice: Gather LCHS from all relevant devices bootdevice: Refactor get_boot_devices_list bootdevice: FW_CFG interface for LCHS values hd-geo-test: Add tests for lchs override
bootdevice.c | 148 ++++++++-- hw/block/virtio-blk.c | 6 + hw/ide/qdev.c | 7 +- hw/nvram/fw_cfg.c | 14 +- hw/scsi/scsi-bus.c | 16 ++ hw/scsi/scsi-disk.c | 12 + include/hw/block/block.h | 22 +- include/hw/scsi/scsi.h | 1 + include/sysemu/sysemu.h | 4 + tests/Makefile.include | 2 +- tests/hd-geo-test.c | 589 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 780 insertions(+), 41 deletions(-)
From: Sam Eiderman shmuel.eiderman@oracle.com
Fixing tabbing in block related macros.
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 --- hw/ide/qdev.c | 2 +- include/hw/block/block.h | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 6fba6b62b8..6dd219944f 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -290,7 +290,7 @@ static void ide_drive_realize(IDEDevice *dev, Error **errp) DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf), \ DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf), \ DEFINE_PROP_STRING("ver", IDEDrive, dev.version), \ - DEFINE_PROP_UINT64("wwn", IDEDrive, dev.wwn, 0), \ + DEFINE_PROP_UINT64("wwn", IDEDrive, dev.wwn, 0), \ DEFINE_PROP_STRING("serial", IDEDrive, dev.serial),\ DEFINE_PROP_STRING("model", IDEDrive, dev.model)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h index 607539057a..fd55a30bca 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -50,21 +50,21 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) _conf.logical_block_size), \ DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ _conf.physical_block_size), \ - DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ + DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \ - DEFINE_PROP_UINT32("discard_granularity", _state, \ - _conf.discard_granularity, -1), \ - DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \ - ON_OFF_AUTO_AUTO), \ + DEFINE_PROP_UINT32("discard_granularity", _state, \ + _conf.discard_granularity, -1), \ + DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \ + ON_OFF_AUTO_AUTO), \ DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
#define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ DEFINE_BLOCK_PROPERTIES_BASE(_state, _conf)
-#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf) \ - DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0), \ - DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \ +#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf) \ + DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0), \ + DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \ DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
#define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf) \
On 9/25/19 1:06 PM, Sam Eiderman wrote:
From: Sam Eiderman shmuel.eiderman@oracle.com
Fixing tabbing in block related macros.
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
Reviewed-by: Philippe Mathieu-Daudé philmd@redhat.com
hw/ide/qdev.c | 2 +- include/hw/block/block.h | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 6fba6b62b8..6dd219944f 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -290,7 +290,7 @@ static void ide_drive_realize(IDEDevice *dev, Error **errp) DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf), \ DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf), \ DEFINE_PROP_STRING("ver", IDEDrive, dev.version), \
- DEFINE_PROP_UINT64("wwn", IDEDrive, dev.wwn, 0), \
- DEFINE_PROP_UINT64("wwn", IDEDrive, dev.wwn, 0), \ DEFINE_PROP_STRING("serial", IDEDrive, dev.serial),\ DEFINE_PROP_STRING("model", IDEDrive, dev.model)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h index 607539057a..fd55a30bca 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -50,21 +50,21 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) _conf.logical_block_size), \ DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ _conf.physical_block_size), \
- DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \
- DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \
- DEFINE_PROP_UINT32("discard_granularity", _state, \
_conf.discard_granularity, -1), \- DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
ON_OFF_AUTO_AUTO), \
- DEFINE_PROP_UINT32("discard_granularity", _state, \
_conf.discard_granularity, -1), \- DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
ON_OFF_AUTO_AUTO), \#define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ DEFINE_BLOCK_PROPERTIES_BASE(_state, _conf)
-#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf) \
- DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0), \
- DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
+#define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf) \
- DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0), \
- DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \ DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
#define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf) \
From: Sam Eiderman shmuel.eiderman@oracle.com
Add logical geometry variables to BlockConf.
A user can now supply "lcyls", "lheads" & "lsecs" for any HD device that supports CHS ("cyls", "heads", "secs").
These devices include: * ide-hd * scsi-hd * virtio-blk-pci
In future commits we will use the provided LCHS and pass it to the BIOS through fw_cfg to be supplied using INT13 routines.
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 --- include/hw/block/block.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h index fd55a30bca..d7246f3862 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -26,6 +26,7 @@ typedef struct BlockConf { uint32_t discard_granularity; /* geometry, not all devices use this */ uint32_t cyls, heads, secs; + uint32_t lcyls, lheads, lsecs; OnOffAuto wce; bool share_rw; BlockdevOnError rerror; @@ -65,7 +66,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf) \ DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0), \ DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \ - DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0) + DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0), \ + DEFINE_PROP_UINT32("lcyls", _state, _conf.lcyls, 0), \ + DEFINE_PROP_UINT32("lheads", _state, _conf.lheads, 0), \ + DEFINE_PROP_UINT32("lsecs", _state, _conf.lsecs, 0)
#define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf) \ DEFINE_PROP_BLOCKDEV_ON_ERROR("rerror", _state, _conf.rerror, \
From: Sam Eiderman shmuel.eiderman@oracle.com
Add an interface to provide direct logical CHS values for boot devices. We will use this interface in the next commits.
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 | 55 +++++++++++++++++++++++++++++++++++++++++ include/sysemu/sysemu.h | 3 +++ 2 files changed, 58 insertions(+)
diff --git a/bootdevice.c b/bootdevice.c index 1d225202f9..bc5e1c2de4 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -343,3 +343,58 @@ void device_add_bootindex_property(Object *obj, int32_t *bootindex, /* initialize devices' bootindex property to -1 */ object_property_set_int(obj, -1, name, NULL); } + +typedef struct FWLCHSEntry FWLCHSEntry; + +struct FWLCHSEntry { + QTAILQ_ENTRY(FWLCHSEntry) link; + DeviceState *dev; + char *suffix; + uint32_t lcyls; + uint32_t lheads; + uint32_t lsecs; +}; + +static QTAILQ_HEAD(, FWLCHSEntry) fw_lchs = + QTAILQ_HEAD_INITIALIZER(fw_lchs); + +void add_boot_device_lchs(DeviceState *dev, const char *suffix, + uint32_t lcyls, uint32_t lheads, uint32_t lsecs) +{ + FWLCHSEntry *node; + + if (!lcyls && !lheads && !lsecs) { + return; + } + + assert(dev != NULL || suffix != NULL); + + node = g_malloc0(sizeof(FWLCHSEntry)); + node->suffix = g_strdup(suffix); + node->dev = dev; + node->lcyls = lcyls; + node->lheads = lheads; + node->lsecs = lsecs; + + QTAILQ_INSERT_TAIL(&fw_lchs, node, link); +} + +void del_boot_device_lchs(DeviceState *dev, const char *suffix) +{ + FWLCHSEntry *i; + + if (dev == NULL) { + return; + } + + QTAILQ_FOREACH(i, &fw_lchs, link) { + if ((!suffix || !g_strcmp0(i->suffix, suffix)) && + i->dev == dev) { + QTAILQ_REMOVE(&fw_lchs, i, link); + g_free(i->suffix); + g_free(i); + + break; + } + } +} diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 44f18eb739..5bc5c79cbc 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -103,6 +103,9 @@ void device_add_bootindex_property(Object *obj, int32_t *bootindex, DeviceState *dev, Error **errp); void restore_boot_order(void *opaque); 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);
/* handler to set the boot_device order for a specific type of MachineClass */ typedef void QEMUBootSetHandler(void *opaque, const char *boot_order,
From: Sam Eiderman shmuel.eiderman@oracle.com
We will need to add LCHS removal logic to scsi-hd's unrealize() in the next commit.
Signed-off-by: Sam Eiderman sameid@google.com 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 --- hw/scsi/scsi-bus.c | 16 ++++++++++++++++ include/hw/scsi/scsi.h | 1 + 2 files changed, 17 insertions(+)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index bccb7cc4c6..359d50d6d0 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -59,6 +59,14 @@ static void scsi_device_realize(SCSIDevice *s, Error **errp) } }
+static void scsi_device_unrealize(SCSIDevice *s, Error **errp) +{ + SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s); + if (sc->unrealize) { + sc->unrealize(s, errp); + } +} + int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf, void *hba_private) { @@ -217,12 +225,20 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp) static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp) { SCSIDevice *dev = SCSI_DEVICE(qdev); + Error *local_err = NULL;
if (dev->vmsentry) { qemu_del_vm_change_state_handler(dev->vmsentry); }
scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE)); + + scsi_device_unrealize(dev, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + blockdev_mark_auto_del(dev->conf.blk); }
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index d77a92361b..332ef602f4 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -59,6 +59,7 @@ struct SCSIRequest { typedef struct SCSIDeviceClass { DeviceClass parent_class; void (*realize)(SCSIDevice *dev, Error **errp); + void (*unrealize)(SCSIDevice *dev, Error **errp); int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf, void *hba_private); SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
On 9/25/19 1:06 PM, Sam Eiderman wrote:
From: Sam Eiderman shmuel.eiderman@oracle.com
We will need to add LCHS removal logic to scsi-hd's unrealize() in the next commit.
Signed-off-by: Sam Eiderman sameid@google.com 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
Reviewed-by: Philippe Mathieu-Daudé philmd@redhat.com
hw/scsi/scsi-bus.c | 16 ++++++++++++++++ include/hw/scsi/scsi.h | 1 + 2 files changed, 17 insertions(+)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index bccb7cc4c6..359d50d6d0 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -59,6 +59,14 @@ static void scsi_device_realize(SCSIDevice *s, Error **errp) } }
+static void scsi_device_unrealize(SCSIDevice *s, Error **errp) +{
- SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
- if (sc->unrealize) {
sc->unrealize(s, errp);- }
+}
int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf, void *hba_private) { @@ -217,12 +225,20 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp) static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp) { SCSIDevice *dev = SCSI_DEVICE(qdev);
Error *local_err = NULL;
if (dev->vmsentry) { qemu_del_vm_change_state_handler(dev->vmsentry); }
scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));
scsi_device_unrealize(dev, &local_err);
if (local_err) {
error_propagate(errp, local_err);return;}
blockdev_mark_auto_del(dev->conf.blk);
}
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index d77a92361b..332ef602f4 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -59,6 +59,7 @@ struct SCSIRequest { typedef struct SCSIDeviceClass { DeviceClass parent_class; void (*realize)(SCSIDevice *dev, Error **errp);
- void (*unrealize)(SCSIDevice *dev, Error **errp); int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf, void *hba_private); SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
From: Sam Eiderman shmuel.eiderman@oracle.com
Relevant devices are: * ide-hd (and ide-cd, ide-drive) * scsi-hd (and scsi-cd, scsi-disk, scsi-block) * virtio-blk-pci
We do not call del_boot_device_lchs() for ide-* since we don't need to - IDE block devices do not support unplugging.
Signed-off-by: Sam Eiderman sameid@google.com 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 --- hw/block/virtio-blk.c | 6 ++++++ hw/ide/qdev.c | 5 +++++ hw/scsi/scsi-disk.c | 12 ++++++++++++ 3 files changed, 23 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 18851601cb..6d8ff34a16 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -1186,6 +1186,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) blk_set_guest_block_size(s->blk, s->conf.conf.logical_block_size);
blk_iostatus_enable(s->blk); + + add_boot_device_lchs(dev, "/disk@0,0", + conf->conf.lcyls, + conf->conf.lheads, + conf->conf.lsecs); }
static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) @@ -1193,6 +1198,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) VirtIODevice *vdev = VIRTIO_DEVICE(dev); VirtIOBlock *s = VIRTIO_BLK(dev);
+ del_boot_device_lchs(dev, "/disk@0,0"); virtio_blk_data_plane_destroy(s->dataplane); s->dataplane = NULL; qemu_del_vm_change_state_handler(s->change); diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 6dd219944f..2ffd387a73 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -220,6 +220,11 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
add_boot_device_path(dev->conf.bootindex, &dev->qdev, dev->unit ? "/disk@1" : "/disk@0"); + + add_boot_device_lchs(&dev->qdev, dev->unit ? "/disk@1" : "/disk@0", + dev->conf.lcyls, + dev->conf.lheads, + dev->conf.lsecs); }
static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name, diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 915641a0f1..d19896fe4d 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -35,6 +35,7 @@ #include "hw/block/block.h" #include "hw/qdev-properties.h" #include "sysemu/dma.h" +#include "sysemu/sysemu.h" #include "qemu/cutils.h" #include "trace.h"
@@ -2402,6 +2403,16 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) blk_set_guest_block_size(s->qdev.conf.blk, s->qdev.blocksize);
blk_iostatus_enable(s->qdev.conf.blk); + + add_boot_device_lchs(&dev->qdev, NULL, + dev->conf.lcyls, + dev->conf.lheads, + dev->conf.lsecs); +} + +static void scsi_unrealize(SCSIDevice *dev, Error **errp) +{ + del_boot_device_lchs(&dev->qdev, NULL); }
static void scsi_hd_realize(SCSIDevice *dev, Error **errp) @@ -3006,6 +3017,7 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data) SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
sc->realize = scsi_hd_realize; + sc->unrealize = scsi_unrealize; sc->alloc_req = scsi_new_request; sc->unit_attention_reported = scsi_disk_unit_attention_reported; dc->desc = "virtual SCSI disk";
From: Sam Eiderman shmuel.eiderman@oracle.com
Move device name construction to a separate function.
We will reuse this function in the following commit to pass logical CHS parameters through fw_cfg much like we currently pass bootindex.
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 | 61 +++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/bootdevice.c b/bootdevice.c index bc5e1c2de4..2b12fb85a4 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -202,6 +202,39 @@ DeviceState *get_boot_device(uint32_t position) return res; }
+static char *get_boot_device_path(DeviceState *dev, bool ignore_suffixes, + char *suffix) +{ + char *devpath = NULL, *s = NULL, *d, *bootpath; + + if (dev) { + devpath = qdev_get_fw_dev_path(dev); + assert(devpath); + } + + if (!ignore_suffixes) { + if (dev) { + d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev); + if (d) { + assert(!suffix); + s = d; + } else { + s = g_strdup(suffix); + } + } else { + s = g_strdup(suffix); + } + } + + bootpath = g_strdup_printf("%s%s", + devpath ? devpath : "", + s ? s : ""); + g_free(devpath); + g_free(s); + + return bootpath; +} + /* * This function returns null terminated string that consist of new line * separated device paths. @@ -218,36 +251,10 @@ char *get_boot_devices_list(size_t *size) bool ignore_suffixes = mc->ignore_boot_device_suffixes;
QTAILQ_FOREACH(i, &fw_boot_order, link) { - char *devpath = NULL, *suffix = NULL; char *bootpath; - char *d; size_t len;
- if (i->dev) { - devpath = qdev_get_fw_dev_path(i->dev); - assert(devpath); - } - - if (!ignore_suffixes) { - if (i->dev) { - d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, - i->dev); - if (d) { - assert(!i->suffix); - suffix = d; - } else { - suffix = g_strdup(i->suffix); - } - } else { - suffix = g_strdup(i->suffix); - } - } - - bootpath = g_strdup_printf("%s%s", - devpath ? devpath : "", - suffix ? suffix : ""); - g_free(devpath); - g_free(suffix); + bootpath = get_boot_device_path(i->dev, ignore_suffixes, i->suffix);
if (total) { list[total-1] = '\n';
On 9/25/19 1:06 PM, Sam Eiderman wrote:
From: Sam Eiderman shmuel.eiderman@oracle.com
Move device name construction to a separate function.
We will reuse this function in the following commit to pass logical CHS parameters through fw_cfg much like we currently pass bootindex.
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 | 61 +++++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/bootdevice.c b/bootdevice.c index bc5e1c2de4..2b12fb85a4 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -202,6 +202,39 @@ DeviceState *get_boot_device(uint32_t position) return res; }
+static char *get_boot_device_path(DeviceState *dev, bool ignore_suffixes,
char *suffix)
Please update to 'const char *suffix'. John, can you do it directly in your tree before sending the pull request? With it: Reviewed-by: Philippe Mathieu-Daudé philmd@redhat.com
+{
- char *devpath = NULL, *s = NULL, *d, *bootpath;
- if (dev) {
devpath = qdev_get_fw_dev_path(dev);assert(devpath);- }
- if (!ignore_suffixes) {
if (dev) {d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);if (d) {assert(!suffix);s = d;} else {s = g_strdup(suffix);}} else {s = g_strdup(suffix);}- }
- bootpath = g_strdup_printf("%s%s",
devpath ? devpath : "",s ? s : "");- g_free(devpath);
- g_free(s);
- return bootpath;
+}
/*
- This function returns null terminated string that consist of new line
- separated device paths.
@@ -218,36 +251,10 @@ char *get_boot_devices_list(size_t *size) bool ignore_suffixes = mc->ignore_boot_device_suffixes;
QTAILQ_FOREACH(i, &fw_boot_order, link) {
char *devpath = NULL, *suffix = NULL; char *bootpath;char *d; size_t len;if (i->dev) {devpath = qdev_get_fw_dev_path(i->dev);assert(devpath);}if (!ignore_suffixes) {if (i->dev) {d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus,i->dev);if (d) {assert(!i->suffix);suffix = d;} else {suffix = g_strdup(i->suffix);}} else {suffix = g_strdup(i->suffix);}}bootpath = g_strdup_printf("%s%s",devpath ? devpath : "",suffix ? suffix : "");g_free(devpath);g_free(suffix);
bootpath = get_boot_device_path(i->dev, ignore_suffixes, i->suffix); if (total) { list[total-1] = '\n';
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,
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 whichdepends 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 headscannot
>>>> 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'sfair 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 systemshmuel.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 systemshmuel.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 systemshmuel.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
From: Sam Eiderman shmuel.eiderman@oracle.com
Add QTest tests to check the logical geometry override option.
The tests in hd-geo-test are out of date - they only test IDE and do not test interesting MBRs.
I added a few helper functions which will make adding more tests easier.
QTest's fw_cfg helper functions support only legacy fw_cfg, so I had to read the new fw_cfg layout on my own.
Creating qcow2 disks with specific size and MBR layout is currently unused - we only use a default empty MBR.
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 --- tests/Makefile.include | 2 +- tests/hd-geo-test.c | 589 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 590 insertions(+), 1 deletion(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include index 479664f899..a5b92fea6a 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -780,7 +780,7 @@ tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y) tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y) qemu-img$(EXESUF) tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o $(libqos-obj-y) tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y) tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y) tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \ diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c index 62eb624726..458de99c31 100644 --- a/tests/hd-geo-test.c +++ b/tests/hd-geo-test.c @@ -17,7 +17,12 @@
#include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/bswap.h" +#include "qapi/qmp/qlist.h" #include "libqtest.h" +#include "libqos/fw_cfg.h" +#include "libqos/libqos.h" +#include "standard-headers/linux/qemu_fw_cfg.h"
#define ARGV_SIZE 256
@@ -388,6 +393,575 @@ static void test_ide_drive_cd_0(void) qtest_quit(qts); }
+typedef struct { + bool active; + uint32_t head; + uint32_t sector; + uint32_t cyl; + uint32_t end_head; + uint32_t end_sector; + uint32_t end_cyl; + uint32_t start_sect; + uint32_t nr_sects; +} MBRpartitions[4]; + +static MBRpartitions empty_mbr = { {false, 0, 0, 0, 0, 0, 0, 0, 0}, + {false, 0, 0, 0, 0, 0, 0, 0, 0}, + {false, 0, 0, 0, 0, 0, 0, 0, 0}, + {false, 0, 0, 0, 0, 0, 0, 0, 0} }; + +static char *create_qcow2_with_mbr(MBRpartitions mbr, uint64_t sectors) +{ + const char *template = "/tmp/qtest.XXXXXX"; + char *raw_path = strdup(template); + char *qcow2_path = strdup(template); + char cmd[100 + 2 * PATH_MAX]; + uint8_t buf[512]; + int i, ret, fd, offset; + uint64_t qcow2_size = sectors * 512; + uint8_t status, parttype, head, sector, cyl; + char *qemu_img_path; + char *qemu_img_abs_path; + + offset = 0xbe; + + for (i = 0; i < 4; i++) { + status = mbr[i].active ? 0x80 : 0x00; + g_assert(mbr[i].head < 256); + g_assert(mbr[i].sector < 64); + g_assert(mbr[i].cyl < 1024); + head = mbr[i].head; + sector = mbr[i].sector + ((mbr[i].cyl & 0x300) >> 2); + cyl = mbr[i].cyl & 0xff; + + buf[offset + 0x0] = status; + buf[offset + 0x1] = head; + buf[offset + 0x2] = sector; + buf[offset + 0x3] = cyl; + + parttype = 0; + g_assert(mbr[i].end_head < 256); + g_assert(mbr[i].end_sector < 64); + g_assert(mbr[i].end_cyl < 1024); + head = mbr[i].end_head; + sector = mbr[i].end_sector + ((mbr[i].end_cyl & 0x300) >> 2); + cyl = mbr[i].end_cyl & 0xff; + + buf[offset + 0x4] = parttype; + buf[offset + 0x5] = head; + buf[offset + 0x6] = sector; + buf[offset + 0x7] = cyl; + + (*(uint32_t *)&buf[offset + 0x8]) = cpu_to_le32(mbr[i].start_sect); + (*(uint32_t *)&buf[offset + 0xc]) = cpu_to_le32(mbr[i].nr_sects); + + offset += 0x10; + } + + fd = mkstemp(raw_path); + g_assert(fd); + close(fd); + + fd = open(raw_path, O_WRONLY); + g_assert(fd >= 0); + ret = write(fd, buf, sizeof(buf)); + g_assert(ret == sizeof(buf)); + close(fd); + + fd = mkstemp(qcow2_path); + g_assert(fd); + close(fd); + + qemu_img_path = getenv("QTEST_QEMU_IMG"); + g_assert(qemu_img_path); + qemu_img_abs_path = realpath(qemu_img_path, NULL); + g_assert(qemu_img_abs_path); + + ret = snprintf(cmd, sizeof(cmd), + "%s convert -f raw -O qcow2 %s %s > /dev/null", + qemu_img_abs_path, + raw_path, qcow2_path); + g_assert((0 < ret) && (ret <= sizeof(cmd))); + ret = system(cmd); + g_assert(ret == 0); + + ret = snprintf(cmd, sizeof(cmd), + "%s resize %s %" PRIu64 " > /dev/null", + qemu_img_abs_path, + qcow2_path, qcow2_size); + g_assert((0 < ret) && (ret <= sizeof(cmd))); + ret = system(cmd); + g_assert(ret == 0); + + free(qemu_img_abs_path); + + unlink(raw_path); + free(raw_path); + + return qcow2_path; +} + +struct QemuCfgFile { + uint32_t size; /* file size */ + uint16_t select; /* write this to 0x510 to read it */ + uint16_t reserved; + char name[56]; +}; + +static uint16_t find_fw_cfg_file(QFWCFG *fw_cfg, + const char *filename) +{ + struct QemuCfgFile qfile; + uint32_t count, e; + uint16_t select; + + count = qfw_cfg_get_u32(fw_cfg, FW_CFG_FILE_DIR); + count = be32_to_cpu(count); + for (select = 0, e = 0; e < count; e++) { + qfw_cfg_read_data(fw_cfg, &qfile, sizeof(qfile)); + if (!strcmp(filename, qfile.name)) { + select = be16_to_cpu(qfile.select); + } + } + + return select; +} + +static void read_fw_cfg_file(QFWCFG *fw_cfg, + const char *filename, + void *data, + size_t len) +{ + uint16_t select = find_fw_cfg_file(fw_cfg, filename); + + g_assert(select); + + qfw_cfg_get(fw_cfg, select, data, len); +} + +#define BIOS_GEOMETRY_MAX_SIZE 10000 + +typedef struct { + uint32_t c; + uint32_t h; + uint32_t s; +} CHS; + +typedef struct { + const char *dev_path; + CHS chs; +} CHSResult; + +static void read_bootdevices(QFWCFG *fw_cfg, CHSResult expected[]) +{ + char *buf = g_malloc0(BIOS_GEOMETRY_MAX_SIZE); + char *cur; + GList *results = NULL, *cur_result; + CHSResult *r; + int i; + int res; + bool found; + + read_fw_cfg_file(fw_cfg, "bios-geometry", buf, BIOS_GEOMETRY_MAX_SIZE); + + for (cur = buf; *cur; cur++) { + if (*cur == '\n') { + *cur = '\0'; + } + } + cur = buf; + + while (strlen(cur)) { + + r = g_malloc0(sizeof(*r)); + r->dev_path = g_malloc0(strlen(cur) + 1); + res = sscanf(cur, "%s %" PRIu32 " %" PRIu32 " %" PRIu32, + (char *)r->dev_path, + &(r->chs.c), &(r->chs.h), &(r->chs.s)); + + g_assert(res == 4); + + results = g_list_prepend(results, r); + + cur += strlen(cur) + 1; + } + + i = 0; + + while (expected[i].dev_path) { + found = false; + cur_result = results; + while (cur_result) { + r = cur_result->data; + if (!strcmp(r->dev_path, expected[i].dev_path) && + !memcmp(&(r->chs), &(expected[i].chs), sizeof(r->chs))) { + found = true; + break; + } + cur_result = g_list_next(cur_result); + } + g_assert(found); + g_free((char *)((CHSResult *)cur_result->data)->dev_path); + g_free(cur_result->data); + results = g_list_delete_link(results, cur_result); + i++; + } + + g_assert(results == NULL); + + g_free(buf); +} + +#define MAX_DRIVES 30 + +typedef struct { + char **argv; + int argc; + char **drives; + int n_drives; + int n_scsi_disks; + int n_scsi_controllers; + int n_virtio_disks; +} TestArgs; + +static TestArgs *create_args(void) +{ + TestArgs *args = g_malloc0(sizeof(*args)); + args->argv = g_new0(char *, ARGV_SIZE); + args->argc = append_arg(args->argc, args->argv, + ARGV_SIZE, g_strdup("-nodefaults")); + args->drives = g_new0(char *, MAX_DRIVES); + return args; +} + +static void add_drive_with_mbr(TestArgs *args, + MBRpartitions mbr, uint64_t sectors) +{ + char *img_file_name; + char part[300]; + int ret; + + g_assert(args->n_drives < MAX_DRIVES); + + img_file_name = create_qcow2_with_mbr(mbr, sectors); + + args->drives[args->n_drives] = img_file_name; + ret = snprintf(part, sizeof(part), + "-drive file=%s,if=none,format=qcow2,id=disk%d", + img_file_name, args->n_drives); + g_assert((0 < ret) && (ret <= sizeof(part))); + args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part)); + args->n_drives++; +} + +static void add_ide_disk(TestArgs *args, + int drive_idx, int bus, int unit, int c, int h, int s) +{ + char part[300]; + int ret; + + ret = snprintf(part, sizeof(part), + "-device ide-hd,drive=disk%d,bus=ide.%d,unit=%d," + "lcyls=%d,lheads=%d,lsecs=%d", + drive_idx, bus, unit, c, h, s); + g_assert((0 < ret) && (ret <= sizeof(part))); + args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part)); +} + +static void add_scsi_controller(TestArgs *args, + const char *type, + const char *bus, + int addr) +{ + char part[300]; + int ret; + + ret = snprintf(part, sizeof(part), + "-device %s,id=scsi%d,bus=%s,addr=%d", + type, args->n_scsi_controllers, bus, addr); + g_assert((0 < ret) && (ret <= sizeof(part))); + args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part)); + args->n_scsi_controllers++; +} + +static void add_scsi_disk(TestArgs *args, + int drive_idx, int bus, + int channel, int scsi_id, int lun, + int c, int h, int s) +{ + char part[300]; + int ret; + + ret = snprintf(part, sizeof(part), + "-device scsi-hd,id=scsi-disk%d,drive=disk%d," + "bus=scsi%d.0," + "channel=%d,scsi-id=%d,lun=%d," + "lcyls=%d,lheads=%d,lsecs=%d", + args->n_scsi_disks, drive_idx, bus, channel, scsi_id, lun, + c, h, s); + g_assert((0 < ret) && (ret <= sizeof(part))); + args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part)); + args->n_scsi_disks++; +} + +static void add_virtio_disk(TestArgs *args, + int drive_idx, const char *bus, int addr, + int c, int h, int s) +{ + char part[300]; + int ret; + + ret = snprintf(part, sizeof(part), + "-device virtio-blk-pci,id=virtio-disk%d," + "drive=disk%d,bus=%s,addr=%d," + "lcyls=%d,lheads=%d,lsecs=%d", + args->n_virtio_disks, drive_idx, bus, addr, c, h, s); + g_assert((0 < ret) && (ret <= sizeof(part))); + args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part)); + args->n_virtio_disks++; +} + +static void test_override(TestArgs *args, CHSResult expected[]) +{ + QTestState *qts; + char *joined_args; + QFWCFG *fw_cfg; + int i; + + joined_args = g_strjoinv(" ", args->argv); + + qts = qtest_init(joined_args); + fw_cfg = pc_fw_cfg_init(qts); + + read_bootdevices(fw_cfg, expected); + + g_free(joined_args); + qtest_quit(qts); + + g_free(fw_cfg); + + for (i = 0; i < args->n_drives; i++) { + unlink(args->drives[i]); + free(args->drives[i]); + } + g_free(args->drives); + g_strfreev(args->argv); + g_free(args); +} + +static void test_override_ide(void) +{ + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/ide@1,1/drive@0/disk@0", {10000, 120, 30} }, + {"/pci@i0cf8/ide@1,1/drive@0/disk@1", {9000, 120, 30} }, + {"/pci@i0cf8/ide@1,1/drive@1/disk@0", {0, 1, 1} }, + {"/pci@i0cf8/ide@1,1/drive@1/disk@1", {1, 0, 0} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_ide_disk(args, 0, 0, 0, 10000, 120, 30); + add_ide_disk(args, 1, 0, 1, 9000, 120, 30); + add_ide_disk(args, 2, 1, 0, 0, 1, 1); + add_ide_disk(args, 3, 1, 1, 1, 0, 0); + test_override(args, expected); +} + +static void test_override_scsi(void) +{ + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/scsi@3/channel@0/disk@0,0", {10000, 120, 30} }, + {"/pci@i0cf8/scsi@3/channel@0/disk@1,0", {9000, 120, 30} }, + {"/pci@i0cf8/scsi@3/channel@0/disk@2,0", {1, 0, 0} }, + {"/pci@i0cf8/scsi@3/channel@0/disk@3,0", {0, 1, 0} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_scsi_controller(args, "lsi53c895a", "pci.0", 3); + add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); + add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); + add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0); + add_scsi_disk(args, 3, 0, 0, 3, 0, 0, 1, 0); + test_override(args, expected); +} + +static void test_override_scsi_2_controllers(void) +{ + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/scsi@3/channel@0/disk@0,0", {10000, 120, 30} }, + {"/pci@i0cf8/scsi@3/channel@0/disk@1,0", {9000, 120, 30} }, + {"/pci@i0cf8/scsi@4/channel@0/disk@0,1", {1, 0, 0} }, + {"/pci@i0cf8/scsi@4/channel@0/disk@1,2", {0, 1, 0} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_scsi_controller(args, "lsi53c895a", "pci.0", 3); + add_scsi_controller(args, "virtio-scsi-pci", "pci.0", 4); + add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); + add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30); + add_scsi_disk(args, 2, 1, 0, 0, 1, 1, 0, 0); + add_scsi_disk(args, 3, 1, 0, 1, 2, 0, 1, 0); + test_override(args, expected); +} + +static void test_override_virtio_blk(void) +{ + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/scsi@3/disk@0,0", {10000, 120, 30} }, + {"/pci@i0cf8/scsi@4/disk@0,0", {9000, 120, 30} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_virtio_disk(args, 0, "pci.0", 3, 10000, 120, 30); + add_virtio_disk(args, 1, "pci.0", 4, 9000, 120, 30); + test_override(args, expected); +} + +static void test_override_zero_chs(void) +{ + TestArgs *args = create_args(); + CHSResult expected[] = { + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_ide_disk(args, 0, 1, 1, 0, 0, 0); + test_override(args, expected); +} + +static void test_override_scsi_hot_unplug(void) +{ + QTestState *qts; + char *joined_args; + QFWCFG *fw_cfg; + QDict *response; + int i; + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/scsi@2/channel@0/disk@0,0", {10000, 120, 30} }, + {"/pci@i0cf8/scsi@2/channel@0/disk@1,0", {20, 20, 20} }, + {NULL, {0, 0, 0} } + }; + CHSResult expected2[] = { + {"/pci@i0cf8/scsi@2/channel@0/disk@1,0", {20, 20, 20} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_scsi_controller(args, "virtio-scsi-pci", "pci.0", 2); + add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30); + add_scsi_disk(args, 1, 0, 0, 1, 0, 20, 20, 20); + + joined_args = g_strjoinv(" ", args->argv); + + qts = qtest_init(joined_args); + fw_cfg = pc_fw_cfg_init(qts); + + read_bootdevices(fw_cfg, expected); + + /* unplug device an restart */ + response = qtest_qmp(qts, + "{ 'execute': 'device_del'," + " 'arguments': {'id': 'scsi-disk0' }}"); + g_assert(response); + g_assert(!qdict_haskey(response, "error")); + qobject_unref(response); + response = qtest_qmp(qts, + "{ 'execute': 'system_reset', 'arguments': { }}"); + g_assert(response); + g_assert(!qdict_haskey(response, "error")); + qobject_unref(response); + + qtest_qmp_eventwait(qts, "RESET"); + + read_bootdevices(fw_cfg, expected2); + + g_free(joined_args); + qtest_quit(qts); + + g_free(fw_cfg); + + for (i = 0; i < args->n_drives; i++) { + unlink(args->drives[i]); + free(args->drives[i]); + } + g_free(args->drives); + g_strfreev(args->argv); + g_free(args); +} + +static void test_override_virtio_hot_unplug(void) +{ + QTestState *qts; + char *joined_args; + QFWCFG *fw_cfg; + QDict *response; + int i; + TestArgs *args = create_args(); + CHSResult expected[] = { + {"/pci@i0cf8/scsi@2/disk@0,0", {10000, 120, 30} }, + {"/pci@i0cf8/scsi@3/disk@0,0", {20, 20, 20} }, + {NULL, {0, 0, 0} } + }; + CHSResult expected2[] = { + {"/pci@i0cf8/scsi@3/disk@0,0", {20, 20, 20} }, + {NULL, {0, 0, 0} } + }; + add_drive_with_mbr(args, empty_mbr, 1); + add_drive_with_mbr(args, empty_mbr, 1); + add_virtio_disk(args, 0, "pci.0", 2, 10000, 120, 30); + add_virtio_disk(args, 1, "pci.0", 3, 20, 20, 20); + + joined_args = g_strjoinv(" ", args->argv); + + qts = qtest_init(joined_args); + fw_cfg = pc_fw_cfg_init(qts); + + read_bootdevices(fw_cfg, expected); + + /* unplug device an restart */ + response = qtest_qmp(qts, + "{ 'execute': 'device_del'," + " 'arguments': {'id': 'virtio-disk0' }}"); + g_assert(response); + g_assert(!qdict_haskey(response, "error")); + qobject_unref(response); + response = qtest_qmp(qts, + "{ 'execute': 'system_reset', 'arguments': { }}"); + g_assert(response); + g_assert(!qdict_haskey(response, "error")); + qobject_unref(response); + + qtest_qmp_eventwait(qts, "RESET"); + + read_bootdevices(fw_cfg, expected2); + + g_free(joined_args); + qtest_quit(qts); + + g_free(fw_cfg); + + for (i = 0; i < args->n_drives; i++) { + unlink(args->drives[i]); + free(args->drives[i]); + } + g_free(args->drives); + g_strfreev(args->argv); + g_free(args); +} + int main(int argc, char **argv) { Backend i; @@ -413,6 +987,21 @@ int main(int argc, char **argv) qtest_add_func("hd-geo/ide/device/mbr/chs", test_ide_device_mbr_chs); qtest_add_func("hd-geo/ide/device/user/chs", test_ide_device_user_chs); qtest_add_func("hd-geo/ide/device/user/chst", test_ide_device_user_chst); + if (have_qemu_img()) { + qtest_add_func("hd-geo/override/ide", test_override_ide); + qtest_add_func("hd-geo/override/scsi", test_override_scsi); + qtest_add_func("hd-geo/override/scsi_2_controllers", + test_override_scsi_2_controllers); + qtest_add_func("hd-geo/override/virtio_blk", test_override_virtio_blk); + qtest_add_func("hd-geo/override/zero_chs", test_override_zero_chs); + qtest_add_func("hd-geo/override/scsi_hot_unplug", + test_override_scsi_hot_unplug); + qtest_add_func("hd-geo/override/virtio_hot_unplug", + test_override_virtio_hot_unplug); + } else { + g_test_message("QTEST_QEMU_IMG not set or qemu-img missing; " + "skipping hd-geo/override/* tests"); + }
ret = g_test_run();
On 9/25/19 1:06 PM, Sam Eiderman wrote:
From: Sam Eiderman shmuel.eiderman@oracle.com
Add QTest tests to check the logical geometry override option.
The tests in hd-geo-test are out of date - they only test IDE and do not test interesting MBRs.
I added a few helper functions which will make adding more tests easier.
QTest's fw_cfg helper functions support only legacy fw_cfg, so I had to read the new fw_cfg layout on my own.
Creating qcow2 disks with specific size and MBR layout is currently unused - we only use a default empty MBR.
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
tests/Makefile.include | 2 +- tests/hd-geo-test.c | 589 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 590 insertions(+), 1 deletion(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include index 479664f899..a5b92fea6a 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -780,7 +780,7 @@ tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y) tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y) qemu-img$(EXESUF) tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o $(libqos-obj-y) tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y) tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y) tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \ diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c index 62eb624726..458de99c31 100644 --- a/tests/hd-geo-test.c +++ b/tests/hd-geo-test.c @@ -17,7 +17,12 @@
#include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/bswap.h" +#include "qapi/qmp/qlist.h" #include "libqtest.h" +#include "libqos/fw_cfg.h" +#include "libqos/libqos.h" +#include "standard-headers/linux/qemu_fw_cfg.h"
#define ARGV_SIZE 256
@@ -388,6 +393,575 @@ static void test_ide_drive_cd_0(void) qtest_quit(qts); }
+typedef struct {
- bool active;
- uint32_t head;
- uint32_t sector;
- uint32_t cyl;
- uint32_t end_head;
- uint32_t end_sector;
- uint32_t end_cyl;
- uint32_t start_sect;
- uint32_t nr_sects;
+} MBRpartitions[4];
+static MBRpartitions empty_mbr = { {false, 0, 0, 0, 0, 0, 0, 0, 0},
{false, 0, 0, 0, 0, 0, 0, 0, 0},{false, 0, 0, 0, 0, 0, 0, 0, 0},{false, 0, 0, 0, 0, 0, 0, 0, 0} };+static char *create_qcow2_with_mbr(MBRpartitions mbr, uint64_t sectors) +{
- const char *template = "/tmp/qtest.XXXXXX";
- char *raw_path = strdup(template);
- char *qcow2_path = strdup(template);
- char cmd[100 + 2 * PATH_MAX];
- uint8_t buf[512];
- int i, ret, fd, offset;
- uint64_t qcow2_size = sectors * 512;
- uint8_t status, parttype, head, sector, cyl;
- char *qemu_img_path;
- char *qemu_img_abs_path;
- offset = 0xbe;
- for (i = 0; i < 4; i++) {
status = mbr[i].active ? 0x80 : 0x00;g_assert(mbr[i].head < 256);g_assert(mbr[i].sector < 64);g_assert(mbr[i].cyl < 1024);head = mbr[i].head;sector = mbr[i].sector + ((mbr[i].cyl & 0x300) >> 2);cyl = mbr[i].cyl & 0xff;buf[offset + 0x0] = status;buf[offset + 0x1] = head;buf[offset + 0x2] = sector;buf[offset + 0x3] = cyl;parttype = 0;g_assert(mbr[i].end_head < 256);g_assert(mbr[i].end_sector < 64);g_assert(mbr[i].end_cyl < 1024);head = mbr[i].end_head;sector = mbr[i].end_sector + ((mbr[i].end_cyl & 0x300) >> 2);cyl = mbr[i].end_cyl & 0xff;buf[offset + 0x4] = parttype;buf[offset + 0x5] = head;buf[offset + 0x6] = sector;buf[offset + 0x7] = cyl;(*(uint32_t *)&buf[offset + 0x8]) = cpu_to_le32(mbr[i].start_sect);(*(uint32_t *)&buf[offset + 0xc]) = cpu_to_le32(mbr[i].nr_sects);offset += 0x10;- }
- fd = mkstemp(raw_path);
- g_assert(fd);
- close(fd);
- fd = open(raw_path, O_WRONLY);
- g_assert(fd >= 0);
- ret = write(fd, buf, sizeof(buf));
- g_assert(ret == sizeof(buf));
- close(fd);
- fd = mkstemp(qcow2_path);
- g_assert(fd);
- close(fd);
- qemu_img_path = getenv("QTEST_QEMU_IMG");
- g_assert(qemu_img_path);
- qemu_img_abs_path = realpath(qemu_img_path, NULL);
- g_assert(qemu_img_abs_path);
- ret = snprintf(cmd, sizeof(cmd),
"%s convert -f raw -O qcow2 %s %s > /dev/null",qemu_img_abs_path,raw_path, qcow2_path);- g_assert((0 < ret) && (ret <= sizeof(cmd)));
- ret = system(cmd);
- g_assert(ret == 0);
- ret = snprintf(cmd, sizeof(cmd),
"%s resize %s %" PRIu64 " > /dev/null",qemu_img_abs_path,qcow2_path, qcow2_size);- g_assert((0 < ret) && (ret <= sizeof(cmd)));
- ret = system(cmd);
- g_assert(ret == 0);
- free(qemu_img_abs_path);
- unlink(raw_path);
- free(raw_path);
- return qcow2_path;
+}
+struct QemuCfgFile {
- uint32_t size; /* file size */
- uint16_t select; /* write this to 0x510 to read it */
- uint16_t reserved;
- char name[56];
+};
+static uint16_t find_fw_cfg_file(QFWCFG *fw_cfg,
const char *filename)+{
- struct QemuCfgFile qfile;
- uint32_t count, e;
- uint16_t select;
- count = qfw_cfg_get_u32(fw_cfg, FW_CFG_FILE_DIR);
- count = be32_to_cpu(count);
- for (select = 0, e = 0; e < count; e++) {
qfw_cfg_read_data(fw_cfg, &qfile, sizeof(qfile));if (!strcmp(filename, qfile.name)) {select = be16_to_cpu(qfile.select);}- }
- return select;
+}
+static void read_fw_cfg_file(QFWCFG *fw_cfg,
const char *filename,void *data,size_t len)+{
- uint16_t select = find_fw_cfg_file(fw_cfg, filename);
- g_assert(select);
- qfw_cfg_get(fw_cfg, select, data, len);
+}
+#define BIOS_GEOMETRY_MAX_SIZE 10000
+typedef struct {
- uint32_t c;
- uint32_t h;
- uint32_t s;
+} CHS;
+typedef struct {
- const char *dev_path;
- CHS chs;
+} CHSResult;
+static void read_bootdevices(QFWCFG *fw_cfg, CHSResult expected[]) +{
- char *buf = g_malloc0(BIOS_GEOMETRY_MAX_SIZE);
- char *cur;
- GList *results = NULL, *cur_result;
- CHSResult *r;
- int i;
- int res;
- bool found;
- read_fw_cfg_file(fw_cfg, "bios-geometry", buf, BIOS_GEOMETRY_MAX_SIZE);
Oh I'm glad to see the test I requested while reviewing the previous patch! I'll have a look at it, but John 589 LoC I doubt I can do it for Friday.
- for (cur = buf; *cur; cur++) {
if (*cur == '\n') {*cur = '\0';}- }
- cur = buf;
- while (strlen(cur)) {
r = g_malloc0(sizeof(*r));r->dev_path = g_malloc0(strlen(cur) + 1);res = sscanf(cur, "%s %" PRIu32 " %" PRIu32 " %" PRIu32,(char *)r->dev_path,&(r->chs.c), &(r->chs.h), &(r->chs.s));g_assert(res == 4);results = g_list_prepend(results, r);cur += strlen(cur) + 1;- }
- i = 0;
- while (expected[i].dev_path) {
found = false;cur_result = results;while (cur_result) {r = cur_result->data;if (!strcmp(r->dev_path, expected[i].dev_path) &&!memcmp(&(r->chs), &(expected[i].chs), sizeof(r->chs))) {found = true;break;}cur_result = g_list_next(cur_result);}g_assert(found);g_free((char *)((CHSResult *)cur_result->data)->dev_path);g_free(cur_result->data);results = g_list_delete_link(results, cur_result);i++;- }
- g_assert(results == NULL);
- g_free(buf);
+}
+#define MAX_DRIVES 30
+typedef struct {
- char **argv;
- int argc;
- char **drives;
- int n_drives;
- int n_scsi_disks;
- int n_scsi_controllers;
- int n_virtio_disks;
+} TestArgs;
+static TestArgs *create_args(void) +{
- TestArgs *args = g_malloc0(sizeof(*args));
- args->argv = g_new0(char *, ARGV_SIZE);
- args->argc = append_arg(args->argc, args->argv,
ARGV_SIZE, g_strdup("-nodefaults"));- args->drives = g_new0(char *, MAX_DRIVES);
- return args;
+}
+static void add_drive_with_mbr(TestArgs *args,
MBRpartitions mbr, uint64_t sectors)+{
- char *img_file_name;
- char part[300];
- int ret;
- g_assert(args->n_drives < MAX_DRIVES);
- img_file_name = create_qcow2_with_mbr(mbr, sectors);
- args->drives[args->n_drives] = img_file_name;
- ret = snprintf(part, sizeof(part),
"-drive file=%s,if=none,format=qcow2,id=disk%d",img_file_name, args->n_drives);- g_assert((0 < ret) && (ret <= sizeof(part)));
- args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part));
- args->n_drives++;
+}
+static void add_ide_disk(TestArgs *args,
int drive_idx, int bus, int unit, int c, int h, int s)+{
- char part[300];
- int ret;
- ret = snprintf(part, sizeof(part),
"-device ide-hd,drive=disk%d,bus=ide.%d,unit=%d,""lcyls=%d,lheads=%d,lsecs=%d",drive_idx, bus, unit, c, h, s);- g_assert((0 < ret) && (ret <= sizeof(part)));
- args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part));
+}
+static void add_scsi_controller(TestArgs *args,
const char *type,const char *bus,int addr)+{
- char part[300];
- int ret;
- ret = snprintf(part, sizeof(part),
"-device %s,id=scsi%d,bus=%s,addr=%d",type, args->n_scsi_controllers, bus, addr);- g_assert((0 < ret) && (ret <= sizeof(part)));
- args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part));
- args->n_scsi_controllers++;
+}
+static void add_scsi_disk(TestArgs *args,
int drive_idx, int bus,int channel, int scsi_id, int lun,int c, int h, int s)+{
- char part[300];
- int ret;
- ret = snprintf(part, sizeof(part),
"-device scsi-hd,id=scsi-disk%d,drive=disk%d,""bus=scsi%d.0,""channel=%d,scsi-id=%d,lun=%d,""lcyls=%d,lheads=%d,lsecs=%d",args->n_scsi_disks, drive_idx, bus, channel, scsi_id, lun,c, h, s);- g_assert((0 < ret) && (ret <= sizeof(part)));
- args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part));
- args->n_scsi_disks++;
+}
+static void add_virtio_disk(TestArgs *args,
int drive_idx, const char *bus, int addr,int c, int h, int s)+{
- char part[300];
- int ret;
- ret = snprintf(part, sizeof(part),
"-device virtio-blk-pci,id=virtio-disk%d,""drive=disk%d,bus=%s,addr=%d,""lcyls=%d,lheads=%d,lsecs=%d",args->n_virtio_disks, drive_idx, bus, addr, c, h, s);- g_assert((0 < ret) && (ret <= sizeof(part)));
- args->argc = append_arg(args->argc, args->argv, ARGV_SIZE, g_strdup(part));
- args->n_virtio_disks++;
+}
+static void test_override(TestArgs *args, CHSResult expected[]) +{
- QTestState *qts;
- char *joined_args;
- QFWCFG *fw_cfg;
- int i;
- joined_args = g_strjoinv(" ", args->argv);
- qts = qtest_init(joined_args);
- fw_cfg = pc_fw_cfg_init(qts);
- read_bootdevices(fw_cfg, expected);
- g_free(joined_args);
- qtest_quit(qts);
- g_free(fw_cfg);
- for (i = 0; i < args->n_drives; i++) {
unlink(args->drives[i]);free(args->drives[i]);- }
- g_free(args->drives);
- g_strfreev(args->argv);
- g_free(args);
+}
+static void test_override_ide(void) +{
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/ide@1,1/drive@0/disk@0", {10000, 120, 30} },{"/pci@i0cf8/ide@1,1/drive@0/disk@1", {9000, 120, 30} },{"/pci@i0cf8/ide@1,1/drive@1/disk@0", {0, 1, 1} },{"/pci@i0cf8/ide@1,1/drive@1/disk@1", {1, 0, 0} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_ide_disk(args, 0, 0, 0, 10000, 120, 30);
- add_ide_disk(args, 1, 0, 1, 9000, 120, 30);
- add_ide_disk(args, 2, 1, 0, 0, 1, 1);
- add_ide_disk(args, 3, 1, 1, 1, 0, 0);
- test_override(args, expected);
+}
+static void test_override_scsi(void) +{
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/scsi@3/channel@0/disk@0,0", {10000, 120, 30} },{"/pci@i0cf8/scsi@3/channel@0/disk@1,0", {9000, 120, 30} },{"/pci@i0cf8/scsi@3/channel@0/disk@2,0", {1, 0, 0} },{"/pci@i0cf8/scsi@3/channel@0/disk@3,0", {0, 1, 0} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_scsi_controller(args, "lsi53c895a", "pci.0", 3);
- add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
- add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
- add_scsi_disk(args, 2, 0, 0, 2, 0, 1, 0, 0);
- add_scsi_disk(args, 3, 0, 0, 3, 0, 0, 1, 0);
- test_override(args, expected);
+}
+static void test_override_scsi_2_controllers(void) +{
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/scsi@3/channel@0/disk@0,0", {10000, 120, 30} },{"/pci@i0cf8/scsi@3/channel@0/disk@1,0", {9000, 120, 30} },{"/pci@i0cf8/scsi@4/channel@0/disk@0,1", {1, 0, 0} },{"/pci@i0cf8/scsi@4/channel@0/disk@1,2", {0, 1, 0} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_scsi_controller(args, "lsi53c895a", "pci.0", 3);
- add_scsi_controller(args, "virtio-scsi-pci", "pci.0", 4);
- add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
- add_scsi_disk(args, 1, 0, 0, 1, 0, 9000, 120, 30);
- add_scsi_disk(args, 2, 1, 0, 0, 1, 1, 0, 0);
- add_scsi_disk(args, 3, 1, 0, 1, 2, 0, 1, 0);
- test_override(args, expected);
+}
+static void test_override_virtio_blk(void) +{
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/scsi@3/disk@0,0", {10000, 120, 30} },{"/pci@i0cf8/scsi@4/disk@0,0", {9000, 120, 30} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_virtio_disk(args, 0, "pci.0", 3, 10000, 120, 30);
- add_virtio_disk(args, 1, "pci.0", 4, 9000, 120, 30);
- test_override(args, expected);
+}
+static void test_override_zero_chs(void) +{
- TestArgs *args = create_args();
- CHSResult expected[] = {
{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_ide_disk(args, 0, 1, 1, 0, 0, 0);
- test_override(args, expected);
+}
+static void test_override_scsi_hot_unplug(void) +{
- QTestState *qts;
- char *joined_args;
- QFWCFG *fw_cfg;
- QDict *response;
- int i;
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/scsi@2/channel@0/disk@0,0", {10000, 120, 30} },{"/pci@i0cf8/scsi@2/channel@0/disk@1,0", {20, 20, 20} },{NULL, {0, 0, 0} }- };
- CHSResult expected2[] = {
{"/pci@i0cf8/scsi@2/channel@0/disk@1,0", {20, 20, 20} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_scsi_controller(args, "virtio-scsi-pci", "pci.0", 2);
- add_scsi_disk(args, 0, 0, 0, 0, 0, 10000, 120, 30);
- add_scsi_disk(args, 1, 0, 0, 1, 0, 20, 20, 20);
- joined_args = g_strjoinv(" ", args->argv);
- qts = qtest_init(joined_args);
- fw_cfg = pc_fw_cfg_init(qts);
- read_bootdevices(fw_cfg, expected);
- /* unplug device an restart */
- response = qtest_qmp(qts,
"{ 'execute': 'device_del',"" 'arguments': {'id': 'scsi-disk0' }}");- g_assert(response);
- g_assert(!qdict_haskey(response, "error"));
- qobject_unref(response);
- response = qtest_qmp(qts,
"{ 'execute': 'system_reset', 'arguments': { }}");- g_assert(response);
- g_assert(!qdict_haskey(response, "error"));
- qobject_unref(response);
- qtest_qmp_eventwait(qts, "RESET");
- read_bootdevices(fw_cfg, expected2);
- g_free(joined_args);
- qtest_quit(qts);
- g_free(fw_cfg);
- for (i = 0; i < args->n_drives; i++) {
unlink(args->drives[i]);free(args->drives[i]);- }
- g_free(args->drives);
- g_strfreev(args->argv);
- g_free(args);
+}
+static void test_override_virtio_hot_unplug(void) +{
- QTestState *qts;
- char *joined_args;
- QFWCFG *fw_cfg;
- QDict *response;
- int i;
- TestArgs *args = create_args();
- CHSResult expected[] = {
{"/pci@i0cf8/scsi@2/disk@0,0", {10000, 120, 30} },{"/pci@i0cf8/scsi@3/disk@0,0", {20, 20, 20} },{NULL, {0, 0, 0} }- };
- CHSResult expected2[] = {
{"/pci@i0cf8/scsi@3/disk@0,0", {20, 20, 20} },{NULL, {0, 0, 0} }- };
- add_drive_with_mbr(args, empty_mbr, 1);
- add_drive_with_mbr(args, empty_mbr, 1);
- add_virtio_disk(args, 0, "pci.0", 2, 10000, 120, 30);
- add_virtio_disk(args, 1, "pci.0", 3, 20, 20, 20);
- joined_args = g_strjoinv(" ", args->argv);
- qts = qtest_init(joined_args);
- fw_cfg = pc_fw_cfg_init(qts);
- read_bootdevices(fw_cfg, expected);
- /* unplug device an restart */
- response = qtest_qmp(qts,
"{ 'execute': 'device_del',"" 'arguments': {'id': 'virtio-disk0' }}");- g_assert(response);
- g_assert(!qdict_haskey(response, "error"));
- qobject_unref(response);
- response = qtest_qmp(qts,
"{ 'execute': 'system_reset', 'arguments': { }}");- g_assert(response);
- g_assert(!qdict_haskey(response, "error"));
- qobject_unref(response);
- qtest_qmp_eventwait(qts, "RESET");
- read_bootdevices(fw_cfg, expected2);
- g_free(joined_args);
- qtest_quit(qts);
- g_free(fw_cfg);
- for (i = 0; i < args->n_drives; i++) {
unlink(args->drives[i]);free(args->drives[i]);- }
- g_free(args->drives);
- g_strfreev(args->argv);
- g_free(args);
+}
int main(int argc, char **argv) { Backend i; @@ -413,6 +987,21 @@ int main(int argc, char **argv) qtest_add_func("hd-geo/ide/device/mbr/chs", test_ide_device_mbr_chs); qtest_add_func("hd-geo/ide/device/user/chs", test_ide_device_user_chs); qtest_add_func("hd-geo/ide/device/user/chst", test_ide_device_user_chst);
if (have_qemu_img()) {
qtest_add_func("hd-geo/override/ide", test_override_ide);qtest_add_func("hd-geo/override/scsi", test_override_scsi);qtest_add_func("hd-geo/override/scsi_2_controllers",test_override_scsi_2_controllers);qtest_add_func("hd-geo/override/virtio_blk", test_override_virtio_blk);qtest_add_func("hd-geo/override/zero_chs", test_override_zero_chs);qtest_add_func("hd-geo/override/scsi_hot_unplug",test_override_scsi_hot_unplug);qtest_add_func("hd-geo/override/virtio_hot_unplug",test_override_virtio_hot_unplug);} else {
g_test_message("QTEST_QEMU_IMG not set or qemu-img missing; ""skipping hd-geo/override/* tests");}
ret = g_test_run();
On 9/26/19 6:00 AM, Philippe Mathieu-Daudé wrote:
On 9/25/19 1:06 PM, Sam Eiderman wrote:
From: Sam Eiderman shmuel.eiderman@oracle.com
Add QTest tests to check the logical geometry override option.
The tests in hd-geo-test are out of date - they only test IDE and do not test interesting MBRs.
I added a few helper functions which will make adding more tests easier.
QTest's fw_cfg helper functions support only legacy fw_cfg, so I had to read the new fw_cfg layout on my own.
Creating qcow2 disks with specific size and MBR layout is currently unused - we only use a default empty MBR.
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
tests/Makefile.include | 2 +- tests/hd-geo-test.c | 589 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 590 insertions(+), 1 deletion(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include index 479664f899..a5b92fea6a 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -780,7 +780,7 @@ tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y) tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y) qemu-img$(EXESUF) tests/ipmi-kcs-test$(EXESUF): tests/ipmi-kcs-test.o tests/ipmi-bt-test$(EXESUF): tests/ipmi-bt-test.o -tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o +tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o $(libqos-obj-y) tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y) tests/boot-serial-test$(EXESUF): tests/boot-serial-test.o $(libqos-obj-y) tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \ diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c index 62eb624726..458de99c31 100644 --- a/tests/hd-geo-test.c +++ b/tests/hd-geo-test.c @@ -17,7 +17,12 @@
#include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/bswap.h" +#include "qapi/qmp/qlist.h" #include "libqtest.h" +#include "libqos/fw_cfg.h" +#include "libqos/libqos.h" +#include "standard-headers/linux/qemu_fw_cfg.h"
#define ARGV_SIZE 256
@@ -388,6 +393,575 @@ static void test_ide_drive_cd_0(void) qtest_quit(qts); }
+typedef struct {
- bool active;
- uint32_t head;
- uint32_t sector;
- uint32_t cyl;
- uint32_t end_head;
- uint32_t end_sector;
- uint32_t end_cyl;
- uint32_t start_sect;
- uint32_t nr_sects;
+} MBRpartitions[4];
+static MBRpartitions empty_mbr = { {false, 0, 0, 0, 0, 0, 0, 0, 0},
{false, 0, 0, 0, 0, 0, 0, 0, 0},{false, 0, 0, 0, 0, 0, 0, 0, 0},{false, 0, 0, 0, 0, 0, 0, 0, 0} };+static char *create_qcow2_with_mbr(MBRpartitions mbr, uint64_t sectors) +{
- const char *template = "/tmp/qtest.XXXXXX";
- char *raw_path = strdup(template);
- char *qcow2_path = strdup(template);
- char cmd[100 + 2 * PATH_MAX];
- uint8_t buf[512];
- int i, ret, fd, offset;
- uint64_t qcow2_size = sectors * 512;
- uint8_t status, parttype, head, sector, cyl;
- char *qemu_img_path;
- char *qemu_img_abs_path;
- offset = 0xbe;
- for (i = 0; i < 4; i++) {
status = mbr[i].active ? 0x80 : 0x00;g_assert(mbr[i].head < 256);g_assert(mbr[i].sector < 64);g_assert(mbr[i].cyl < 1024);head = mbr[i].head;sector = mbr[i].sector + ((mbr[i].cyl & 0x300) >> 2);cyl = mbr[i].cyl & 0xff;buf[offset + 0x0] = status;buf[offset + 0x1] = head;buf[offset + 0x2] = sector;buf[offset + 0x3] = cyl;parttype = 0;g_assert(mbr[i].end_head < 256);g_assert(mbr[i].end_sector < 64);g_assert(mbr[i].end_cyl < 1024);head = mbr[i].end_head;sector = mbr[i].end_sector + ((mbr[i].end_cyl & 0x300) >> 2);cyl = mbr[i].end_cyl & 0xff;buf[offset + 0x4] = parttype;buf[offset + 0x5] = head;buf[offset + 0x6] = sector;buf[offset + 0x7] = cyl;(*(uint32_t *)&buf[offset + 0x8]) = cpu_to_le32(mbr[i].start_sect);(*(uint32_t *)&buf[offset + 0xc]) = cpu_to_le32(mbr[i].nr_sects);offset += 0x10;- }
- fd = mkstemp(raw_path);
- g_assert(fd);
- close(fd);
- fd = open(raw_path, O_WRONLY);
- g_assert(fd >= 0);
- ret = write(fd, buf, sizeof(buf));
- g_assert(ret == sizeof(buf));
- close(fd);
- fd = mkstemp(qcow2_path);
- g_assert(fd);
- close(fd);
- qemu_img_path = getenv("QTEST_QEMU_IMG");
- g_assert(qemu_img_path);
- qemu_img_abs_path = realpath(qemu_img_path, NULL);
- g_assert(qemu_img_abs_path);
- ret = snprintf(cmd, sizeof(cmd),
"%s convert -f raw -O qcow2 %s %s > /dev/null",qemu_img_abs_path,raw_path, qcow2_path);- g_assert((0 < ret) && (ret <= sizeof(cmd)));
- ret = system(cmd);
- g_assert(ret == 0);
- ret = snprintf(cmd, sizeof(cmd),
"%s resize %s %" PRIu64 " > /dev/null",qemu_img_abs_path,qcow2_path, qcow2_size);- g_assert((0 < ret) && (ret <= sizeof(cmd)));
- ret = system(cmd);
- g_assert(ret == 0);
- free(qemu_img_abs_path);
- unlink(raw_path);
- free(raw_path);
- return qcow2_path;
+}
+struct QemuCfgFile {
- uint32_t size; /* file size */
- uint16_t select; /* write this to 0x510 to read it */
- uint16_t reserved;
- char name[56];
+};
+static uint16_t find_fw_cfg_file(QFWCFG *fw_cfg,
const char *filename)+{
- struct QemuCfgFile qfile;
- uint32_t count, e;
- uint16_t select;
- count = qfw_cfg_get_u32(fw_cfg, FW_CFG_FILE_DIR);
- count = be32_to_cpu(count);
- for (select = 0, e = 0; e < count; e++) {
qfw_cfg_read_data(fw_cfg, &qfile, sizeof(qfile));if (!strcmp(filename, qfile.name)) {select = be16_to_cpu(qfile.select);}- }
- return select;
+}
+static void read_fw_cfg_file(QFWCFG *fw_cfg,
const char *filename,void *data,size_t len)+{
- uint16_t select = find_fw_cfg_file(fw_cfg, filename);
- g_assert(select);
- qfw_cfg_get(fw_cfg, select, data, len);
+}
+#define BIOS_GEOMETRY_MAX_SIZE 10000
+typedef struct {
- uint32_t c;
- uint32_t h;
- uint32_t s;
+} CHS;
+typedef struct {
- const char *dev_path;
- CHS chs;
+} CHSResult;
+static void read_bootdevices(QFWCFG *fw_cfg, CHSResult expected[]) +{
- char *buf = g_malloc0(BIOS_GEOMETRY_MAX_SIZE);
- char *cur;
- GList *results = NULL, *cur_result;
- CHSResult *r;
- int i;
- int res;
- bool found;
- read_fw_cfg_file(fw_cfg, "bios-geometry", buf, BIOS_GEOMETRY_MAX_SIZE);
Oh I'm glad to see the test I requested while reviewing the previous patch! I'll have a look at it, but John 589 LoC I doubt I can do it for Friday.
It's just a test, even so -- we can amend it. There's no real hurry.
--js
On 9/25/19 7:06 AM, Sam Eiderman via wrote:
v1:
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 guess - SeaBIOS, for large enough disks - will use LBA translation, which will report 63 SPT instead.
In addition we can not enforce SeaBIOS to rely on phyiscal geometries at all. A virtio-blk-pci virtual disk with 255 phyiscal heads can not report more than 16 physical heads when moved to an IDE controller, the ATA spec allows a maximum of 16 heads - this is an artifact of virtualization.
By supplying the logical geometies directly we are able to support such "exotic" disks.
We will use fw_cfg to do just that.
v2:
Fix missing parenthesis check in "hd-geo-test: Add tests for lchs override"
v3:
- Rename fw_cfg key to "bios-geometry".
- Remove "extendible" interface.
- Add cpu_to_le32 fix as Laszlo suggested or big endian hosts
- Fix last qtest commit - automatic docker tester for some reason does not have qemu-img set
v4:
- Change fw_cfg interface from mixed textual/binary to textual only
v5:
- Fix line > 80 chars in tests/hd-geo-test.c
v6:
- Small fixes for issues pointed by Max
- (&conf->conf)->lcyls to conf->conf.lcyls and so on
- Remove scsi_unrealize from everything other than scsi-hd
- Add proper include to sysemu.h
- scsi_device_unrealize() after scsi_device_purge_requests()
v7:
- Adapted last commit (tests) to changes in qtest
Sam Eiderman (8): block: Refactor macros - fix tabbing block: Support providing LCHS from user bootdevice: Add interface to gather LCHS scsi: Propagate unrealize() callback to scsi-hd bootdevice: Gather LCHS from all relevant devices bootdevice: Refactor get_boot_devices_list bootdevice: FW_CFG interface for LCHS values hd-geo-test: Add tests for lchs override
bootdevice.c | 148 ++++++++-- hw/block/virtio-blk.c | 6 + hw/ide/qdev.c | 7 +- hw/nvram/fw_cfg.c | 14 +- hw/scsi/scsi-bus.c | 16 ++ hw/scsi/scsi-disk.c | 12 + include/hw/block/block.h | 22 +- include/hw/scsi/scsi.h | 1 + include/sysemu/sysemu.h | 4 + tests/Makefile.include | 2 +- tests/hd-geo-test.c | 589 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 780 insertions(+), 41 deletions(-)
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git
--js
Is that the right tree? Nope, but time's marching on without us. If any other maintainer has an objection, you have until Friday before I send the PR!
-
John Snow -
Laszlo Ersek -
Philippe Mathieu-Daudé -
Sam Eiderman