[Qemu-devel] hw/nand hw/onenand and read-only

[Qemu-devel] hw/nand hw/onenand and read-only

[Markus Armbruster]

These guys have been converted to qdev relatively recently.

I wonder what happens when they use a drive defined with "-drive
if=none,readonly".

If that's not a valid configuration, we better reject read-only drives,
like ide_init_drive() does.

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Juha.Riihimaki]

On 18.10.11 11:23 , "ext Markus Armbruster" <armbru@redhat.com> wrote:

>These guys have been converted to qdev relatively recently.
>
>I wonder what happens when they use a drive defined with "-drive
>if=none,readonly".
>
>If that's not a valid configuration, we better reject read-only drives,
>like ide_init_drive() does.

I'm not an expert with QEMU command line options; how do you pass such a
drive to a NAND device? With "if=mtd" I get the following which I guess is
expected result:

$ qemu-system-arm -M beagle -drive if=mtd,file=nand.img,readonly
qemu: readonly flag not supported for drive with this interface


Regards,
Juha

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Markus Armbruster]

<Juha.Riihimaki@nokia.com> writes:

> On 18.10.11 11:23 , "ext Markus Armbruster" <armbru@redhat.com> wrote:
>
>>These guys have been converted to qdev relatively recently.
>>
>>I wonder what happens when they use a drive defined with "-drive
>>if=none,readonly".
>>
>>If that's not a valid configuration, we better reject read-only drives,
>>like ide_init_drive() does.
>
> I'm not an expert with QEMU command line options; how do you pass such a
> drive to a NAND device? With "if=mtd" I get the following which I guess is
> expected result:
>
> $ qemu-system-arm -M beagle -drive if=mtd,file=nand.img,readonly
> qemu: readonly flag not supported for drive with this interface

Yes, that way works:

    $ qemu-system-arm -drive if=mtd,file=tmp.qcow2,readonly
    qemu-system-arm: -drive if=mtd,file=tmp.qcow2,readonly: readonly not supported by this bus type

But try this way:

    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo
    Kernel image must be specified

Grr, force it:

    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo -kernel /dev/null
    qemu: hardware error: nand_device_init: Unsupported NAND block size.
    [...]

Note that I didn't bother supplying a drive with sensible size.  If I
did, I guess I'd get nand coupled to a read-only drive.  Easy enough for
you to try :)

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Peter Maydell]

On 18 October 2011 14:38, Markus Armbruster <armbru@redhat.com> wrote:
> Yes, that way works:
>
>    $ qemu-system-arm -drive if=mtd,file=tmp.qcow2,readonly
>    qemu-system-arm: -drive if=mtd,file=tmp.qcow2,readonly: readonly not supported by this bus type
>
> But try this way:
>
>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo
>    Kernel image must be specified
>
> Grr, force it:
>
>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo -kernel /dev/null
>    qemu: hardware error: nand_device_init: Unsupported NAND block size.
>    [...]
>
> Note that I didn't bother supplying a drive with sensible size.  If I
> did, I guess I'd get nand coupled to a read-only drive.  Easy enough for
> you to try :)

Isn't this a block layer problem? It seems to be blockdev.c that
emits the "readonly not supported" message for MTD devices, so presumably
it ought to do so however the user sets up the device...

-- PMM

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 18 October 2011 14:38, Markus Armbruster <armbru@redhat.com> wrote:
>> Yes, that way works:
>>
>>    $ qemu-system-arm -drive if=mtd,file=tmp.qcow2,readonly
>>    qemu-system-arm: -drive if=mtd,file=tmp.qcow2,readonly: readonly not supported by this bus type
>>
>> But try this way:
>>
>>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo
>>    Kernel image must be specified
>>
>> Grr, force it:
>>
>>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo -device nand,drive=foo -kernel /dev/null
>>    qemu: hardware error: nand_device_init: Unsupported NAND block size.
>>    [...]
>>
>> Note that I didn't bother supplying a drive with sensible size.  If I
>> did, I guess I'd get nand coupled to a read-only drive.  Easy enough for
>> you to try :)
>
> Isn't this a block layer problem? It seems to be blockdev.c that
> emits the "readonly not supported" message for MTD devices, so presumably
> it ought to do so however the user sets up the device...

Only the device knows what kind of drive it needs.  The block layer can
only find out by asking the device.  There's no way to ask yet.  It
could be added to BlockDevOps, and checked in bdrv_attach_dev().

The existing readonly check in blockdev.c is a layering violation.  It
predates qdev, and it can't be cleanly extended to cover if=none.

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Juha.Riihimaki]

On 18.10.11 16:38 , "ext Markus Armbruster" <armbru@redhat.com> wrote:

>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo
>-device nand,drive=foo -kernel /dev/null
>    qemu: hardware error: nand_device_init: Unsupported NAND block size.
>    [...]
>
>Note that I didn't bother supplying a drive with sensible size.  If I
>did, I guess I'd get nand coupled to a read-only drive.  Easy enough for
>you to try :)

Indeed, thanks ;) While I'm at it, I'll also fix the NAND init function to
return -1 instead of aborting. I'll send patches for hw/nand and
hw/onenand shortly. I'll simply make them reject read-only drives however
both device models already support running without a drive as well by
using a memory buffer instead so it would also be possible to make them
use a read-only drive in a way that initial NAND/OneNAND contents would be
read from the drive but any changes would not be written back to the drive
and would be lost when QEMU is killed.


Cheers,
Juha

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Markus Armbruster]

<Juha.Riihimaki@nokia.com> writes:

> On 18.10.11 16:38 , "ext Markus Armbruster" <armbru@redhat.com> wrote:
>
>>    $ qemu-system-arm -drive if=none,file=tmp.qcow2,readonly,id=foo
>>-device nand,drive=foo -kernel /dev/null
>>    qemu: hardware error: nand_device_init: Unsupported NAND block size.
>>    [...]
>>
>>Note that I didn't bother supplying a drive with sensible size.  If I
>>did, I guess I'd get nand coupled to a read-only drive.  Easy enough for
>>you to try :)
>
> Indeed, thanks ;) While I'm at it, I'll also fix the NAND init function to
> return -1 instead of aborting. I'll send patches for hw/nand and
> hw/onenand shortly. I'll simply make them reject read-only drives however

Thanks!

> both device models already support running without a drive as well by
> using a memory buffer instead so it would also be possible to make them
> use a read-only drive in a way that initial NAND/OneNAND contents would be
> read from the drive but any changes would not be written back to the drive
> and would be lost when QEMU is killed.

Sounds like it could be useful, but it's not what I'd expect for
"readonly".

You could create a boolean device property to make memory contents
transient rather than persistent.  Then reject read-only drives only in
persistent mode, i.e. when the property is false.  Feels cleaner to me.

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Peter Maydell]

On 19 October 2011 09:03, Markus Armbruster <armbru@redhat.com> wrote:
> <Juha.Riihimaki@nokia.com> writes:
>> both device models already support running without a drive as well by
>> using a memory buffer instead so it would also be possible to make them
>> use a read-only drive in a way that initial NAND/OneNAND contents would be
>> read from the drive but any changes would not be written back to the drive
>> and would be lost when QEMU is killed.
>
> Sounds like it could be useful, but it's not what I'd expect for
> "readonly".
>
> You could create a boolean device property to make memory contents
> transient rather than persistent.  Then reject read-only drives only in
> persistent mode, i.e. when the property is false.  Feels cleaner to me.

That doesn't sound very onenand/nand specific to me, though.

-- PMM

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Kevin Wolf]

Am 19.10.2011 10:46, schrieb Peter Maydell:
> On 19 October 2011 09:03, Markus Armbruster <armbru@redhat.com> wrote:
>> <Juha.Riihimaki@nokia.com> writes:
>>> both device models already support running without a drive as well by
>>> using a memory buffer instead so it would also be possible to make them
>>> use a read-only drive in a way that initial NAND/OneNAND contents would be
>>> read from the drive but any changes would not be written back to the drive
>>> and would be lost when QEMU is killed.
>>
>> Sounds like it could be useful, but it's not what I'd expect for
>> "readonly".
>>
>> You could create a boolean device property to make memory contents
>> transient rather than persistent.  Then reject read-only drives only in
>> persistent mode, i.e. when the property is false.  Feels cleaner to me.
> 
> That doesn't sound very onenand/nand specific to me, though.

And in fact, you already get this with -drive snapshot=on (but still
leaving the drive r/w)

Kevin

Re: [Qemu-devel] hw/nand hw/onenand and read-only

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 19 October 2011 09:03, Markus Armbruster <armbru@redhat.com> wrote:
>> <Juha.Riihimaki@nokia.com> writes:
>>> both device models already support running without a drive as well by
>>> using a memory buffer instead so it would also be possible to make them
>>> use a read-only drive in a way that initial NAND/OneNAND contents would be
>>> read from the drive but any changes would not be written back to the drive
>>> and would be lost when QEMU is killed.
>>
>> Sounds like it could be useful, but it's not what I'd expect for
>> "readonly".
>>
>> You could create a boolean device property to make memory contents
>> transient rather than persistent.  Then reject read-only drives only in
>> persistent mode, i.e. when the property is false.  Feels cleaner to me.
>
> That doesn't sound very onenand/nand specific to me, though.

Better ideas welcome :)

First thing to decide is whether it's a property of the device or one of
its backend.

Similar devices have similar properties.  Sometimes we factor out common
ones.  See for instance DEFINE_BLOCK_PROPERTIES().