* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf [not found] ` <1340984094-5451-19-git-send-email-armbru@redhat.com> @ 2012-07-02 12:55 ` Stefan Hajnoczi 2012-07-02 13:07 ` Andreas Färber 0 siblings, 1 reply; 36+ messages in thread From: Stefan Hajnoczi @ 2012-07-02 12:55 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, pbonzini, aliguori, qemu-devel On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: > Best to use the same type, to avoid unwanted truncation or sign > extension. > > BlockConf can't use plain int for cyls, heads and secs, because > integer properties require an exact width. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > blockdev.h | 2 +- > hw/hd-geometry.c | 4 ++-- > hw/ide/core.c | 2 +- > hw/scsi-disk.c | 2 +- > hw/virtio-blk.c | 2 +- > 5 files changed, 6 insertions(+), 6 deletions(-) It would be nice to update the hd_geometry_lchs_guess() trace event that you added to use uint32_t + %u instead of int + %d. Stefan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-02 12:55 ` [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf Stefan Hajnoczi @ 2012-07-02 13:07 ` Andreas Färber 2012-07-02 14:15 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Andreas Färber @ 2012-07-02 13:07 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, aliguori, Markus Armbruster, qemu-devel Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: > On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >> Best to use the same type, to avoid unwanted truncation or sign >> extension. >> >> BlockConf can't use plain int for cyls, heads and secs, because >> integer properties require an exact width. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> blockdev.h | 2 +- >> hw/hd-geometry.c | 4 ++-- >> hw/ide/core.c | 2 +- >> hw/scsi-disk.c | 2 +- >> hw/virtio-blk.c | 2 +- >> 5 files changed, 6 insertions(+), 6 deletions(-) > > It would be nice to update the hd_geometry_lchs_guess() trace event that > you added to use uint32_t + %u instead of int + %d. PRIu32? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-02 13:07 ` Andreas Färber @ 2012-07-02 14:15 ` Markus Armbruster 2012-07-02 14:34 ` Stefan Hajnoczi 0 siblings, 1 reply; 36+ messages in thread From: Markus Armbruster @ 2012-07-02 14:15 UTC (permalink / raw) To: Andreas Färber Cc: kwolf, pbonzini, aliguori, Stefan Hajnoczi, qemu-devel Andreas Färber <afaerber@suse.de> writes: > Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: >> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >>> Best to use the same type, to avoid unwanted truncation or sign >>> extension. >>> >>> BlockConf can't use plain int for cyls, heads and secs, because >>> integer properties require an exact width. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> blockdev.h | 2 +- >>> hw/hd-geometry.c | 4 ++-- >>> hw/ide/core.c | 2 +- >>> hw/scsi-disk.c | 2 +- >>> hw/virtio-blk.c | 2 +- >>> 5 files changed, 6 insertions(+), 6 deletions(-) >> >> It would be nice to update the hd_geometry_lchs_guess() trace event that >> you added to use uint32_t + %u instead of int + %d. > > PRIu32? uint32_t: Good point, will do. Conversion specifier: trace-events routinely prints uint32_t with %d and %x, never with PRI*32. I'm happy to change from %d to %u, but reluctant to add the first use of PRIu32. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-02 14:15 ` Markus Armbruster @ 2012-07-02 14:34 ` Stefan Hajnoczi 2012-07-03 19:11 ` Blue Swirl 0 siblings, 1 reply; 36+ messages in thread From: Stefan Hajnoczi @ 2012-07-02 14:34 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, Stefan Hajnoczi, qemu-devel, pbonzini, Andreas Färber On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote: > Andreas Färber <afaerber@suse.de> writes: > >> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: >>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >>>> Best to use the same type, to avoid unwanted truncation or sign >>>> extension. >>>> >>>> BlockConf can't use plain int for cyls, heads and secs, because >>>> integer properties require an exact width. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> blockdev.h | 2 +- >>>> hw/hd-geometry.c | 4 ++-- >>>> hw/ide/core.c | 2 +- >>>> hw/scsi-disk.c | 2 +- >>>> hw/virtio-blk.c | 2 +- >>>> 5 files changed, 6 insertions(+), 6 deletions(-) >>> >>> It would be nice to update the hd_geometry_lchs_guess() trace event that >>> you added to use uint32_t + %u instead of int + %d. >> >> PRIu32? > > uint32_t: Good point, will do. > > Conversion specifier: trace-events routinely prints uint32_t with %d and > %x, never with PRI*32. I'm happy to change from %d to %u, but reluctant > to add the first use of PRIu32. Either is fine by me although we might as well continue to do %u. Stefan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-02 14:34 ` Stefan Hajnoczi @ 2012-07-03 19:11 ` Blue Swirl 2012-07-03 20:15 ` Andreas Färber 0 siblings, 1 reply; 36+ messages in thread From: Blue Swirl @ 2012-07-03 19:11 UTC (permalink / raw) To: Stefan Hajnoczi Cc: kwolf, aliguori, Stefan Hajnoczi, qemu-devel, Markus Armbruster, pbonzini, Andreas Färber On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Andreas Färber <afaerber@suse.de> writes: >> >>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: >>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >>>>> Best to use the same type, to avoid unwanted truncation or sign >>>>> extension. >>>>> >>>>> BlockConf can't use plain int for cyls, heads and secs, because >>>>> integer properties require an exact width. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>> --- >>>>> blockdev.h | 2 +- >>>>> hw/hd-geometry.c | 4 ++-- >>>>> hw/ide/core.c | 2 +- >>>>> hw/scsi-disk.c | 2 +- >>>>> hw/virtio-blk.c | 2 +- >>>>> 5 files changed, 6 insertions(+), 6 deletions(-) >>>> >>>> It would be nice to update the hd_geometry_lchs_guess() trace event that >>>> you added to use uint32_t + %u instead of int + %d. >>> >>> PRIu32? >> >> uint32_t: Good point, will do. >> >> Conversion specifier: trace-events routinely prints uint32_t with %d and >> %x, never with PRI*32. I'm happy to change from %d to %u, but reluctant >> to add the first use of PRIu32. > > Either is fine by me although we might as well continue to do %u. I'd also vote for %u. PRI*32 do not seem very useful compared to plain int versions. > > Stefan > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-03 19:11 ` Blue Swirl @ 2012-07-03 20:15 ` Andreas Färber 2012-07-04 16:19 ` Paolo Bonzini 2012-07-05 18:30 ` Blue Swirl 0 siblings, 2 replies; 36+ messages in thread From: Andreas Färber @ 2012-07-03 20:15 UTC (permalink / raw) To: Blue Swirl Cc: kwolf, aliguori, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, pbonzini Am 03.07.2012 21:11, schrieb Blue Swirl: > On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Andreas Färber <afaerber@suse.de> writes: >>> >>>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: >>>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >>>>>> Best to use the same type, to avoid unwanted truncation or sign >>>>>> extension. >>>>>> >>>>>> BlockConf can't use plain int for cyls, heads and secs, because >>>>>> integer properties require an exact width. >>>>>> >>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>>> --- >>>>>> blockdev.h | 2 +- >>>>>> hw/hd-geometry.c | 4 ++-- >>>>>> hw/ide/core.c | 2 +- >>>>>> hw/scsi-disk.c | 2 +- >>>>>> hw/virtio-blk.c | 2 +- >>>>>> 5 files changed, 6 insertions(+), 6 deletions(-) >>>>> >>>>> It would be nice to update the hd_geometry_lchs_guess() trace event that >>>>> you added to use uint32_t + %u instead of int + %d. >>>> >>>> PRIu32? >>> >>> uint32_t: Good point, will do. >>> >>> Conversion specifier: trace-events routinely prints uint32_t with %d and >>> %x, never with PRI*32. I'm happy to change from %d to %u, but reluctant >>> to add the first use of PRIu32. >> >> Either is fine by me although we might as well continue to do %u. > > I'd also vote for %u. PRI*32 do not seem very useful compared to plain > int versions. If it's not useful we should use unsigned int. Mixing both is simply wrong. I'm okay with fixing it consistently in a follow-up though. The Broken Window theory scores again (lack of const, non-CamelCase type names, placement of {, wrong semicolons, etc.). Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-03 20:15 ` Andreas Färber @ 2012-07-04 16:19 ` Paolo Bonzini 2012-07-04 16:36 ` Eric Blake 2012-07-05 18:30 ` Blue Swirl 1 sibling, 1 reply; 36+ messages in thread From: Paolo Bonzini @ 2012-07-04 16:19 UTC (permalink / raw) To: Andreas Färber Cc: kwolf, aliguori, Stefan Hajnoczi, Stefan Hajnoczi, Markus Armbruster, qemu-devel, Blue Swirl Il 03/07/2012 22:15, Andreas Färber ha scritto: >> > >> > I'd also vote for %u. PRI*32 do not seem very useful compared to plain >> > int versions. > If it's not useful we should use unsigned int. It just means that we're assuming 32-bit ints, which is a fact. The day an IL32 platform appears that has "typedef long int32_t", we'll worry about PRIu32. Paolo > Mixing both is simply wrong. I'm okay with fixing it consistently in a > follow-up though. The Broken Window theory scores again (lack of const, > non-CamelCase type names, placement of {, wrong semicolons, etc.). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-04 16:19 ` Paolo Bonzini @ 2012-07-04 16:36 ` Eric Blake 2012-07-05 18:37 ` Blue Swirl 0 siblings, 1 reply; 36+ messages in thread From: Eric Blake @ 2012-07-04 16:36 UTC (permalink / raw) To: Paolo Bonzini Cc: kwolf, aliguori, Stefan Hajnoczi, Stefan Hajnoczi, Markus Armbruster, qemu-devel, Blue Swirl, Andreas Färber [-- Attachment #1: Type: text/plain, Size: 847 bytes --] On 07/04/2012 10:19 AM, Paolo Bonzini wrote: > Il 03/07/2012 22:15, Andreas Färber ha scritto: >>>> >>>> I'd also vote for %u. PRI*32 do not seem very useful compared to plain >>>> int versions. >> If it's not useful we should use unsigned int. > > It just means that we're assuming 32-bit ints, which is a fact. The day > an IL32 platform appears that has "typedef long int32_t", we'll worry > about PRIu32. Newlib's <stdint.h> does just that, on platforms that __have_long32. Which makes Cygwin a great platform for flushing out these sorts of type discrepancies (since cygwin inherits from newlib). http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/include/stdint.h.diff?r1=1.8&r2=1.9&cvsroot=src&f=h -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 620 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-04 16:36 ` Eric Blake @ 2012-07-05 18:37 ` Blue Swirl 0 siblings, 0 replies; 36+ messages in thread From: Blue Swirl @ 2012-07-05 18:37 UTC (permalink / raw) To: Eric Blake Cc: kwolf, aliguori, Stefan Hajnoczi, Stefan Hajnoczi, Markus Armbruster, qemu-devel, Paolo Bonzini, Andreas Färber On Wed, Jul 4, 2012 at 4:36 PM, Eric Blake <eblake@redhat.com> wrote: > On 07/04/2012 10:19 AM, Paolo Bonzini wrote: >> Il 03/07/2012 22:15, Andreas Färber ha scritto: >>>>> >>>>> I'd also vote for %u. PRI*32 do not seem very useful compared to plain >>>>> int versions. >>> If it's not useful we should use unsigned int. >> >> It just means that we're assuming 32-bit ints, which is a fact. The day >> an IL32 platform appears that has "typedef long int32_t", we'll worry >> about PRIu32. > > Newlib's <stdint.h> does just that, on platforms that __have_long32. > Which makes Cygwin a great platform for flushing out these sorts of type > discrepancies (since cygwin inherits from newlib). > > http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/include/stdint.h.diff?r1=1.8&r2=1.9&cvsroot=src&f=h Interesting, my thoughts went pretty much along Paolo's line. Now I think PRI*32 indeed makes some sense. But we don't support Cygwin builds (only mingw), would that be useful? > > -- > Eric Blake eblake@redhat.com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-03 20:15 ` Andreas Färber 2012-07-04 16:19 ` Paolo Bonzini @ 2012-07-05 18:30 ` Blue Swirl 1 sibling, 0 replies; 36+ messages in thread From: Blue Swirl @ 2012-07-05 18:30 UTC (permalink / raw) To: Andreas Färber Cc: kwolf, aliguori, Stefan Hajnoczi, Stefan Hajnoczi, qemu-devel, Markus Armbruster, pbonzini On Tue, Jul 3, 2012 at 8:15 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 03.07.2012 21:11, schrieb Blue Swirl: >> On Mon, Jul 2, 2012 at 2:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> On Mon, Jul 2, 2012 at 3:15 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Andreas Färber <afaerber@suse.de> writes: >>>> >>>>> Am 02.07.2012 14:55, schrieb Stefan Hajnoczi: >>>>>> On Fri, Jun 29, 2012 at 05:34:40PM +0200, Markus Armbruster wrote: >>>>>>> Best to use the same type, to avoid unwanted truncation or sign >>>>>>> extension. >>>>>>> >>>>>>> BlockConf can't use plain int for cyls, heads and secs, because >>>>>>> integer properties require an exact width. >>>>>>> >>>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>>>> --- >>>>>>> blockdev.h | 2 +- >>>>>>> hw/hd-geometry.c | 4 ++-- >>>>>>> hw/ide/core.c | 2 +- >>>>>>> hw/scsi-disk.c | 2 +- >>>>>>> hw/virtio-blk.c | 2 +- >>>>>>> 5 files changed, 6 insertions(+), 6 deletions(-) >>>>>> >>>>>> It would be nice to update the hd_geometry_lchs_guess() trace event that >>>>>> you added to use uint32_t + %u instead of int + %d. >>>>> >>>>> PRIu32? >>>> >>>> uint32_t: Good point, will do. >>>> >>>> Conversion specifier: trace-events routinely prints uint32_t with %d and >>>> %x, never with PRI*32. I'm happy to change from %d to %u, but reluctant >>>> to add the first use of PRIu32. >>> >>> Either is fine by me although we might as well continue to do %u. >> >> I'd also vote for %u. PRI*32 do not seem very useful compared to plain >> int versions. > > If it's not useful we should use unsigned int. > > Mixing both is simply wrong. I'm okay with fixing it consistently in a > follow-up though. The Broken Window theory scores again (lack of const, > non-CamelCase type names, placement of {, wrong semicolons, etc.). Since the 'const' issues are not related to CODING_STYLE (for those Anthony vetoes global reformatting), I think global patches (preferably broken down for bisection though) should be OK. What's wrong with the semicolons? > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-9-git-send-email-armbru@redhat.com>]
[parent not found: <CAAu8pHsKC6QMrhjUADiXT8DUrhy9HzbkN8pbYeWjM2aAE38ZwQ@mail.gmail.com>]
[parent not found: <m3k3ypmjqu.fsf@blackfin.pond.sub.org>]
* Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c [not found] ` <m3k3ypmjqu.fsf@blackfin.pond.sub.org> @ 2012-07-03 18:40 ` Blue Swirl 2012-07-04 8:24 ` Kevin Wolf 0 siblings, 1 reply; 36+ messages in thread From: Blue Swirl @ 2012-07-03 18:40 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, pbonzini, aliguori, qemu-devel, stefanha On Sat, Jun 30, 2012 at 5:50 AM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Commit f3d54fc4 factored it out of hw/ide.c for reuse. Sensible, >>> except it was put into block.c. Device-specific functionality should >>> be kept in device code, not the block layer. Move it to >>> hw/hd-geometry.c, and make stylistic changes required to keep >>> checkpatch.pl happy. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> block.c | 121 ------------------------------------------ >>> block.h | 1 - >>> blockdev.h | 5 ++ >>> hw/Makefile.objs | 2 +- >>> hw/hd-geometry.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> hw/ide/core.c | 2 +- >>> hw/scsi-disk.c | 4 +- >>> hw/virtio-blk.c | 2 +- >>> 8 files changed, 163 insertions(+), 127 deletions(-) >>> create mode 100644 hw/hd-geometry.c > [...] >>> diff --git a/blockdev.h b/blockdev.h >>> index 260e16b..7b05945 100644 >>> --- a/blockdev.h >>> +++ b/blockdev.h >>> @@ -62,4 +62,9 @@ void qmp_change_blockdev(const char *device, const char *filename, >>> bool has_format, const char *format, Error **errp); >>> void do_commit(Monitor *mon, const QDict *qdict); >>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); >>> + >>> +/* Hard disk geometry */ >>> +void hd_geometry_guess(BlockDriverState *bs, >>> + int *pcyls, int *pheads, int *psecs); >> >> I'd move this to a separate header under hw/. > > hw/hd-geometry.h? Or a header collecting shared block device model > code, say hw/block-common.h? I'd be happy with either. > >>> + >>> #endif >>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs >>> index 3d77259..5bf1d76 100644 >>> --- a/hw/Makefile.objs >>> +++ b/hw/Makefile.objs >>> @@ -137,7 +137,7 @@ common-obj-$(CONFIG_MAX111X) += max111x.o >>> common-obj-$(CONFIG_DS1338) += ds1338.o >>> common-obj-y += i2c.o smbus.o smbus_eeprom.o >>> common-obj-y += eeprom93xx.o >>> -common-obj-y += scsi-disk.o cdrom.o >>> +common-obj-y += scsi-disk.o cdrom.o hd-geometry.o >>> common-obj-y += scsi-generic.o scsi-bus.o >>> common-obj-y += hid.o >>> common-obj-$(CONFIG_SSI) += ssi.o >>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c >>> new file mode 100644 >>> index 0000000..9b22e3f >>> --- /dev/null >>> +++ b/hw/hd-geometry.c >>> @@ -0,0 +1,153 @@ >>> +/* >>> + * Hard disk geometry utilities >>> + * >>> + * Copyright (c) 2003 Fabrice Bellard >>> + * >>> + * Permission is hereby granted, free of charge, to any person obtaining a copy >>> + * of this software and associated documentation files (the "Software"), to deal >>> + * in the Software without restriction, including without limitation the rights >>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell >>> + * copies of the Software, and to permit persons to whom the Software is >>> + * furnished to do so, subject to the following conditions: >>> + * >>> + * The above copyright notice and this permission notice shall be included in >>> + * all copies or substantial portions of the Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, >>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >>> + * THE SOFTWARE. >>> + */ > > By the way, X11 license. I'd very much prefer to put my contributions > under GPLv2+. Is there an acceptable way to do that? IANAL, but isn't MIT license compatible with GPL? Then maybe you could license your contributions under GPLv2+, for example with the same statement as GPLv2 to GPLv2+: "Contributions after 2012-mm-dd are licensed under the terms of the GNU GPL, version 2 or (at your option) any later version." Though after that, the file would be dual licensed. Those who want to use the MIT version can use the file before the change. >>> +#include "blockdev.h" >>> + >>> +struct partition { >> >> Partition > > Okay. > > [...] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c 2012-07-03 18:40 ` [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c Blue Swirl @ 2012-07-04 8:24 ` Kevin Wolf 0 siblings, 0 replies; 36+ messages in thread From: Kevin Wolf @ 2012-07-04 8:24 UTC (permalink / raw) To: Blue Swirl; +Cc: pbonzini, aliguori, Markus Armbruster, stefanha, qemu-devel Am 03.07.2012 20:40, schrieb Blue Swirl: > On Sat, Jun 30, 2012 at 5:50 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@gmail.com> writes: >> >>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Commit f3d54fc4 factored it out of hw/ide.c for reuse. Sensible, >>>> except it was put into block.c. Device-specific functionality should >>>> be kept in device code, not the block layer. Move it to >>>> hw/hd-geometry.c, and make stylistic changes required to keep >>>> checkpatch.pl happy. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> block.c | 121 ------------------------------------------ >>>> block.h | 1 - >>>> blockdev.h | 5 ++ >>>> hw/Makefile.objs | 2 +- >>>> hw/hd-geometry.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> hw/ide/core.c | 2 +- >>>> hw/scsi-disk.c | 4 +- >>>> hw/virtio-blk.c | 2 +- >>>> 8 files changed, 163 insertions(+), 127 deletions(-) >>>> create mode 100644 hw/hd-geometry.c >> [...] >>>> diff --git a/blockdev.h b/blockdev.h >>>> index 260e16b..7b05945 100644 >>>> --- a/blockdev.h >>>> +++ b/blockdev.h >>>> @@ -62,4 +62,9 @@ void qmp_change_blockdev(const char *device, const char *filename, >>>> bool has_format, const char *format, Error **errp); >>>> void do_commit(Monitor *mon, const QDict *qdict); >>>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); >>>> + >>>> +/* Hard disk geometry */ >>>> +void hd_geometry_guess(BlockDriverState *bs, >>>> + int *pcyls, int *pheads, int *psecs); >>> >>> I'd move this to a separate header under hw/. >> >> hw/hd-geometry.h? Or a header collecting shared block device model >> code, say hw/block-common.h? > > I'd be happy with either. I think block-common.h is better. We don't have that much block layer specific guest-side code that it would make sense to split it up. >>>> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c >>>> new file mode 100644 >>>> index 0000000..9b22e3f >>>> --- /dev/null >>>> +++ b/hw/hd-geometry.c >>>> @@ -0,0 +1,153 @@ >>>> +/* >>>> + * Hard disk geometry utilities >>>> + * >>>> + * Copyright (c) 2003 Fabrice Bellard >>>> + * >>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy >>>> + * of this software and associated documentation files (the "Software"), to deal >>>> + * in the Software without restriction, including without limitation the rights >>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell >>>> + * copies of the Software, and to permit persons to whom the Software is >>>> + * furnished to do so, subject to the following conditions: >>>> + * >>>> + * The above copyright notice and this permission notice shall be included in >>>> + * all copies or substantial portions of the Software. >>>> + * >>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, >>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >>>> + * THE SOFTWARE. >>>> + */ >> >> By the way, X11 license. I'd very much prefer to put my contributions >> under GPLv2+. Is there an acceptable way to do that? > > IANAL, but isn't MIT license compatible with GPL? IANAL either, but as I understand it: It is, otherwise distributing qemu would be illegal. But compatibility means more or less that you can mix them within one project, it doesn't mean you can convert from one to the other. > Then maybe you could > license your contributions under GPLv2+, for example with the same > statement as GPLv2 to GPLv2+: > "Contributions after 2012-mm-dd are licensed under the terms of the > GNU GPL, version 2 or (at your option) any later version." > > Though after that, the file would be dual licensed. Those who want to > use the MIT version can use the file before the change. This might be possible. (Though true dual-licensing would mean that the whole code, including new parts, can be used under either license. This isn't what we would be doing.) Or have a GPL header and after that add a notice like "Portions of the code are licensed under the following conditions: <old license header>". In any case it's pretty clear to me that you're not allowed to drop the MIT license header (and therefore blockdev.c still needs a fix) Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-4-git-send-email-armbru@redhat.com>]
[parent not found: <CAAu8pHtQORnuc2G8DiWTt1QcWOKbp7L8WJ=P7HMMZCNV7L-NRQ@mail.gmail.com>]
* Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table [not found] ` <CAAu8pHtQORnuc2G8DiWTt1QcWOKbp7L8WJ=P7HMMZCNV7L-NRQ@mail.gmail.com> @ 2012-07-04 15:17 ` Kevin Wolf 2012-07-05 9:23 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-04 15:17 UTC (permalink / raw) To: Blue Swirl; +Cc: pbonzini, aliguori, Markus Armbruster, stefanha, qemu-devel Am 29.06.2012 22:33, schrieb Blue Swirl: > On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Unless parameter ":floppy:" is given, vvfat creates a virtual image >> with DOS MBR defining a single partition which holds the FAT file >> system. The size of the virtual image depends on the width of the >> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >> (1024*16-1)*64 = 1032129 sectors for the partition. >> >> However, it screws up the end of the partition in the MBR: >> >> FAT width param. start CHS end CHS start LBA size >> :32: 0,1,1 1023,14,63 63 1032065 >> :16: 0,1,1 1023,14,55 63 1032057 >> :12: 0,1,1 63,14,55 63 64377 >> >> The actual FAT file system nevertheless assumes the partition has >> 1032129 or 64449 sectors. Oops. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/vvfat.c | 7 ++++--- >> 1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block/vvfat.c b/block/vvfat.c >> index 0fd3367..62745b5 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >> >> /* LBA is used when partition is outside the CHS geometry */ >> lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1); >> - lba|= sector2CHS(s->bs, &partition->end_CHS, s->sector_count); >> + lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >> >> /*LBA partitions are identified only by start/length_sector_long not by CHS*/ >> - partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1); >> - partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1); >> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); > > Spaces around '-'. Thanks for fixing the other cases, BTW. For compensation there's an extra space before the '='. >> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >> + - s->first_sectors_number + 1); Just wondering... This should be the same as s->sector_count, right? This whole geometry calculation code in vvfat is way too convoluted to understand when you haven't looked at it for some months... Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table 2012-07-04 15:17 ` [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table Kevin Wolf @ 2012-07-05 9:23 ` Markus Armbruster 2012-07-05 9:55 ` Kevin Wolf 0 siblings, 1 reply; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 9:23 UTC (permalink / raw) To: Kevin Wolf; +Cc: Blue Swirl, pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 22:33, schrieb Blue Swirl: >> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>> with DOS MBR defining a single partition which holds the FAT file >>> system. The size of the virtual image depends on the width of the >>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>> (1024*16-1)*64 = 1032129 sectors for the partition. >>> >>> However, it screws up the end of the partition in the MBR: >>> >>> FAT width param. start CHS end CHS start LBA size >>> :32: 0,1,1 1023,14,63 63 1032065 >>> :16: 0,1,1 1023,14,55 63 1032057 >>> :12: 0,1,1 63,14,55 63 64377 >>> >>> The actual FAT file system nevertheless assumes the partition has >>> 1032129 or 64449 sectors. Oops. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> block/vvfat.c | 7 ++++--- >>> 1 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/block/vvfat.c b/block/vvfat.c >>> index 0fd3367..62745b5 100644 >>> --- a/block/vvfat.c >>> +++ b/block/vvfat.c >>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>> >>> /* LBA is used when partition is outside the CHS geometry */ >>> lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1); >>> - lba|= sector2CHS(s->bs, &partition->end_CHS, s->sector_count); >>> + lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>> >>> /*LBA partitions are identified only by start/length_sector_long not by CHS*/ >>> - partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1); >>> - partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1); >>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >> >> Spaces around '-'. Thanks for fixing the other cases, BTW. > > For compensation there's an extra space before the '='. The original lined up the two '='. I preserved that. Not that I care for it. Want me to drop the extra space? >>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>> + - s->first_sectors_number + 1); > > Just wondering... This should be the same as s->sector_count, right? Hmm. vvfat_open() assigns: s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); bs->total_sectors = cyls * heads * secs; But it then changes it minds and does: s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > This whole geometry calculation code in vvfat is way too convoluted to > understand when you haven't looked at it for some months... Yes, it is. I was briefly tempted to replace it, but then decided to limit my footprint in this buck-crazy block driver to minimal bug fixes. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table 2012-07-05 9:23 ` Markus Armbruster @ 2012-07-05 9:55 ` Kevin Wolf 2012-07-05 11:10 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 9:55 UTC (permalink / raw) To: Markus Armbruster; +Cc: Blue Swirl, pbonzini, aliguori, qemu-devel, stefanha Am 05.07.2012 11:23, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 29.06.2012 22:33, schrieb Blue Swirl: >>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>>> with DOS MBR defining a single partition which holds the FAT file >>>> system. The size of the virtual image depends on the width of the >>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>>> (1024*16-1)*64 = 1032129 sectors for the partition. >>>> >>>> However, it screws up the end of the partition in the MBR: >>>> >>>> FAT width param. start CHS end CHS start LBA size >>>> :32: 0,1,1 1023,14,63 63 1032065 >>>> :16: 0,1,1 1023,14,55 63 1032057 >>>> :12: 0,1,1 63,14,55 63 64377 >>>> >>>> The actual FAT file system nevertheless assumes the partition has >>>> 1032129 or 64449 sectors. Oops. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> block/vvfat.c | 7 ++++--- >>>> 1 files changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/block/vvfat.c b/block/vvfat.c >>>> index 0fd3367..62745b5 100644 >>>> --- a/block/vvfat.c >>>> +++ b/block/vvfat.c >>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>>> >>>> /* LBA is used when partition is outside the CHS geometry */ >>>> lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1); >>>> - lba|= sector2CHS(s->bs, &partition->end_CHS, s->sector_count); >>>> + lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>>> >>>> /*LBA partitions are identified only by start/length_sector_long not by CHS*/ >>>> - partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1); >>>> - partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1); >>>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >>> >>> Spaces around '-'. Thanks for fixing the other cases, BTW. >> >> For compensation there's an extra space before the '='. > > The original lined up the two '='. I preserved that. Not that I care > for it. Want me to drop the extra space? Ah, didn't notice that. I don't mind then. >>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>> + - s->first_sectors_number + 1); >> >> Just wondering... This should be the same as s->sector_count, right? > > Hmm. vvfat_open() assigns: > > s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); > bs->total_sectors = cyls * heads * secs; > > But it then changes it minds and does: > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; Which probably means that they differ if some sub-cluster sized space is left unused at the end of the disk. It's not useful to have this space included in the partition, but it doesn't hurt either. So I suppose either way is fine. >> This whole geometry calculation code in vvfat is way too convoluted to >> understand when you haven't looked at it for some months... > > Yes, it is. I was briefly tempted to replace it, but then decided to > limit my footprint in this buck-crazy block driver to minimal bug fixes. Heh, quite understandable. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table 2012-07-05 9:55 ` Kevin Wolf @ 2012-07-05 11:10 ` Markus Armbruster 2012-07-05 11:14 ` Kevin Wolf 0 siblings, 1 reply; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 11:10 UTC (permalink / raw) To: Kevin Wolf; +Cc: Blue Swirl, pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 05.07.2012 11:23, schrieb Markus Armbruster: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 29.06.2012 22:33, schrieb Blue Swirl: >>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>>>> with DOS MBR defining a single partition which holds the FAT file >>>>> system. The size of the virtual image depends on the width of the >>>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>>>> (1024*16-1)*64 = 1032129 sectors for the partition. >>>>> >>>>> However, it screws up the end of the partition in the MBR: >>>>> >>>>> FAT width param. start CHS end CHS start LBA size >>>>> :32: 0,1,1 1023,14,63 63 1032065 >>>>> :16: 0,1,1 1023,14,55 63 1032057 >>>>> :12: 0,1,1 63,14,55 63 64377 >>>>> >>>>> The actual FAT file system nevertheless assumes the partition has >>>>> 1032129 or 64449 sectors. Oops. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>> --- >>>>> block/vvfat.c | 7 ++++--- >>>>> 1 files changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/block/vvfat.c b/block/vvfat.c >>>>> index 0fd3367..62745b5 100644 >>>>> --- a/block/vvfat.c >>>>> +++ b/block/vvfat.c >>>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>>>> >>>>> /* LBA is used when partition is outside the CHS geometry */ >>>>> lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1); >>>>> - lba|= sector2CHS(s->bs, &partition->end_CHS, s->sector_count); >>>>> + lba |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>>>> >>>>> /*LBA partitions are identified only by start/length_sector_long not by CHS*/ >>>>> - partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1); >>>>> - partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1); >>>>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >>>> >>>> Spaces around '-'. Thanks for fixing the other cases, BTW. >>> >>> For compensation there's an extra space before the '='. >> >> The original lined up the two '='. I preserved that. Not that I care >> for it. Want me to drop the extra space? > > Ah, didn't notice that. I don't mind then. > >>>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>>> + - s->first_sectors_number + 1); >>> >>> Just wondering... This should be the same as s->sector_count, right? >> >> Hmm. vvfat_open() assigns: >> >> s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); >> bs->total_sectors = cyls * heads * secs; >> >> But it then changes it minds and does: >> >> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > Which probably means that they differ if some sub-cluster sized space is > left unused at the end of the disk. It's not useful to have this space > included in the partition, but it doesn't hurt either. So I suppose > either way is fine. Complication: the partition should end on a cylinder boundary. Shaving off an unused tail may well interfere with that. Let's stick to v1 here. [...] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table 2012-07-05 11:10 ` Markus Armbruster @ 2012-07-05 11:14 ` Kevin Wolf 0 siblings, 0 replies; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 11:14 UTC (permalink / raw) To: Markus Armbruster; +Cc: Blue Swirl, pbonzini, aliguori, qemu-devel, stefanha Am 05.07.2012 13:10, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 05.07.2012 11:23, schrieb Markus Armbruster: >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 29.06.2012 22:33, schrieb Blue Swirl: >>>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>>>> + - s->first_sectors_number + 1); >>>> >>>> Just wondering... This should be the same as s->sector_count, right? >>> >>> Hmm. vvfat_open() assigns: >>> >>> s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); >>> bs->total_sectors = cyls * heads * secs; >>> >>> But it then changes it minds and does: >>> >>> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; >> >> Which probably means that they differ if some sub-cluster sized space is >> left unused at the end of the disk. It's not useful to have this space >> included in the partition, but it doesn't hurt either. So I suppose >> either way is fine. > > Complication: the partition should end on a cylinder boundary. Shaving > off an unused tail may well interfere with that. Let's stick to v1 > here. Good point. If anything, maybe add a comment. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-5-git-send-email-armbru@redhat.com>]
* Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry [not found] ` <1340984094-5451-5-git-send-email-armbru@redhat.com> @ 2012-07-04 15:23 ` Kevin Wolf 2012-07-04 16:25 ` Paolo Bonzini 2012-07-05 11:13 ` Markus Armbruster 0 siblings, 2 replies; 36+ messages in thread From: Kevin Wolf @ 2012-07-04 15:23 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 29.06.2012 17:34, schrieb Markus Armbruster: > vvfat creates a virtual VFAT filesystem with a certain logical > geometry that depends on its options. It sets the "geometry hint" to > this geometry. It is the only block driver to do this. > > The geometry hint is about about *physical* geometry, and used only by > certain hard disk device models. > > vvfat's hint is normally invisible for device models, because > bdrv_open() puts a raw format on top of vvfat's fat protocol. That > raw format is where drive_init() puts the user's geometry (if any), > and where the device model gets it from. > > Nobody complained, because the default physical geometry is the same > as vvfat's logical geometry: > > opts LCHS def. PCHS > 1024,16,63 same > :32: 1024,16,63 same > :16: 1024,16,63 same > :12: 64,16,63 same > > Except when you specify :floppy: > > opts LCHS def. PCHS > :floppy: 80, 2,36 5,16,63 > :32:floppy: 80, 2,36 5,16,63 > :16:floppy: 80, 2,36 5,16,63 > :12:floppy: 80, 2,18 2,16,63 > > Silly thing to do for use with a hard disk. > > However, the "raw" format can be suppressed by adding an > redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's > hint clobbers the user's geometry, i.e. -drive options cyls, heads, > secs get silently ignored. Don't do that. > > No change without format=vvfat. With it, the user's hard disk > geometry (-drive options cyls, heads, secs) is now obeyed, and the > default hard disk geometry with :floppy: now matches the one without > format=vvfat. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> For some values of "obeyed". If I understand correctly, the user defined geometry will indeed by visible for the device emulation now, but this still doesn't mean that vvfat also provides an image that matches this geometry. Not sure if it is a good idea to allow such mismatches. > @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) { > else > dirname += i+1; > > - bs->total_sectors=bs->cyls*bs->heads*bs->secs; > + bs->total_sectors = cyls * heads * secs; > > - if(init_directories(s, dirname)) > + if (init_directories(s, dirname, heads, secs)) { > return -1; > + } > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > if(s->first_sectors_number==0x40) > - init_mbr(s); > - else { > - /* MS-DOS does not like to know about CHS (?). */ > - bs->heads = bs->cyls = bs->secs = 0; > - } > + init_mbr(s, cyls, heads, secs); You can add braces here while touching the code. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry 2012-07-04 15:23 ` [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry Kevin Wolf @ 2012-07-04 16:25 ` Paolo Bonzini 2012-07-05 7:06 ` Kevin Wolf 2012-07-05 11:13 ` Markus Armbruster 1 sibling, 1 reply; 36+ messages in thread From: Paolo Bonzini @ 2012-07-04 16:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: aliguori, Markus Armbruster, stefanha, qemu-devel Il 04/07/2012 17:23, Kevin Wolf ha scritto: >> > opts LCHS def. PCHS >> > :floppy: 80, 2,36 5,16,63 >> > :32:floppy: 80, 2,36 5,16,63 >> > :16:floppy: 80, 2,36 5,16,63 >> > :12:floppy: 80, 2,18 2,16,63 >> > >> > Silly thing to do for use with a hard disk. >> > >> > However, the "raw" format can be suppressed by adding an >> > redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >> > hint clobbers the user's geometry, i.e. -drive options cyls, heads, >> > secs get silently ignored. Don't do that. >> > >> > No change without format=vvfat. With it, the user's hard disk >> > geometry (-drive options cyls, heads, secs) is now obeyed, and the >> > default hard disk geometry with :floppy: now matches the one without >> > format=vvfat. >> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > For some values of "obeyed". If I understand correctly, the user defined > geometry will indeed by visible for the device emulation now, but this > still doesn't mean that vvfat also provides an image that matches this > geometry. Not sure if it is a good idea to allow such mismatches. > Does this only matter if you declare an image with :floppy: and pass it as a hard disk? Then we can honestly say we don't care... Paolo ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry 2012-07-04 16:25 ` Paolo Bonzini @ 2012-07-05 7:06 ` Kevin Wolf 2012-07-05 9:16 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 7:06 UTC (permalink / raw) To: Paolo Bonzini; +Cc: aliguori, Markus Armbruster, stefanha, qemu-devel Am 04.07.2012 18:25, schrieb Paolo Bonzini: > Il 04/07/2012 17:23, Kevin Wolf ha scritto: >>>> opts LCHS def. PCHS >>>> :floppy: 80, 2,36 5,16,63 >>>> :32:floppy: 80, 2,36 5,16,63 >>>> :16:floppy: 80, 2,36 5,16,63 >>>> :12:floppy: 80, 2,18 2,16,63 >>>> >>>> Silly thing to do for use with a hard disk. >>>> >>>> However, the "raw" format can be suppressed by adding an >>>> redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads, >>>> secs get silently ignored. Don't do that. >>>> >>>> No change without format=vvfat. With it, the user's hard disk >>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the >>>> default hard disk geometry with :floppy: now matches the one without >>>> format=vvfat. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> For some values of "obeyed". If I understand correctly, the user defined >> geometry will indeed by visible for the device emulation now, but this >> still doesn't mean that vvfat also provides an image that matches this >> geometry. Not sure if it is a good idea to allow such mismatches. >> > > Does this only matter if you declare an image with :floppy: and pass it > as a hard disk? Then we can honestly say we don't care... As I understand it, the patch has two effect: 1. the user's hard disk geometry (-drive options cyls, heads, secs) is now obeyed 2. the default hard disk geometry with :floppy: now matches the one without format=vvfat I don't care about 2. indeed, but doing 1. right would be nice. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry 2012-07-05 7:06 ` Kevin Wolf @ 2012-07-05 9:16 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 9:16 UTC (permalink / raw) To: Kevin Wolf; +Cc: Paolo Bonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 04.07.2012 18:25, schrieb Paolo Bonzini: >> Il 04/07/2012 17:23, Kevin Wolf ha scritto: >>>>> opts LCHS def. PCHS >>>>> :floppy: 80, 2,36 5,16,63 >>>>> :32:floppy: 80, 2,36 5,16,63 >>>>> :16:floppy: 80, 2,36 5,16,63 >>>>> :12:floppy: 80, 2,18 2,16,63 >>>>> >>>>> Silly thing to do for use with a hard disk. >>>>> >>>>> However, the "raw" format can be suppressed by adding an >>>>> redundant-looking "format=vvfat" to "file=fat:FOO". Then, vvfat's >>>>> hint clobbers the user's geometry, i.e. -drive options cyls, heads, >>>>> secs get silently ignored. Don't do that. >>>>> >>>>> No change without format=vvfat. With it, the user's hard disk >>>>> geometry (-drive options cyls, heads, secs) is now obeyed, and the >>>>> default hard disk geometry with :floppy: now matches the one without >>>>> format=vvfat. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> For some values of "obeyed". If I understand correctly, the user defined >>> geometry will indeed by visible for the device emulation now, but this >>> still doesn't mean that vvfat also provides an image that matches this >>> geometry. Not sure if it is a good idea to allow such mismatches. This patch doesn't let the user do anything he couldn't do before. It only makes the special case "vvfat without raw on top" consistent with all other block backend configurations. With "raw over vvfat" in particular. So, if anything needs additional user parameter validation, it's the common case. Note that any other image type could contain the very same FAT filesystem as a vvfat one, with the very same impact on a guest vulnerable to mismatches between geometry and block device contents. To avoid confusion, let me recap geometry. Physical geometry is a property of the disk. Or rather, a property of the disk's legacy CHS command interface; it ceased to have any useful relation to platters and heads long ago. Non-legacy commands use LBA to address blocks. Logical geometry is a property of the PC BIOS legacy CHS interface (int 13h). It was invented when disks outgrew the BIOS's CHS limits. It need not match physical geometry (*not* matching is its purpose). The BIOS translates logical geometry to something the disk understands. Long ago, this meant translating to physical CHS. Nowadays, it means translating to LBA. As far as I can tell, SeaBIOS always translates to LBA. SeaBIOS picks a logical geometry based on what it finds in CMOS. Logical geometry matters only for software that uses legacy int 13h. DOS, basically. Physical geometry matters only for software that talks to the disk directly, using legacy CHS commands. Sufficently old versions of Unix, perhaps? The choice of geometry isn't really important, as long as: 1. The mapping to LBA is sane: lba = ((c * #h) + h * #s) + (s-1) where c, h, s is the address, and #c, #h, #s is the geometry. Increasing first s, then h, then c runs through the lbas in order, regardless of what you picked for geometry. For physical geometry, the device model maps. As far as I can tell, they all map sanely. For logical geometry, SeaBIOS maps. As far as I can tell, it maps sanely. Actually, even an insane mapping would work for block drivers that only schlep blocks, as long as it doesn't change. Same as with physical disks. 2. The OS is happy with the partition table. Some OSs want each partition to start and end on a cylinder boundary. For them, whatever geometry they use should be consistent with the partition table. Note that before PATCH 03/32, vvfat's partition end was screwed up, and nobody noticed. >> Does this only matter if you declare an image with :floppy: and pass it >> as a hard disk? Then we can honestly say we don't care... > > As I understand it, the patch has two effect: > > 1. the user's hard disk geometry (-drive options cyls, heads, secs) is > now obeyed ... even when the user suppresses raw by specifying format=vvfat with file=fat:... Consistent with how all other block drivers work. > 2. the default hard disk geometry with :floppy: now matches the one > without format=vvfat Yes. > I don't care about 2. indeed, but doing 1. right would be nice. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry 2012-07-04 15:23 ` [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry Kevin Wolf 2012-07-04 16:25 ` Paolo Bonzini @ 2012-07-05 11:13 ` Markus Armbruster 1 sibling, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 11:13 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: [...] >> @@ -1067,19 +1074,16 @@ DLOG(if (stderr == NULL) { >> else >> dirname += i+1; >> >> - bs->total_sectors=bs->cyls*bs->heads*bs->secs; >> + bs->total_sectors = cyls * heads * secs; >> >> - if(init_directories(s, dirname)) >> + if (init_directories(s, dirname, heads, secs)) { >> return -1; >> + } >> >> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; >> >> if(s->first_sectors_number==0x40) >> - init_mbr(s); >> - else { >> - /* MS-DOS does not like to know about CHS (?). */ >> - bs->heads = bs->cyls = bs->secs = 0; >> - } >> + init_mbr(s, cyls, heads, secs); > > You can add braces here while touching the code. I'm not touching the if... I'll do it if you insist. ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-6-git-send-email-armbru@redhat.com>]
* Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly [not found] ` <1340984094-5451-6-git-send-email-armbru@redhat.com> @ 2012-07-05 8:29 ` Kevin Wolf 2012-07-05 9:27 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 8:29 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 29.06.2012 17:34, schrieb Markus Armbruster: > Each test litters /tmp with several files: a pid file and two > sockets. Tidy up. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Thanks! Still leaves the files around if a test case fails, but much better than before. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly 2012-07-05 8:29 ` [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly Kevin Wolf @ 2012-07-05 9:27 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 9:27 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: >> Each test litters /tmp with several files: a pid file and two >> sockets. Tidy up. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Thanks! Still leaves the files around if a test case fails, but much > better than before. I didn't have the stomach to fix that part in the middle of this series :) ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-14-git-send-email-armbru@redhat.com>]
* Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint [not found] ` <1340984094-5451-14-git-send-email-armbru@redhat.com> @ 2012-07-05 9:16 ` Kevin Wolf 2012-07-05 9:28 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 9:16 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 29.06.2012 17:34, schrieb Markus Armbruster: > When hd_geometry_guess() picks a geometry, it also picks the > appropriate translation, but only when the prior translation hint is > BIOS_ATA_TRANSLATION_AUTO. Looks wrong, because such a prior > translation would be passed to the BIOS whether it's suitable for the > geometry or not. > > Fortunately, that can't happen. There are just two ways to for the s/to// > translation hint to get set to something other than > BIOS_ATA_TRANSLATION_AUTO: drive_init() on behalf of -drive trans=..., > and hd_geometry_guess(). Both set it only when they also set a valid > geometry hint, i.e. one with a non-zero number of cylinders. > > Since hd_geometry_guess() returns right away when it finds a valid > geometry hint, translation can only be BIOS_ATA_TRANSLATION_AUTO in > the remainder of the function. > > Assert this, and simplify accordingly. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/hd-geometry.c | 17 +++++++---------- > 1 files changed, 7 insertions(+), 10 deletions(-) Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint 2012-07-05 9:16 ` [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint Kevin Wolf @ 2012-07-05 9:28 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 9:28 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: >> When hd_geometry_guess() picks a geometry, it also picks the >> appropriate translation, but only when the prior translation hint is >> BIOS_ATA_TRANSLATION_AUTO. Looks wrong, because such a prior >> translation would be passed to the BIOS whether it's suitable for the >> geometry or not. >> >> Fortunately, that can't happen. There are just two ways to for the > > s/to// Got it, thanks! [...] ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-23-git-send-email-armbru@redhat.com>]
* Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry [not found] ` <1340984094-5451-23-git-send-email-armbru@redhat.com> @ 2012-07-05 11:33 ` Kevin Wolf 2012-07-05 12:08 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 11:33 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 29.06.2012 17:34, schrieb Markus Armbruster: > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/hd-geo-test.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c > index 0ab573c..02eb5c2 100644 > --- a/tests/hd-geo-test.c > +++ b/tests/hd-geo-test.c > @@ -321,13 +321,15 @@ static void test_ide_drive_user(const char *dev, bool trans) > const chst expected_chst = { secs / (4 * 32) , 4, 32, trans }; > > argc = setup_common(argv, ARRAY_SIZE(argv)); > - opts = g_strdup_printf(",cyls=%d,heads=%d,secs=%d%s", > + opts = g_strdup_printf("%s,cyls=%d,heads=%d,secs=%d%s", > + dev && !trans ? dev : "", > expected_chst.cyls, expected_chst.heads, > expected_chst.secs, > trans ? ",trans=lba" : ""); > cur_ide[0] = &expected_chst; > argc = setup_ide(argc, argv, ARRAY_SIZE(argv), > - 0, dev, backend_small, mbr_chs, opts); > + 0, dev && !trans ? opts : NULL, backend_small, mbr_chs, > + dev && !trans ? "" : opts); > qtest_start(g_strjoinv(" ", argv)); > test_cmos(); > qtest_quit(global_qtest); I've spent more time parsing this test code than I needed for the review for most patches that touch actual code... Maybe an explicit if (dev && !trans) would help somewhat. don't know. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry 2012-07-05 11:33 ` [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry Kevin Wolf @ 2012-07-05 12:08 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 12:08 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> tests/hd-geo-test.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c >> index 0ab573c..02eb5c2 100644 >> --- a/tests/hd-geo-test.c >> +++ b/tests/hd-geo-test.c >> @@ -321,13 +321,15 @@ static void test_ide_drive_user(const char *dev, bool trans) >> const chst expected_chst = { secs / (4 * 32) , 4, 32, trans }; >> >> argc = setup_common(argv, ARRAY_SIZE(argv)); >> - opts = g_strdup_printf(",cyls=%d,heads=%d,secs=%d%s", >> + opts = g_strdup_printf("%s,cyls=%d,heads=%d,secs=%d%s", >> + dev && !trans ? dev : "", >> expected_chst.cyls, expected_chst.heads, >> expected_chst.secs, >> trans ? ",trans=lba" : ""); >> cur_ide[0] = &expected_chst; >> argc = setup_ide(argc, argv, ARRAY_SIZE(argv), >> - 0, dev, backend_small, mbr_chs, opts); >> + 0, dev && !trans ? opts : NULL, backend_small, mbr_chs, >> + dev && !trans ? "" : opts); >> qtest_start(g_strjoinv(" ", argv)); >> test_cmos(); >> qtest_quit(global_qtest); > > I've spent more time parsing this test code than I needed for the review > for most patches that touch actual code... Maybe an explicit if (dev && > !trans) would help somewhat. don't know. Sorry about that. It gets better again in PATCH 26/32. ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <1340984094-5451-33-git-send-email-armbru@redhat.com>]
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 [not found] ` <1340984094-5451-33-git-send-email-armbru@redhat.com> @ 2012-07-05 15:27 ` Kevin Wolf 2012-07-05 16:20 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-05 15:27 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 29.06.2012 17:34, schrieb Markus Armbruster: > New limits straight from ATA4 6.2 Register delivered data transfer > command sector addressing. > > I figure the old sector limit 63 was blindly copied from the BIOS > int 13 limit. Doesn't apply to the hardware. No idea where the old > cylinder limit comes from. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Now I think we have the very same thing in IDE, SCSI and virtio-blk. Would it make sense to have a helper function in hd-geometry.c that takes and validates the geometry from a BlockConf, applies defaults and puts the result into device state fields passed by reference? Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-05 15:27 ` [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 Kevin Wolf @ 2012-07-05 16:20 ` Markus Armbruster 2012-07-05 16:39 ` Markus Armbruster 2012-07-06 14:50 ` Alexander Graf 0 siblings, 2 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 16:20 UTC (permalink / raw) To: Kevin Wolf; +Cc: aliguori, stefanha, qemu-devel, Alexander Graf, pbonzini Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 17:34, schrieb Markus Armbruster: >> New limits straight from ATA4 6.2 Register delivered data transfer >> command sector addressing. >> >> I figure the old sector limit 63 was blindly copied from the BIOS >> int 13 limit. Doesn't apply to the hardware. No idea where the old >> cylinder limit comes from. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Now I think we have the very same thing in IDE, SCSI and virtio-blk. > Would it make sense to have a helper function in hd-geometry.c that > takes and validates the geometry from a BlockConf, applies defaults and > puts the result into device state fields passed by reference? I can look into this, but I'm afraid we'd need two helpers, because of IDE complications. When it comes to block device models, IDE is *always* the troublemaker. And a big reason for that is the messy data structures that are impractical to clean up while we still support non-qdevified IDE. Which we do almost three years after IDE qdevification. The laggards are: * mac99, g3beige Alexander Graf <agraf@suse.de> * spitz, borzoi, terrier Andrzej Zaborowski <balrogg@gmail.com> * tosa unmaintained * r2d Magnus Damm <magnus.damm@gmail.com> Any plans to drag these boards into the current decade already? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-05 16:20 ` Markus Armbruster @ 2012-07-05 16:39 ` Markus Armbruster 2012-07-06 8:50 ` Kevin Wolf 2012-07-06 14:50 ` Alexander Graf 1 sibling, 1 reply; 36+ messages in thread From: Markus Armbruster @ 2012-07-05 16:39 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Markus Armbruster <armbru@redhat.com> writes: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 29.06.2012 17:34, schrieb Markus Armbruster: >>> New limits straight from ATA4 6.2 Register delivered data transfer >>> command sector addressing. >>> >>> I figure the old sector limit 63 was blindly copied from the BIOS >>> int 13 limit. Doesn't apply to the hardware. No idea where the old >>> cylinder limit comes from. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Now I think we have the very same thing in IDE, SCSI and virtio-blk. >> Would it make sense to have a helper function in hd-geometry.c that >> takes and validates the geometry from a BlockConf, applies defaults and >> puts the result into device state fields passed by reference? > > I can look into this, but I'm afraid we'd need two helpers, because of > IDE complications. I'd like to try this in a follow-up series, together with a few more cleanups made possible by this series. Okay? [...] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-05 16:39 ` Markus Armbruster @ 2012-07-06 8:50 ` Kevin Wolf 2012-07-11 13:10 ` Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Kevin Wolf @ 2012-07-06 8:50 UTC (permalink / raw) To: Markus Armbruster; +Cc: pbonzini, aliguori, qemu-devel, stefanha Am 05.07.2012 18:39, schrieb Markus Armbruster: > Markus Armbruster <armbru@redhat.com> writes: > >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 29.06.2012 17:34, schrieb Markus Armbruster: >>>> New limits straight from ATA4 6.2 Register delivered data transfer >>>> command sector addressing. >>>> >>>> I figure the old sector limit 63 was blindly copied from the BIOS >>>> int 13 limit. Doesn't apply to the hardware. No idea where the old >>>> cylinder limit comes from. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> >>> Now I think we have the very same thing in IDE, SCSI and virtio-blk. >>> Would it make sense to have a helper function in hd-geometry.c that >>> takes and validates the geometry from a BlockConf, applies defaults and >>> puts the result into device state fields passed by reference? >> >> I can look into this, but I'm afraid we'd need two helpers, because of >> IDE complications. > > I'd like to try this in a follow-up series, together with a few more > cleanups made possible by this series. Okay? Fine with me. Kevin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-06 8:50 ` Kevin Wolf @ 2012-07-11 13:10 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-11 13:10 UTC (permalink / raw) To: Kevin Wolf; +Cc: pbonzini, aliguori, qemu-devel, stefanha Kevin Wolf <kwolf@redhat.com> writes: > Am 05.07.2012 18:39, schrieb Markus Armbruster: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 29.06.2012 17:34, schrieb Markus Armbruster: >>>>> New limits straight from ATA4 6.2 Register delivered data transfer >>>>> command sector addressing. >>>>> >>>>> I figure the old sector limit 63 was blindly copied from the BIOS >>>>> int 13 limit. Doesn't apply to the hardware. No idea where the old >>>>> cylinder limit comes from. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> >>>> Now I think we have the very same thing in IDE, SCSI and virtio-blk. >>>> Would it make sense to have a helper function in hd-geometry.c that >>>> takes and validates the geometry from a BlockConf, applies defaults and >>>> puts the result into device state fields passed by reference? >>> >>> I can look into this, but I'm afraid we'd need two helpers, because of >>> IDE complications. >> >> I'd like to try this in a follow-up series, together with a few more >> cleanups made possible by this series. Okay? > > Fine with me. Done: [PATCH 0/4] Cleanups around hw/block-common.h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-05 16:20 ` Markus Armbruster 2012-07-05 16:39 ` Markus Armbruster @ 2012-07-06 14:50 ` Alexander Graf 2012-07-06 18:15 ` Markus Armbruster 1 sibling, 1 reply; 36+ messages in thread From: Alexander Graf @ 2012-07-06 14:50 UTC (permalink / raw) To: Markus Armbruster; +Cc: Kevin Wolf, aliguori, stefanha, qemu-devel, pbonzini On 05.07.2012, at 18:20, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 29.06.2012 17:34, schrieb Markus Armbruster: >>> New limits straight from ATA4 6.2 Register delivered data transfer >>> command sector addressing. >>> >>> I figure the old sector limit 63 was blindly copied from the BIOS >>> int 13 limit. Doesn't apply to the hardware. No idea where the old >>> cylinder limit comes from. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Now I think we have the very same thing in IDE, SCSI and virtio-blk. >> Would it make sense to have a helper function in hd-geometry.c that >> takes and validates the geometry from a BlockConf, applies defaults and >> puts the result into device state fields passed by reference? > > I can look into this, but I'm afraid we'd need two helpers, because of > IDE complications. > > > When it comes to block device models, IDE is *always* the troublemaker. > And a big reason for that is the messy data structures that are > impractical to clean up while we still support non-qdevified IDE. Which > we do almost three years after IDE qdevification. > > The laggards are: > > * mac99, g3beige > Alexander Graf <agraf@suse.de> > > * spitz, borzoi, terrier > Andrzej Zaborowski <balrogg@gmail.com> > > * tosa > unmaintained > > * r2d > Magnus Damm <magnus.damm@gmail.com> > > Any plans to drag these boards into the current decade already? Got a pointer to a board that does it right? Alex ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 2012-07-06 14:50 ` Alexander Graf @ 2012-07-06 18:15 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-06 18:15 UTC (permalink / raw) To: Alexander Graf; +Cc: Kevin Wolf, aliguori, stefanha, qemu-devel, pbonzini Alexander Graf <agraf@suse.de> writes: > On 05.07.2012, at 18:20, Markus Armbruster wrote: [...] >> When it comes to block device models, IDE is *always* the troublemaker. >> And a big reason for that is the messy data structures that are >> impractical to clean up while we still support non-qdevified IDE. Which >> we do almost three years after IDE qdevification. >> >> The laggards are: >> >> * mac99, g3beige >> Alexander Graf <agraf@suse.de> >> >> * spitz, borzoi, terrier >> Andrzej Zaborowski <balrogg@gmail.com> >> >> * tosa >> unmaintained >> >> * r2d >> Magnus Damm <magnus.damm@gmail.com> >> >> Any plans to drag these boards into the current decade already? > > Got a pointer to a board that does it right? Any board that uses pci_cmd646_ide_init(), pci_piix[34]_ide_init(), or vt82c686b_ide_init(). These create a qdev providing an IDE bus. The non-qdevified IDE controllers use ide_init2_with_non_qdev_drives() instead. Funny: g3beige has two IDE controllers, first one isn't qdevified (created with pmac_ide_init()), second one is (created with pci_cmd646_ide_init()). Feel free to ask for more help! ^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 00/32] Disk geometry cleanup @ 2012-07-06 6:57 Markus Armbruster 2012-07-06 6:57 ` [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf Markus Armbruster 0 siblings, 1 reply; 36+ messages in thread From: Markus Armbruster @ 2012-07-06 6:57 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, blauwirbel, stefanha, pbonzini 32 patches may look discouraging, but most patches are small, and the ones that aren't just move code around. Goals of this series: 1. One more step towards a clean separation block device host and guest part. 2. Purge CHS geometry from the block layer Part I [PATCH 01-02/32]: Floppy geometry Trivial respin of prior [PATCH 0/2] Floppy geometry cleanup Part II [PATCH 03-04/32]: vvfat geometry bug fixes Part III [PATCH 05-13/32]: Clean up hard disk geometry guessing code Part IV [PATCH 14-15/32]: Clean up CMOS hard disk info setup Part V [PATCH 16-27/32]: qdev properties for disk geometry Part VI [PATCH 28-32/32]: A few more fixes and cleanups This patch series is also available at git://repo.or.cz/qemu/armbru.git tag geo-v2 v2: New hw/block-common.h (Blue & Kevin) Coding style here & there (Blue) Tracepoint parameter types (Stefan) Markus Armbruster (32): fdc: Drop broken code for user-defined floppy geometry fdc: Move floppy geometry guessing back from block.c vvfat: Fix partition table vvfat: Do not clobber the user's geometry qtest: Tidy up temporary files properly qtest: Add hard disk geometry test block: Factor bdrv_read_unthrottled() out of guess_disk_lchs() hd-geometry: Move disk geometry guessing back from block.c hd-geometry: Add tracepoints hd-geometry: Unnest conditional in hd_geometry_guess() hd-geometry: Factor out guess_chs_for_size() hd-geometry: Clean up gratuitous goto in hd_geometry_guess() hd-geometry: Clean up confusing use of prior translation hint hd-geometry: Cut out block layer translation middleman ide pc: Cut out the block layer geometry middleman blockdev: Save geometry in DriveInfo qdev: Introduce block geometry properties hd-geometry: Switch to uint32_t to match BlockConf scsi-hd: qdev properties for disk geometry virtio-blk: qdev properties for disk geometry ide: qdev properties for disk geometry qtest: Cover qdev properties for disk geometry qdev: Collect private helpers in one place qdev: New property type chs-translation ide: qdev property for BIOS CHS translation qtest: Cover qdev property for BIOS CHS translation block: Geometry and translation hints are now useless, purge them ide pc: Put hard disk info into CMOS only for hard disks qtest: Test we don't put hard disk info into CMOS for a CD-ROM hd-geometry: Compute BIOS CHS translation in one place blockdev: Drop redundant CHS validation for if=ide Relax IDE CHS limits from 16383,16,63 to 65535,16,255 block.c | 278 ++------------------------------- block.h | 41 +---- block/vvfat.c | 57 ++++--- block_int.h | 1 - blockdev.c | 24 +-- blockdev.h | 2 + hw/Makefile.objs | 2 +- hw/block-common.h | 29 ++++ hw/fdc.c | 125 +++++++++++++-- hw/fdc.h | 10 +- hw/hd-geometry.c | 157 ++++++++++++++++++ hw/ide.h | 4 +- hw/ide/core.c | 30 +++- hw/ide/internal.h | 7 +- hw/ide/qdev.c | 46 +++++- hw/pc.c | 80 ++++------ hw/qdev-properties.c | 160 ++++++++++--------- hw/qdev.h | 3 + hw/s390-virtio-bus.c | 1 + hw/scsi-disk.c | 70 ++++++--- hw/virtio-blk.c | 42 ++++- hw/virtio-pci.c | 1 + tests/Makefile | 2 + tests/hd-geo-test.c | 428 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/libqtest.c | 29 ++-- trace-events | 4 + vl.c | 2 +- 27 files changed, 1100 insertions(+), 535 deletions(-) create mode 100644 hw/block-common.h create mode 100644 hw/hd-geometry.c create mode 100644 tests/hd-geo-test.c -- 1.7.6.5 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf 2012-07-06 6:57 [Qemu-devel] [PATCH 00/32] Disk geometry cleanup Markus Armbruster @ 2012-07-06 6:57 ` Markus Armbruster 0 siblings, 0 replies; 36+ messages in thread From: Markus Armbruster @ 2012-07-06 6:57 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, blauwirbel, stefanha, pbonzini Best to use the same type, to avoid unwanted truncation or sign extension. BlockConf can't use plain int for cyls, heads and secs, because integer properties require an exact width. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/block-common.h | 2 +- hw/hd-geometry.c | 4 ++-- hw/ide/core.c | 2 +- hw/scsi-disk.c | 2 +- hw/virtio-blk.c | 2 +- trace-events | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hw/block-common.h b/hw/block-common.h index bba817a..2f65186 100644 --- a/hw/block-common.h +++ b/hw/block-common.h @@ -16,7 +16,7 @@ /* Hard disk geometry */ void hd_geometry_guess(BlockDriverState *bs, - int *pcyls, int *pheads, int *psecs, + uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs, int *ptrans); #endif diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c index 4d746b7..7626cbb 100644 --- a/hw/hd-geometry.c +++ b/hw/hd-geometry.c @@ -98,7 +98,7 @@ static int guess_disk_lchs(BlockDriverState *bs, } static void guess_chs_for_size(BlockDriverState *bs, - int *pcyls, int *pheads, int *psecs) + uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs) { uint64_t nb_sectors; int cylinders; @@ -117,7 +117,7 @@ static void guess_chs_for_size(BlockDriverState *bs, } void hd_geometry_guess(BlockDriverState *bs, - int *pcyls, int *pheads, int *psecs, + uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs, int *ptrans) { int cylinders, heads, secs, translation; diff --git a/hw/ide/core.c b/hw/ide/core.c index 7f5ad07..f1966e3 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1927,7 +1927,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, const char *version, const char *serial, const char *model, uint64_t wwn) { - int cylinders, heads, secs; + uint32_t cylinders, heads, secs; uint64_t nb_sectors; s->bs = bs; diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index ecc9d05..bb758f2 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -927,7 +927,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf, }; BlockDriverState *bdrv = s->qdev.conf.bs; - int cylinders, heads, secs; + uint32_t cylinders, heads, secs; uint8_t *p = *p_outbuf; if ((mode_sense_valid[page] & (1 << s->qdev.type)) == 0) { diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index d2709a7..4344e28 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -590,7 +590,7 @@ static const BlockDevOps virtio_block_ops = { VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk) { VirtIOBlock *s; - int cylinders, heads, secs; + uint32_t cylinders, heads, secs; static int virtio_blk_id; DriveInfo *dinfo; diff --git a/trace-events b/trace-events index cb1df3e..b0440ae 100644 --- a/trace-events +++ b/trace-events @@ -143,7 +143,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x # hw/hd-geometry.c hd_geometry_lchs_guess(void *bs, int cyls, int heads, int secs) "bs %p LCHS %d %d %d" -hd_geometry_guess(void *bs, int cyls, int heads, int secs, int trans) "bs %p CHS %d %d %d trans %d" +hd_geometry_guess(void *bs, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "bs %p CHS %u %u %u trans %d" # hw/jazz-led.c jazz_led_read(uint64_t addr, uint8_t val) "read addr=0x%"PRIx64": 0x%x" -- 1.7.6.5 ^ permalink raw reply related [flat|nested] 36+ messages in thread
end of thread, other threads:[~2012-07-11 13:10 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1340984094-5451-1-git-send-email-armbru@redhat.com> [not found] ` <1340984094-5451-19-git-send-email-armbru@redhat.com> 2012-07-02 12:55 ` [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf Stefan Hajnoczi 2012-07-02 13:07 ` Andreas Färber 2012-07-02 14:15 ` Markus Armbruster 2012-07-02 14:34 ` Stefan Hajnoczi 2012-07-03 19:11 ` Blue Swirl 2012-07-03 20:15 ` Andreas Färber 2012-07-04 16:19 ` Paolo Bonzini 2012-07-04 16:36 ` Eric Blake 2012-07-05 18:37 ` Blue Swirl 2012-07-05 18:30 ` Blue Swirl [not found] ` <1340984094-5451-9-git-send-email-armbru@redhat.com> [not found] ` <CAAu8pHsKC6QMrhjUADiXT8DUrhy9HzbkN8pbYeWjM2aAE38ZwQ@mail.gmail.com> [not found] ` <m3k3ypmjqu.fsf@blackfin.pond.sub.org> 2012-07-03 18:40 ` [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c Blue Swirl 2012-07-04 8:24 ` Kevin Wolf [not found] ` <1340984094-5451-4-git-send-email-armbru@redhat.com> [not found] ` <CAAu8pHtQORnuc2G8DiWTt1QcWOKbp7L8WJ=P7HMMZCNV7L-NRQ@mail.gmail.com> 2012-07-04 15:17 ` [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table Kevin Wolf 2012-07-05 9:23 ` Markus Armbruster 2012-07-05 9:55 ` Kevin Wolf 2012-07-05 11:10 ` Markus Armbruster 2012-07-05 11:14 ` Kevin Wolf [not found] ` <1340984094-5451-5-git-send-email-armbru@redhat.com> 2012-07-04 15:23 ` [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry Kevin Wolf 2012-07-04 16:25 ` Paolo Bonzini 2012-07-05 7:06 ` Kevin Wolf 2012-07-05 9:16 ` Markus Armbruster 2012-07-05 11:13 ` Markus Armbruster [not found] ` <1340984094-5451-6-git-send-email-armbru@redhat.com> 2012-07-05 8:29 ` [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly Kevin Wolf 2012-07-05 9:27 ` Markus Armbruster [not found] ` <1340984094-5451-14-git-send-email-armbru@redhat.com> 2012-07-05 9:16 ` [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint Kevin Wolf 2012-07-05 9:28 ` Markus Armbruster [not found] ` <1340984094-5451-23-git-send-email-armbru@redhat.com> 2012-07-05 11:33 ` [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry Kevin Wolf 2012-07-05 12:08 ` Markus Armbruster [not found] ` <1340984094-5451-33-git-send-email-armbru@redhat.com> 2012-07-05 15:27 ` [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255 Kevin Wolf 2012-07-05 16:20 ` Markus Armbruster 2012-07-05 16:39 ` Markus Armbruster 2012-07-06 8:50 ` Kevin Wolf 2012-07-11 13:10 ` Markus Armbruster 2012-07-06 14:50 ` Alexander Graf 2012-07-06 18:15 ` Markus Armbruster 2012-07-06 6:57 [Qemu-devel] [PATCH 00/32] Disk geometry cleanup Markus Armbruster 2012-07-06 6:57 ` [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf Markus Armbruster
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.