Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[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.

Stefan

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[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.

>
> Stefan
>

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Paolo Bonzini]

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.).

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Blue Swirl]

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
>
>
>

Re: [Qemu-devel] [PATCH 18/32] hd-geometry: Switch to uint32_t to match BlockConf

[Blue Swirl]

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

Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c

[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.

>
>>> +
>>>  #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.
>
> [...]

Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[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?

>>> +    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.

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[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:
>>>>> 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.

[...]

Re: [Qemu-devel] [PATCH 03/32] vvfat: Fix partition table

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[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...

Paolo

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 05/32] qtest: Tidy up temporary files properly

[Markus Armbruster]

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
:)

Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 13/32] hd-geometry: Clean up confusing use of prior translation hint

[Markus Armbruster]

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!

[...]

Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 22/32] qtest: Cover qdev properties for disk geometry

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[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?

[...]

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 32/32] Relax IDE CHS limits from 16383, 16, 63 to 65535, 16, 255

[Markus Armbruster]

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!

[Qemu-devel] [PATCH 00/32] Disk geometry cleanup

[Markus Armbruster]

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

[Qemu-devel] [PATCH 04/32] vvfat: Do not clobber the user's geometry

[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>
---
 block/vvfat.c |   52 ++++++++++++++++++++++++++++------------------------
 1 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index e2b83a2..07d637b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -359,11 +359,12 @@ typedef struct BDRVVVFATState {
  * if the position is outside the specified geometry, fill maximum value for CHS
  * and return 1 to signal overflow.
  */
-static int sector2CHS(BlockDriverState* bs, mbr_chs_t * chs, int spos){
+static int sector2CHS(mbr_chs_t *chs, int spos, int cyls, int heads, int secs)
+{
     int head,sector;
-    sector   = spos % (bs->secs);  spos/= bs->secs;
-    head     = spos % (bs->heads); spos/= bs->heads;
-    if(spos >= bs->cyls){
+    sector   = spos % secs;  spos /= secs;
+    head     = spos % heads; spos /= heads;
+    if (spos >= cyls) {
         /* Overflow,
         it happens if 32bit sector positions are used, while CHS is only 24bit.
         Windows/Dos is said to take 1023/255/63 as nonrepresentable CHS */
@@ -378,7 +379,7 @@ static int sector2CHS(BlockDriverState* bs, mbr_chs_t * chs, int spos){
     return 0;
 }
 
-static void init_mbr(BDRVVVFATState* s)
+static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
 {
     /* TODO: if the files mbr.img and bootsect.img exist, use them */
     mbr_t* real_mbr=(mbr_t*)s->first_sectors;
@@ -393,8 +394,10 @@ static void init_mbr(BDRVVVFATState* s)
     partition->attributes=0x80; /* bootable */
 
     /* 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->bs->total_sectors - 1);
+    lba  = sector2CHS(&partition->start_CHS, s->first_sectors_number - 1,
+                     cyls, heads, secs);
+    lba |= sector2CHS(&partition->end_CHS,   s->bs->total_sectors - 1,
+                     cyls, heads, secs);
 
     /*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);
@@ -831,7 +834,7 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
 }
 
 static int init_directories(BDRVVVFATState* s,
-	const char* dirname)
+                            const char *dirname, int heads, int secs)
 {
     bootsector_t* bootsector;
     mapping_t* mapping;
@@ -958,8 +961,8 @@ static int init_directories(BDRVVVFATState* s,
     bootsector->media_type=(s->first_sectors_number>1?0xf8:0xf0); /* media descriptor (f8=hd, f0=3.5 fd)*/
     s->fat.pointer[0] = bootsector->media_type;
     bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat);
-    bootsector->sectors_per_track=cpu_to_le16(s->bs->secs);
-    bootsector->number_of_heads=cpu_to_le16(s->bs->heads);
+    bootsector->sectors_per_track = cpu_to_le16(secs);
+    bootsector->number_of_heads = cpu_to_le16(heads);
     bootsector->hidden_sectors=cpu_to_le32(s->first_sectors_number==1?0:0x3f);
     bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0);
 
@@ -992,7 +995,7 @@ static void vvfat_rebind(BlockDriverState *bs)
 static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
-    int i;
+    int i, cyls, heads, secs;
 
 #ifdef DEBUG
     vvv = s;
@@ -1034,24 +1037,28 @@ DLOG(if (stderr == NULL) {
 	/* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
 	if (!s->fat_type) {
 	    s->fat_type = 12;
-	    bs->secs = 36;
+            secs = 36;
 	    s->sectors_per_cluster=2;
 	} else {
-	    bs->secs=(s->fat_type == 12 ? 18 : 36);
+            secs = s->fat_type == 12 ? 18 : 36;
 	    s->sectors_per_cluster=1;
 	}
 	s->first_sectors_number = 1;
-	bs->cyls=80; bs->heads=2;
+        cyls = 80;
+        heads = 2;
     } else {
 	/* 32MB or 504MB disk*/
 	if (!s->fat_type) {
 	    s->fat_type = 16;
 	}
-	bs->cyls=(s->fat_type == 12 ? 64 : 1024);
-	bs->heads=16; bs->secs=63;
+        cyls = s->fat_type == 12 ? 64 : 1024;
+        heads = 16;
+        secs = 63;
     }
+    fprintf(stderr, "vvfat %s chs %d,%d,%d\n",
+            dirname, cyls, heads, secs);
 
-    s->sector_count=bs->cyls*bs->heads*bs->secs-(s->first_sectors_number-1);
+    s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
 
     if (strstr(dirname, ":rw:")) {
 	if (enable_write_target(s))
@@ -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);
 
     //    assert(is_consistent(s));
     qemu_co_mutex_init(&s->lock);
-- 
1.7.6.5