| Message ID | 20190827082427.64280-1-sameid@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Qemu to SeaBIOS LCHS interface | expand |
Patchew URL: https://patchew.org/QEMU/20190827082427.64280-1-sameid@google.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190827082427.64280-1-sameid@google.com
Type: series
Subject: [Qemu-devel] [PATCH v6 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
Switched to a new branch 'test'
4c35c4b hd-geo-test: Add tests for lchs override
94fe50a bootdevice: FW_CFG interface for LCHS values
feb142b bootdevice: Refactor get_boot_devices_list
d0d1b0a bootdevice: Gather LCHS from all relevant devices
47aaba7 scsi: Propagate unrealize() callback to scsi-hd
c15a6ef bootdevice: Add interface to gather LCHS
5ee6a57 block: Support providing LCHS from user
7058d32 block: Refactor macros - fix tabbing
=== OUTPUT BEGIN ===
1/8 Checking commit 7058d3229d71 (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 5ee6a57a8e93 (block: Support providing LCHS from user)
3/8 Checking commit c15a6ef42266 (bootdevice: Add interface to gather LCHS)
4/8 Checking commit 47aaba7c960c (scsi: Propagate unrealize() callback to scsi-hd)
5/8 Checking commit d0d1b0a500c3 (bootdevice: Gather LCHS from all relevant devices)
6/8 Checking commit feb142b1966b (bootdevice: Refactor get_boot_devices_list)
7/8 Checking commit 94fe50a2c30f (bootdevice: FW_CFG interface for LCHS values)
8/8 Checking commit 4c35c4b69cae (hd-geo-test: Add tests for lchs override)
WARNING: Block comments use a leading /* on a separate line
#641: FILE: tests/hd-geo-test.c:996:
+ "skipping hd-geo/override/* tests");
total: 0 errors, 1 warnings, 609 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/20190827082427.64280-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
Patchew URL: https://patchew.org/QEMU/20190827082427.64280-1-sameid@google.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190827082427.64280-1-sameid@google.com
Type: series
Subject: [Qemu-devel] [PATCH v6 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
Switched to a new branch 'test'
ae278de hd-geo-test: Add tests for lchs override
715cfa8 bootdevice: FW_CFG interface for LCHS values
54cd6e4 bootdevice: Refactor get_boot_devices_list
381ba9d bootdevice: Gather LCHS from all relevant devices
f16e5f7 scsi: Propagate unrealize() callback to scsi-hd
12bf34c bootdevice: Add interface to gather LCHS
fa36200 block: Support providing LCHS from user
3acc0a3 block: Refactor macros - fix tabbing
=== OUTPUT BEGIN ===
1/8 Checking commit 3acc0a31ae55 (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 fa36200c8ceb (block: Support providing LCHS from user)
3/8 Checking commit 12bf34c38eb5 (bootdevice: Add interface to gather LCHS)
4/8 Checking commit f16e5f7d73f7 (scsi: Propagate unrealize() callback to scsi-hd)
5/8 Checking commit 381ba9d8d879 (bootdevice: Gather LCHS from all relevant devices)
6/8 Checking commit 54cd6e409022 (bootdevice: Refactor get_boot_devices_list)
7/8 Checking commit 715cfa8c4ab1 (bootdevice: FW_CFG interface for LCHS values)
8/8 Checking commit ae278de99d62 (hd-geo-test: Add tests for lchs override)
WARNING: Block comments use a leading /* on a separate line
#641: FILE: tests/hd-geo-test.c:996:
+ "skipping hd-geo/override/* tests");
total: 0 errors, 1 warnings, 609 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/20190827082427.64280-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
Gentle ping On Tue, Aug 27, 2019, 11:24 Sam Eiderman <sameid@google.com> 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() > > 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 | 582 +++++++++++++++++++++++++++++++++++++++ > 11 files changed, 773 insertions(+), 41 deletions(-) > > -- > 2.23.0.187.g17f5b7556c-goog > >
Gentle ping On Wed, Sep 11, 2019 at 5:36 PM Sam Eiderman <sameid@google.com> wrote: > > Gentle ping > > On Tue, Aug 27, 2019, 11:24 Sam Eiderman <sameid@google.com> 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() >> >> 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 | 582 +++++++++++++++++++++++++++++++++++++++ >> 11 files changed, 773 insertions(+), 41 deletions(-) >> >> -- >> 2.23.0.187.g17f5b7556c-goog >>
On 27.08.19 10:24, Sam Eiderman via Qemu-devel 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() > > 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 | 582 +++++++++++++++++++++++++++++++++++++++ > 11 files changed, 773 insertions(+), 41 deletions(-) Acked-by: Max Reitz <mreitz@redhat.com>
Nobody was making movement on this patch series, and in response to Max acking the whole series, I was just going to send a pull request for the whole thing and see who barked, because nobody likes or hates this series enough to offer any feedback. Unfortunately, it's rotted on the vine a bit and has some conflicts with the testing infrastructure now: /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c: In function ‘test_override’: /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c:732:5: error: implicit declaration of function ‘qtest_start’ [-Werror=implicit-function-declaration] 732 | qtest_start(joined_args); You can jump right to the test by invoking it like this: > export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 > make tests/hd-geo-test It looks like some definitions got moved out from under our feet, but hopefully it won't take long to rectify. --js On 8/27/19 4:24 AM, Sam Eiderman via Qemu-block 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() > > 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 | 582 +++++++++++++++++++++++++++++++++++++++ > 11 files changed, 773 insertions(+), 41 deletions(-) >
On 24/09/2019 20.49, John Snow wrote: > Nobody was making movement on this patch series, and in response to Max > acking the whole series, I was just going to send a pull request for the > whole thing and see who barked, because nobody likes or hates this > series enough to offer any feedback. > > Unfortunately, it's rotted on the vine a bit and has some conflicts with > the testing infrastructure now: > > /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c: In function > ‘test_override’: > /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c:732:5: error: > implicit declaration of function ‘qtest_start’ > [-Werror=implicit-function-declaration] > 732 | qtest_start(joined_args); > > > You can jump right to the test by invoking it like this: > >> export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 >> make tests/hd-geo-test > > It looks like some definitions got moved out from under our feet, but > hopefully it won't take long to rectify. Please replace qtest_start() with qts = qtest_init() and qtest_end() with qtest_quit(qts). See this commit for some more details: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=44c2364aaa5e366c4 Thomas
Thanks Thomas, Resubmitting the tests, all other code will remain the same. Sam On Wed, Sep 25, 2019 at 10:58 AM Thomas Huth <thuth@redhat.com> wrote: > On 24/09/2019 20.49, John Snow wrote: > > Nobody was making movement on this patch series, and in response to Max > > acking the whole series, I was just going to send a pull request for the > > whole thing and see who barked, because nobody likes or hates this > > series enough to offer any feedback. > > > > Unfortunately, it's rotted on the vine a bit and has some conflicts with > > the testing infrastructure now: > > > > /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c: In function > > ‘test_override’: > > /home/jhuston/src/qemu.git/ide/tests/hd-geo-test.c:732:5: error: > > implicit declaration of function ‘qtest_start’ > > [-Werror=implicit-function-declaration] > > 732 | qtest_start(joined_args); > > > > > > You can jump right to the test by invoking it like this: > > > >> export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 > >> make tests/hd-geo-test > > > > It looks like some definitions got moved out from under our feet, but > > hopefully it won't take long to rectify. > > Please replace qtest_start() with qts = qtest_init() and qtest_end() > with qtest_quit(qts). > > See this commit for some more details: > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=44c2364aaa5e366c4 > > Thomas >
Hi Sam Eiderman via! We received your email, but were unable to deliver it because it contains HTML. HTML emails are not permitted. The following guide can help you configure your client to send in plain text instead: https://useplaintext.email If you have any questions, please reply to this email to reach the mail admin. We apologise for the inconvenience.
On 9/25/19 6:20 AM, Sam Eiderman wrote: > Thanks Thomas, > > Resubmitting the tests, all other code will remain the same. > > Sam > OK, feel free to just send the test by itself if only patch 8/8 changes. --js
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() 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 | 582 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 773 insertions(+), 41 deletions(-)