* [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface @ 2019-09-25 11:06 Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 1/8] block: Refactor macros - fix tabbing Sam Eiderman via ` (9 more replies) 0 siblings, 10 replies; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, sameid, karl.heubaum 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(-) -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 1/8] block: Refactor macros - fix tabbing 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-26 9:38 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 2/8] block: Support providing LCHS from user Sam Eiderman via ` (8 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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) \ -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 1/8] block: Refactor macros - fix tabbing 2019-09-25 11:06 ` [PATCH v7 1/8] block: Refactor macros - fix tabbing Sam Eiderman via @ 2019-09-26 9:38 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 9:38 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, karl.heubaum 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_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) \ > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 2/8] block: Support providing LCHS from user 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 1/8] block: Refactor macros - fix tabbing Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 3/8] bootdevice: Add interface to gather LCHS Sam Eiderman via ` (7 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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, \ -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v7 3/8] bootdevice: Add interface to gather LCHS 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 1/8] block: Refactor macros - fix tabbing Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 2/8] block: Support providing LCHS from user Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd Sam Eiderman via ` (6 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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, -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (2 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 3/8] bootdevice: Add interface to gather LCHS Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-26 9:38 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 5/8] bootdevice: Gather LCHS from all relevant devices Sam Eiderman via ` (5 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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, -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd 2019-09-25 11:06 ` [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd Sam Eiderman via @ 2019-09-26 9:38 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 9:38 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, karl.heubaum 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, > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 5/8] bootdevice: Gather LCHS from all relevant devices 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (3 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list Sam Eiderman via ` (4 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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"; -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (4 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 5/8] bootdevice: Gather LCHS from all relevant devices Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-26 9:42 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values Sam Eiderman via ` (3 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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'; -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list 2019-09-25 11:06 ` [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list Sam Eiderman via @ 2019-09-26 9:42 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 9:42 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, karl.heubaum 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'; > ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (5 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-26 9:57 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 8/8] hd-geo-test: Add tests for lchs override Sam Eiderman via ` (2 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum From: Sam Eiderman <shmuel.eiderman@oracle.com> Using fw_cfg, supply logical CHS values directly from QEMU to the BIOS. Non-standard logical geometries break under QEMU. A virtual disk which contains an operating system which depends on logical geometries (consistent values being reported from BIOS INT13 AH=08) will most likely break under QEMU/SeaBIOS if it has non-standard logical geometries - for example 56 SPT (sectors per track). No matter what QEMU will report - SeaBIOS, for large enough disks - will use LBA translation, which will report 63 SPT instead. In addition we cannot force SeaBIOS to rely on physical geometries at all. A virtio-blk-pci virtual disk with 255 phyiscal heads cannot report more than 16 physical heads when moved to an IDE controller, since the ATA spec allows a maximum of 16 heads - this is an artifact of virtualization. By supplying the logical geometries directly we are able to support such "exotic" disks. We serialize this information in a similar way to the "bootorder" interface. The new fw_cfg entry is "bios-geometry". Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> Reviewed-by: Arbel Moshe <arbel.moshe@oracle.com> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com> --- bootdevice.c | 32 ++++++++++++++++++++++++++++++++ hw/nvram/fw_cfg.c | 14 +++++++++++--- include/sysemu/sysemu.h | 1 + 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/bootdevice.c b/bootdevice.c index 2b12fb85a4..b034ad7bdc 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -405,3 +405,35 @@ void del_boot_device_lchs(DeviceState *dev, const char *suffix) } } } + +/* Serialized as: (device name\0 + lchs struct) x devices */ +char *get_boot_devices_lchs_list(size_t *size) +{ + FWLCHSEntry *i; + size_t total = 0; + char *list = NULL; + + QTAILQ_FOREACH(i, &fw_lchs, link) { + char *bootpath; + char *chs_string; + size_t len; + + bootpath = get_boot_device_path(i->dev, false, i->suffix); + chs_string = g_strdup_printf("%s %" PRIu32 " %" PRIu32 " %" PRIu32, + bootpath, i->lcyls, i->lheads, i->lsecs); + + if (total) { + list[total - 1] = '\n'; + } + len = strlen(chs_string) + 1; + list = g_realloc(list, total + len); + memcpy(&list[total], chs_string, len); + total += len; + g_free(chs_string); + g_free(bootpath); + } + + *size = total; + + return list; +} diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 7dc3ac378e..18aff658c0 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -920,13 +920,21 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, static void fw_cfg_machine_reset(void *opaque) { + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); + FWCfgState *s = opaque; void *ptr; size_t len; - FWCfgState *s = opaque; - char *bootindex = get_boot_devices_list(&len); + char *buf; - ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len); + buf = get_boot_devices_list(&len); + ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)buf, len); g_free(ptr); + + if (!mc->legacy_fw_cfg_order) { + buf = get_boot_devices_lchs_list(&len); + ptr = fw_cfg_modify_file(s, "bios-geometry", (uint8_t *)buf, len); + g_free(ptr); + } } static void fw_cfg_machine_ready(struct Notifier *n, void *data) diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 5bc5c79cbc..80c57fdc4e 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -106,6 +106,7 @@ void validate_bootdevices(const char *devices, Error **errp); void add_boot_device_lchs(DeviceState *dev, const char *suffix, uint32_t lcyls, uint32_t lheads, uint32_t lsecs); void del_boot_device_lchs(DeviceState *dev, const char *suffix); +char *get_boot_devices_lchs_list(size_t *size); /* handler to set the boot_device order for a specific type of MachineClass */ typedef void QEMUBootSetHandler(void *opaque, const char *boot_order, -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-25 11:06 ` [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values Sam Eiderman via @ 2019-09-26 9:57 ` Philippe Mathieu-Daudé 2019-09-26 18:26 ` John Snow 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 9:57 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek, karl.heubaum 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, > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-26 9:57 ` [SeaBIOS] " Philippe Mathieu-Daudé @ 2019-09-26 18:26 ` John Snow 2019-09-26 19:09 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 30+ messages in thread From: John Snow @ 2019-09-26 18:26 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-26 18:26 ` John Snow @ 2019-09-26 19:09 ` Philippe Mathieu-Daudé 2019-09-26 19:16 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 19:09 UTC (permalink / raw) To: John Snow, Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek 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 :) :) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-26 19:09 ` Philippe Mathieu-Daudé @ 2019-09-26 19:16 ` Philippe Mathieu-Daudé 2019-09-29 10:13 ` Sam Eiderman 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 19:16 UTC (permalink / raw) To: John Snow, Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek 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> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-26 19:16 ` Philippe Mathieu-Daudé @ 2019-09-29 10:13 ` Sam Eiderman 2019-10-08 11:34 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman @ 2019-09-29 10:13 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: John Snow, qemu-devel, kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 6941 bytes --] 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> > [-- Attachment #2: Type: text/html, Size: 9972 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-09-29 10:13 ` Sam Eiderman @ 2019-10-08 11:34 ` Philippe Mathieu-Daudé 2019-10-08 11:51 ` Sam Eiderman 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-10-08 11:34 UTC (permalink / raw) To: Sam Eiderman Cc: kwolf, qemu-block, arbel.moshe, Laszlo Ersek, seabios, qemu-devel, kraxel, John Snow 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-08 11:34 ` Philippe Mathieu-Daudé @ 2019-10-08 11:51 ` Sam Eiderman 2019-10-16 11:02 ` Sam Eiderman 0 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman @ 2019-10-08 11:51 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: John Snow, qemu-devel, kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 9520 bytes --] 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. > [-- Attachment #2: Type: text/html, Size: 15293 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-08 11:51 ` Sam Eiderman @ 2019-10-16 11:02 ` Sam Eiderman 2019-10-16 12:14 ` [SeaBIOS] " Laszlo Ersek 0 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman @ 2019-10-16 11:02 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: John Snow, qemu-devel, kwolf, qemu-block, arbel.moshe, seabios, kraxel, Laszlo Ersek 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 11:02 ` Sam Eiderman @ 2019-10-16 12:14 ` Laszlo Ersek 2019-10-16 12:29 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 30+ messages in thread From: Laszlo Ersek @ 2019-10-16 12:14 UTC (permalink / raw) To: Sam Eiderman, Philippe Mathieu-Daudé Cc: kwolf, qemu-block, arbel.moshe, seabios, qemu-devel, kraxel, John Snow Hi Sam, On 10/16/19 13:02, Sam Eiderman wrote: > Gentle Ping, > > Philippe, John? > > Just wondering if the series is okay, as Gerd pointed out this series > is a blocker for the corresponding changes in SeaBIOS for v 1.13 The QEMU series is still not merged, due to a bug in the last patch (namely, the test case, "hd-geo-test: Add tests for lchs override"). To my knowledge, SeaBIOS prefers to merge patches with the underlying QEMU patches merged first, so you'll likely have to fix that QEMU issue first. I explained the bug in the QEMU test case here: http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com (Alternative links to the same: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html ) I've never received feedback to those messages, and I think you must have missed them. FWIW, when I hit "Reply All" in that thread, you were on the "To:" list with: Sam Eiderman <shmuel.eiderman@oracle.com> but here you are present with Sam Eiderman <sameid@google.com> In addition, when I posted those messages, I got the following auto-response ("Undelivered Mail Returned to Sender"): > This is the mail system at host mx1.redhat.com. > > I'm sorry to have to inform you that your message could not > be delivered to one or more recipients. It's attached below. > > For further assistance, please send mail to postmaster. > > If you do so, please include this problem report. You can > delete your own text from the attached returned message. > > The mail system > > <shmuel.eiderman@oracle.com>: host > aserp2030.oracle.com[141.146.126.74] said: > 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command) I didn't know your new address, so I could only hope you'd find my feedback on qemu-devel. Thanks Laszlo ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 12:14 ` [SeaBIOS] " Laszlo Ersek @ 2019-10-16 12:29 ` Philippe Mathieu-Daudé 2019-10-16 14:55 ` Sam Eiderman 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-10-16 12:29 UTC (permalink / raw) To: Laszlo Ersek, Sam Eiderman Cc: kwolf, qemu-block, arbel.moshe, seabios, qemu-devel, kraxel, John Snow On 10/16/19 2:14 PM, Laszlo Ersek wrote: > Hi Sam, > > On 10/16/19 13:02, Sam Eiderman wrote: >> Gentle Ping, >> >> Philippe, John? >> >> Just wondering if the series is okay, as Gerd pointed out this series >> is a blocker for the corresponding changes in SeaBIOS for v 1.13 > > The QEMU series is still not merged, due to a bug in the last patch > (namely, the test case, "hd-geo-test: Add tests for lchs override"). > > To my knowledge, SeaBIOS prefers to merge patches with the underlying > QEMU patches merged first, so you'll likely have to fix that QEMU issue > first. > > I explained the bug in the QEMU test case here: > > http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com > http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com Yes, I was expecting a respin with find_fw_cfg_file() fixed per Laszlo detailed review. > (Alternative links to the same: > > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html > ) > > I've never received feedback to those messages, and I think you must > have missed them. > > FWIW, when I hit "Reply All" in that thread, you were on the "To:" list > with: > > Sam Eiderman <shmuel.eiderman@oracle.com> > > but here you are present with > > Sam Eiderman <sameid@google.com> > > In addition, when I posted those messages, I got the following > auto-response ("Undelivered Mail Returned to Sender"): > >> This is the mail system at host mx1.redhat.com. >> >> I'm sorry to have to inform you that your message could not >> be delivered to one or more recipients. It's attached below. >> >> For further assistance, please send mail to postmaster. >> >> If you do so, please include this problem report. You can >> delete your own text from the attached returned message. >> >> The mail system >> >> <shmuel.eiderman@oracle.com>: host >> aserp2030.oracle.com[141.146.126.74] said: >> 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command) That explains it :) > > I didn't know your new address, so I could only hope you'd find my > feedback on qemu-devel. > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 12:29 ` Philippe Mathieu-Daudé @ 2019-10-16 14:55 ` Sam Eiderman 2019-10-16 15:07 ` John Snow 0 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman @ 2019-10-16 14:55 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, John Snow, qemu-devel, kwolf, qemu-block, arbel.moshe, seabios, kraxel Thanks for the detailed comment Laszlo, Indeed my e-mail has changed and I only received replies to the commits where I added this new mail in the S-o-b section, should of added in all of them. So as you said it, the problem was actually in using qfw_cfg_get_u32 which assumes the value is encoded LE and has an additional le32_to_cpu, should have used qfw_cfg_get directly like qfw_cfg_get_file does. Regarding qfw_cfg_get_file - I wrote this code when this function did not exist yet, I think it was added 6 months ago. In any case, I will use it instead. Thanks for this. I will resubmit this entire commit series: * I will only change code in the last commit (tests) * I will remove a comment which is now not true anymore * I will add my new email in S-o-b Sam On Wed, Oct 16, 2019 at 3:29 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 10/16/19 2:14 PM, Laszlo Ersek wrote: > > Hi Sam, > > > > On 10/16/19 13:02, Sam Eiderman wrote: > >> Gentle Ping, > >> > >> Philippe, John? > >> > >> Just wondering if the series is okay, as Gerd pointed out this series > >> is a blocker for the corresponding changes in SeaBIOS for v 1.13 > > > > The QEMU series is still not merged, due to a bug in the last patch > > (namely, the test case, "hd-geo-test: Add tests for lchs override"). > > > > To my knowledge, SeaBIOS prefers to merge patches with the underlying > > QEMU patches merged first, so you'll likely have to fix that QEMU issue > > first. > > > > I explained the bug in the QEMU test case here: > > > > http://mid.mail-archive.com/6b00dc74-7267-8ce8-3271-5db269edb1b7@redhat.com > > http://mid.mail-archive.com/700cd594-1446-e478-fb03-d2e6b862dc6c@redhat.com > > Yes, I was expecting a respin with find_fw_cfg_file() fixed per Laszlo > detailed review. > > > (Alternative links to the same: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01790.html > > https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01793.html > > ) > > > > I've never received feedback to those messages, and I think you must > > have missed them. > > > > FWIW, when I hit "Reply All" in that thread, you were on the "To:" list > > with: > > > > Sam Eiderman <shmuel.eiderman@oracle.com> > > > > but here you are present with > > > > Sam Eiderman <sameid@google.com> > > > > In addition, when I posted those messages, I got the following > > auto-response ("Undelivered Mail Returned to Sender"): > > > >> This is the mail system at host mx1.redhat.com. > >> > >> I'm sorry to have to inform you that your message could not > >> be delivered to one or more recipients. It's attached below. > >> > >> For further assistance, please send mail to postmaster. > >> > >> If you do so, please include this problem report. You can > >> delete your own text from the attached returned message. > >> > >> The mail system > >> > >> <shmuel.eiderman@oracle.com>: host > >> aserp2030.oracle.com[141.146.126.74] said: > >> 550 5.1.1 Unknown recipient address. (in reply to RCPT TO command) > > That explains it :) > > > > > I didn't know your new address, so I could only hope you'd find my > > feedback on qemu-devel. > > > > Thanks > > Laszlo > > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 14:55 ` Sam Eiderman @ 2019-10-16 15:07 ` John Snow 2019-10-16 15:19 ` Sam Eiderman 0 siblings, 1 reply; 30+ messages in thread From: John Snow @ 2019-10-16 15:07 UTC (permalink / raw) To: Sam Eiderman, Philippe Mathieu-Daudé Cc: kwolf, qemu-block, arbel.moshe, seabios, qemu-devel, kraxel, Laszlo Ersek 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 15:07 ` John Snow @ 2019-10-16 15:19 ` Sam Eiderman 2019-10-16 16:41 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman @ 2019-10-16 15:19 UTC (permalink / raw) To: John Snow Cc: Philippe Mathieu-Daudé, Laszlo Ersek, qemu-devel, kwolf, qemu-block, arbel.moshe, seabios, kraxel 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] Re: [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values 2019-10-16 15:19 ` Sam Eiderman @ 2019-10-16 16:41 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-10-16 16:41 UTC (permalink / raw) To: Sam Eiderman, John Snow Cc: kwolf, qemu-block, arbel.moshe, seabios, qemu-devel, kraxel, Laszlo Ersek 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v7 8/8] hd-geo-test: Add tests for lchs override 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (6 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values Sam Eiderman via @ 2019-09-25 11:06 ` Sam Eiderman via 2019-09-26 10:00 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 20:38 ` [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface John Snow 2019-09-27 17:02 ` no-reply 9 siblings, 1 reply; 30+ messages in thread From: Sam Eiderman via @ 2019-09-25 11:06 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel, Sam Eiderman, sameid, karl.heubaum 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(); -- 2.23.0.351.gc4317032e6-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 8/8] hd-geo-test: Add tests for lchs override 2019-09-25 11:06 ` [PATCH v7 8/8] hd-geo-test: Add tests for lchs override Sam Eiderman via @ 2019-09-26 10:00 ` Philippe Mathieu-Daudé 2019-09-26 18:19 ` John Snow 0 siblings, 1 reply; 30+ messages in thread From: Philippe Mathieu-Daudé @ 2019-09-26 10:00 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kraxel, karl.heubaum 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(); > > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [SeaBIOS] [PATCH v7 8/8] hd-geo-test: Add tests for lchs override 2019-09-26 10:00 ` [SeaBIOS] " Philippe Mathieu-Daudé @ 2019-09-26 18:19 ` John Snow 0 siblings, 0 replies; 30+ messages in thread From: John Snow @ 2019-09-26 18:19 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Sam Eiderman, qemu-devel Cc: kwolf, seabios, kraxel, qemu-block, arbel.moshe 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (7 preceding siblings ...) 2019-09-25 11:06 ` [PATCH v7 8/8] hd-geo-test: Add tests for lchs override Sam Eiderman via @ 2019-09-25 20:38 ` John Snow 2019-09-27 17:02 ` no-reply 9 siblings, 0 replies; 30+ messages in thread From: John Snow @ 2019-09-25 20:38 UTC (permalink / raw) To: Sam Eiderman, qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, kevin, liran.alon, kraxel 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! ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via ` (8 preceding siblings ...) 2019-09-25 20:38 ` [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface John Snow @ 2019-09-27 17:02 ` no-reply 9 siblings, 0 replies; 30+ messages in thread From: no-reply @ 2019-09-27 17:02 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, arbel.moshe, seabios, qemu-devel, kevin, liran.alon, kraxel, sameid, karl.heubaum Patchew URL: https://patchew.org/QEMU/20190925110639.100699-1-sameid@google.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20190925110639.100699-1-sameid@google.com Subject: [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu deee6ff..c6f5012 master -> master - [tag update] patchew/1569590461-12562-1-git-send-email-mjrosato@linux.ibm.com -> patchew/1569590461-12562-1-git-send-email-mjrosato@linux.ibm.com - [tag update] patchew/1569591203-15258-1-git-send-email-imbrenda@linux.ibm.com -> patchew/1569591203-15258-1-git-send-email-imbrenda@linux.ibm.com * [new tag] patchew/20190927101110.25581-1-berrange@redhat.com -> patchew/20190927101110.25581-1-berrange@redhat.com * [new tag] patchew/20190927134224.14550-1-marcandre.lureau@redhat.com -> patchew/20190927134224.14550-1-marcandre.lureau@redhat.com * [new tag] patchew/20190927134639.4284-1-armbru@redhat.com -> patchew/20190927134639.4284-1-armbru@redhat.com * [new tag] patchew/20190927141728.7137-1-crosa@redhat.com -> patchew/20190927141728.7137-1-crosa@redhat.com Switched to a new branch 'test' 3d14890 hd-geo-test: Add tests for lchs override 8db970f bootdevice: FW_CFG interface for LCHS values f73e65a8 bootdevice: Refactor get_boot_devices_list d1d97ec bootdevice: Gather LCHS from all relevant devices 31221f5 scsi: Propagate unrealize() callback to scsi-hd 6422c6a bootdevice: Add interface to gather LCHS b2360f7 block: Support providing LCHS from user 1c43be6 block: Refactor macros - fix tabbing === OUTPUT BEGIN === 1/8 Checking commit 1c43be6de15d (block: Refactor macros - fix tabbing) ERROR: Macros with complex values should be enclosed in parenthesis #55: FILE: include/hw/block/block.h:65: +#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) total: 1 errors, 0 warnings, 37 lines checked Patch 1/8 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/8 Checking commit b2360f718807 (block: Support providing LCHS from user) 3/8 Checking commit 6422c6acc42a (bootdevice: Add interface to gather LCHS) 4/8 Checking commit 31221f5a0d4d (scsi: Propagate unrealize() callback to scsi-hd) 5/8 Checking commit d1d97eca7de2 (bootdevice: Gather LCHS from all relevant devices) 6/8 Checking commit f73e65a879c8 (bootdevice: Refactor get_boot_devices_list) 7/8 Checking commit 8db970f739ad (bootdevice: FW_CFG interface for LCHS values) 8/8 Checking commit 3d148902136a (hd-geo-test: Add tests for lchs override) WARNING: Block comments use a leading /* on a separate line #648: FILE: tests/hd-geo-test.c:1003: + "skipping hd-geo/override/* tests"); total: 0 errors, 1 warnings, 616 lines checked Patch 8/8 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190925110639.100699-1-sameid@google.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-10-16 16:51 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-25 11:06 [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 1/8] block: Refactor macros - fix tabbing Sam Eiderman via 2019-09-26 9:38 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 2/8] block: Support providing LCHS from user Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 3/8] bootdevice: Add interface to gather LCHS Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 4/8] scsi: Propagate unrealize() callback to scsi-hd Sam Eiderman via 2019-09-26 9:38 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 5/8] bootdevice: Gather LCHS from all relevant devices Sam Eiderman via 2019-09-25 11:06 ` [PATCH v7 6/8] bootdevice: Refactor get_boot_devices_list Sam Eiderman via 2019-09-26 9:42 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 7/8] bootdevice: FW_CFG interface for LCHS values Sam Eiderman via 2019-09-26 9:57 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-26 18:26 ` John Snow 2019-09-26 19:09 ` Philippe Mathieu-Daudé 2019-09-26 19:16 ` Philippe Mathieu-Daudé 2019-09-29 10:13 ` Sam Eiderman 2019-10-08 11:34 ` Philippe Mathieu-Daudé 2019-10-08 11:51 ` Sam Eiderman 2019-10-16 11:02 ` Sam Eiderman 2019-10-16 12:14 ` [SeaBIOS] " Laszlo Ersek 2019-10-16 12:29 ` Philippe Mathieu-Daudé 2019-10-16 14:55 ` Sam Eiderman 2019-10-16 15:07 ` John Snow 2019-10-16 15:19 ` Sam Eiderman 2019-10-16 16:41 ` Philippe Mathieu-Daudé 2019-09-25 11:06 ` [PATCH v7 8/8] hd-geo-test: Add tests for lchs override Sam Eiderman via 2019-09-26 10:00 ` [SeaBIOS] " Philippe Mathieu-Daudé 2019-09-26 18:19 ` John Snow 2019-09-25 20:38 ` [PATCH v7 0/8] Add Qemu to SeaBIOS LCHS interface John Snow 2019-09-27 17:02 ` no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.