Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Markus Armbruster]

Peter Maydell  writes:

> On 7 January 2016 at 22:43, Max Reitz  wrote:
>> I hope that the above explanation helped you understand why it bled into
>> tray-less devices, from a technical perspective.
>
> Yes, thanks, that was definitely a helpful explanation for why
> the design is the way it is. I'm still not sure how useful it
> is to model "floppy is in the drive slot but not pushed in"
> (we don't model "floppy is sat on my kitchen table", which
> is also a situation that a real floppy disk could be in ;-)).
> But I can see the attraction of trying to separate commands
> operating on the device from commands operating on the backend.

Hysterical raisins, as usual.

The modelling of devices with removable media evolved to do the right
thing for CD-ROMs with a tray-type loading mechanism.  There are others,
notably slot loading.  Anyway, this model has been "close enough" for
all devices, or at least nobody got bothered enough to complicate the
model with additional cases.

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 22:43, Max Reitz  wrote:
> I hope that the above explanation helped you understand why it bled into
> tray-less devices, from a technical perspective.

Yes, thanks, that was definitely a helpful explanation for why
the design is the way it is. I'm still not sure how useful it
is to model "floppy is in the drive slot but not pushed in"
(we don't model "floppy is sat on my kitchen table", which
is also a situation that a real floppy disk could be in ;-)).
But I can see the attraction of trying to separate commands
operating on the device from commands operating on the backend.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 19:37, Max Reitz  wrote:
> Compare floppy disks, for which we now have a "virtual" tray status:
> Whenever a medium is inserted, the "tray" is considered closed.
> Otherwise, it is open. This works pretty much like a physical tray would
> work; whenever the tray is closed, you cannot exchange the medium, but
> when it is open, you can.
>
> There is only one difference to devices which actually have a tray: For
> floppy disks, you cannot have a closed tray without a medium in it.
>
> Anyway, we can implement the same model for SD cards. I'll see to it.

It looks like sd.c is the only one which implements a change_media_cb
but no is_tray_open, but it would be nice if we could implement this
in the default blk_dev_is_tray_open() method rather than in the
sd and floppy models (ie if I don't implement tray-open then the
tray is closed if there's a medium attached, and the block backend
ought to know if there's a medium attached itself already).

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 10 November 2015 at 14:09, Kevin Wolf  wrote:
> From: Max Reitz 
>
> Implement 'change' on block devices by calling blockdev-open-tray,
> blockdev-remove-medium, blockdev-insert-medium (a variation of that
> which does not need a node-name) and blockdev-close-tray.
>
> Signed-off-by: Max Reitz 
> Signed-off-by: Kevin Wolf 

I think this commit broke the monitor 'change' command for
sd card devices. In 2.4, for a Zaurus ('spitz') machine:
change sd0 /home/petmay01/test-images/RaspberryPi/pifi-4g.img

causes the guest to print
mmc0: new SDHC card at address 4567
mmcblk0: mmc0:4567 QEMU! 4.00 GiB
 mmcblk0: p1 p2 p3 p4

ie it detects we have just provided a new SD card.

In 2.5 trying this gives an error in the monitor:
Tray of device 'sd0' is not open

This seems to be because with this commit we now try to do
a qmp_blockdev_open_tray() on the device. This fails for SD cards
(which don't have any kind of tray in real life), because we
don't implement the is_tray_open hook and so get the default
"tray always closed" behaviour. But in the old code you could
perfectly well change the backing medium even on a device
without a tray...

Was this breakage intentional?

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

On 07.01.2016 22:42, Peter Maydell wrote:
> On 7 January 2016 at 20:14, Max Reitz  wrote:
>> On 07.01.2016 20:56, Peter Maydell wrote:
>>> It looks like sd.c is the only one which implements a change_media_cb
>>> but no is_tray_open, but it would be nice if we could implement this
>>> in the default blk_dev_is_tray_open() method rather than in the
>>> sd and floppy models (ie if I don't implement tray-open then the
>>> tray is closed if there's a medium attached, and the block backend
>>> ought to know if there's a medium attached itself already).
>>
>> That would be nice, but there's a difference between "there's a medium
>> attached" (tray can be both open and closed) and "the medium is
>> accessible by the guest" (tray must be closed). The BlockBackend does
>> not know this difference, only the guest devices does.
>>
>> It gets told of when to open/close the tray by invocation of the
>> change_media_cb() (the @load parameter set to false or true,
>> respectively), and we could track this state in the BlockBackend instead
>> of in the SDState. But that looks like the wrong place to me.
> 
> Well, previously sd.c didn't need to have any state for this
> to all work right (or indeed care about implementing a fake
> tray status for a device that doesn't have a tray), so it seems
> odd that we need to invent some extra state for it to work now.

Before, it took the state from the block layer by calling
blk_is_inserted(). Works fine as long we only have the high-level
operations change and eject. Stops to work once we introduce lower-level
operations (open-tray, remove-medium, insert-medium, close-tray).

Why do we need the low-level operations? Mainly because they integrate
much better into the model of a more freely configurable block layer
(there aren't many options one can give to the 'change' operation; now
you can use blockdev-add and the lower-level operations afterwards).

Why did I reimplement 'change' and 'eject' using these new operations?
Because otherwise we'd have those operations doing one kind of thing,
and the lower-level ones doing something completely different. I'd find
that bad.

>> Right now, sd.c completely ignores the @load parameter of
>> change_media_cb(), which seems wrong; this means that "opening the tray"
>> keeps the medium accessible for the guest. In the past that was fine,
>> because eject closed the associated BDS (the medium) right afterwards,
>> so it actually wasn't accessible any more. The new
>> blockdev-open-tray no longer does that, so now we need to respect the
>> value of @load and no longer rely on blk_is_inserted() alone any more.
> 
> I think blockdev-open-tray should probably not work for SD cards
> (or for floppies?), because saying "open the tray" on a device
> without a tray is meaningless.

Depends on what you think it is. For me, blockdev-open-tray generally
means "pushing the eject button".

For actual tray devices, this means that the tray may open (or the OS
may be asked to do so). For floppy disk drives, this means the floppy
disk will be ejected. For SD slots (where there often is no dedicated
button, but one can push the SD card further in for it to get released),
this means the SD card will be ejected.

Note that this is different from the 'eject' command which models a
drive where the user pushes the eject button and suddenly the medium
bursts into flames/is dropped on the floor/just disappears. Therefore, I
find the 'eject' command quite misnamed, but having named the new
command 'blockdev-eject-medium' wouldn't have made things any better.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 20:14, Max Reitz  wrote:
> On 07.01.2016 20:56, Peter Maydell wrote:
>> It looks like sd.c is the only one which implements a change_media_cb
>> but no is_tray_open, but it would be nice if we could implement this
>> in the default blk_dev_is_tray_open() method rather than in the
>> sd and floppy models (ie if I don't implement tray-open then the
>> tray is closed if there's a medium attached, and the block backend
>> ought to know if there's a medium attached itself already).
>
> That would be nice, but there's a difference between "there's a medium
> attached" (tray can be both open and closed) and "the medium is
> accessible by the guest" (tray must be closed). The BlockBackend does
> not know this difference, only the guest devices does.
>
> It gets told of when to open/close the tray by invocation of the
> change_media_cb() (the @load parameter set to false or true,
> respectively), and we could track this state in the BlockBackend instead
> of in the SDState. But that looks like the wrong place to me.

Well, previously sd.c didn't need to have any state for this
to all work right (or indeed care about implementing a fake
tray status for a device that doesn't have a tray), so it seems
odd that we need to invent some extra state for it to work now.

> Right now, sd.c completely ignores the @load parameter of
> change_media_cb(), which seems wrong; this means that "opening the tray"
> keeps the medium accessible for the guest. In the past that was fine,
> because eject closed the associated BDS (the medium) right afterwards,
> so it actually wasn't accessible any more. The new
> blockdev-open-tray no longer does that, so now we need to respect the
> value of @load and no longer rely on blk_is_inserted() alone any more.

I think blockdev-open-tray should probably not work for SD cards
(or for floppies?), because saying "open the tray" on a device
without a tray is meaningless.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Peter Maydell]

On 7 January 2016 at 21:57, Max Reitz  wrote:
> On 07.01.2016 22:42, Peter Maydell wrote:
>> Well, previously sd.c didn't need to have any state for this
>> to all work right (or indeed care about implementing a fake
>> tray status for a device that doesn't have a tray), so it seems
>> odd that we need to invent some extra state for it to work now.
>
> Before, it took the state from the block layer by calling
> blk_is_inserted(). Works fine as long we only have the high-level
> operations change and eject. Stops to work once we introduce lower-level
> operations (open-tray, remove-medium, insert-medium, close-tray).
>
> Why do we need the low-level operations? Mainly because they integrate
> much better into the model of a more freely configurable block layer
> (there aren't many options one can give to the 'change' operation; now
> you can use blockdev-add and the lower-level operations afterwards).
>
> Why did I reimplement 'change' and 'eject' using these new operations?
> Because otherwise we'd have those operations doing one kind of thing,
> and the lower-level ones doing something completely different. I'd find
> that bad.

But these lower level operations now let you put the system
conceptually into states that it really can't be in in hardware.
That seems wrong to me and we should fail those commands where
it makes sense (eg trying to "open a tray" when there is no tray),
rather than forcing the sd.c layer code to cope with the user
putting the system into impossible conditions.

>>> Right now, sd.c completely ignores the @load parameter of
>>> change_media_cb(), which seems wrong; this means that "opening the tray"
>>> keeps the medium accessible for the guest. In the past that was fine,
>>> because eject closed the associated BDS (the medium) right afterwards,
>>> so it actually wasn't accessible any more. The new
>>> blockdev-open-tray no longer does that, so now we need to respect the
>>> value of @load and no longer rely on blk_is_inserted() alone any more.
>>
>> I think blockdev-open-tray should probably not work for SD cards
>> (or for floppies?), because saying "open the tray" on a device
>> without a tray is meaningless.
>
> Depends on what you think it is. For me, blockdev-open-tray generally
> means "pushing the eject button".
>
> For actual tray devices, this means that the tray may open (or the OS
> may be asked to do so). For floppy disk drives, this means the floppy
> disk will be ejected. For SD slots (where there often is no dedicated
> button, but one can push the SD card further in for it to get released),
> this means the SD card will be ejected.
>
> Note that this is different from the 'eject' command which models a
> drive where the user pushes the eject button and suddenly the medium
> bursts into flames/is dropped on the floor/just disappears. Therefore, I
> find the 'eject' command quite misnamed, but having named the new
> command 'blockdev-eject-medium' wouldn't have made things any better.

I don't understand what the difference is between your
"floppy disk will be ejected" and "SD card will be ejected"
cases, and "medium is dropped on the floor" case. Those seem
like exactly the same thing to me, so we should just use "eject"
and not have an extra command.

I can see why we want to model tray state for devices with trays,
but I don't see why this is bleeding into devices without trays.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

On 07.01.2016 23:19, Peter Maydell wrote:
> On 7 January 2016 at 21:57, Max Reitz  wrote:
>> On 07.01.2016 22:42, Peter Maydell wrote:
>>> Well, previously sd.c didn't need to have any state for this
>>> to all work right (or indeed care about implementing a fake
>>> tray status for a device that doesn't have a tray), so it seems
>>> odd that we need to invent some extra state for it to work now.
>>
>> Before, it took the state from the block layer by calling
>> blk_is_inserted(). Works fine as long we only have the high-level
>> operations change and eject. Stops to work once we introduce lower-level
>> operations (open-tray, remove-medium, insert-medium, close-tray).
>>
>> Why do we need the low-level operations? Mainly because they integrate
>> much better into the model of a more freely configurable block layer
>> (there aren't many options one can give to the 'change' operation; now
>> you can use blockdev-add and the lower-level operations afterwards).
>>
>> Why did I reimplement 'change' and 'eject' using these new operations?
>> Because otherwise we'd have those operations doing one kind of thing,
>> and the lower-level ones doing something completely different. I'd find
>> that bad.
> 
> But these lower level operations now let you put the system
> conceptually into states that it really can't be in in hardware.
> That seems wrong to me and we should fail those commands where
> it makes sense (eg trying to "open a tray" when there is no tray),
> rather than forcing the sd.c layer code to cope with the user
> putting the system into impossible conditions.

Hm, OK. So I assume you are proposing just not doing any *-tray
operations on devices without a tray? I personally still consider
"pushing the eject button" and "actually taking the medium out of the
slot" to distinct operations, but I think I could live with that.

Technically, the block layer should handle that just fine.

 Right now, sd.c completely ignores the @load parameter of
 change_media_cb(), which seems wrong; this means that "opening the tray"
 keeps the medium accessible for the guest. In the past that was fine,
 because eject closed the associated BDS (the medium) right afterwards,
 so it actually wasn't accessible any more. The new
 blockdev-open-tray no longer does that, so now we need to respect the
 value of @load and no longer rely on blk_is_inserted() alone any more.
>>>
>>> I think blockdev-open-tray should probably not work for SD cards
>>> (or for floppies?), because saying "open the tray" on a device
>>> without a tray is meaningless.
>>
>> Depends on what you think it is. For me, blockdev-open-tray generally
>> means "pushing the eject button".
>>
>> For actual tray devices, this means that the tray may open (or the OS
>> may be asked to do so). For floppy disk drives, this means the floppy
>> disk will be ejected. For SD slots (where there often is no dedicated
>> button, but one can push the SD card further in for it to get released),
>> this means the SD card will be ejected.
>>
>> Note that this is different from the 'eject' command which models a
>> drive where the user pushes the eject button and suddenly the medium
>> bursts into flames/is dropped on the floor/just disappears. Therefore, I
>> find the 'eject' command quite misnamed, but having named the new
>> command 'blockdev-eject-medium' wouldn't have made things any better.
> 
> I don't understand what the difference is between your
> "floppy disk will be ejected" and "SD card will be ejected"
> cases, and "medium is dropped on the floor" case. Those seem
> like exactly the same thing to me, so we should just use "eject"
> and not have an extra command.

From the guest's perspective, you are right, there is no difference. I
do see a difference from the user's perspective, but I think you are
right in that that difference may not actually matter to anyone.

The difference is the following (let's use a floppy disk drive, because
there are all kinds of SD slots in common use): On a physical device,
you push the eject button and the disk comes out. You can then just push
the disk back in, or you can remove it, and maybe replace it.

Using 'eject', you cannot push the disk back in. It's gone. You'll have
to find a new one to insert. Yes, this is a use case that rarely anybody
ever needs, but it still is a difference from what physical hardware does.

Using 'blockdev-open-tray' means pushing the eject button. You can use
'blockdev-close-tray' to push the medium back in. Or you invoke
'(x-)blockdev-remove-medium' to remove it and
'(x-)blockdev-insert-medium' to insert a new one (which you'd then push
in using 'blockdev-close-tray').

Technically, what happens is this: The *-tray commands are for the
device model. blockdev-close-tray invokes the change_media_cb with
load=false; blockdev-open-tray invokes it with load=true. The general
block layer doesn't do anything here.

The *-medium commands are for the block layer.