[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status
Date: Wed, 04 Mar 2015 17:06:10 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 2015-03-04 at 09:00, Kevin Wolf wrote:
Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
The tray of an FDD is open iff there is no medium inserted (there are
only two states for an FDD: "medium inserted" or "no medium inserted").

This results in the tray being reported as open if qemu has been started
with the default floppy drive, which breaks some tests. Fix them.

Signed-off-by: Max Reitz <address@hidden>
  hw/block/fdc.c             | 20 +++++++++++++---
  tests/fdc-test.c           |  4 +---
  tests/qemu-iotests/067.out | 60 +++++++---------------------------------------
  tests/qemu-iotests/071.out |  2 --
  tests/qemu-iotests/081.out |  1 -
  tests/qemu-iotests/087.out |  6 -----
  6 files changed, 26 insertions(+), 67 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2bf87c9..0c5a6b4 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -192,6 +192,8 @@ typedef struct FDrive {
      uint8_t ro;               /* Is read-only           */
      uint8_t media_changed;    /* Is media changed       */
      uint8_t media_rate;       /* Data rate of medium    */
+    bool media_inserted;      /* Is there a medium in the tray */
  } FDrive;
static void fd_init(FDrive *drv)
@@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t 
track, uint8_t sect,
          drv->head = head;
          if (drv->track != track) {
-            if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
+            if (drv->media_inserted) {
I suspect that with the removal of blk_is_inserted() in several places,
floppy passthrough (host_floppy block driver) is now even more broken
than before, potentially not noticing removal of a medium any more.

While checking this, I noticed that since commit 21fcf360,
bdrv_media_changed() is completely unused. Media change has therefore
probably been broken since at least May 2012.

Considering this, it might actually be reasonable enough to remove the
block driver. It's definitely better than having it there, but not
working any better than host_device.

Of course, alternatively you would also be welcome to fix the device
model and reintroduce bdrv_media_changed() and blk_is_inserted() calls
where necessary.


I thought about why I need this tray status at all. The result is: We need it because otherwise blockdev-close-tray doesn't do anything on floppy disk drives (you'd insert a medium with blockdev-insert-medium and the tray would immediately be reported as being closed, without any TRAY_MOVED event). So we do need it.

I tested floppy passthrough in a VM and it's horror, as can be expected. For instance, one cannot start qemu with floppy passthrough if the host drive is empty (which is actually helpful).

So what I'll be doing is set media_inserted to load && drv->blk in the media change CB and set it to true in the floppy drive initialization code if there is a BB and blk_is_inserted() is true. The tray will be considered closed if media_inserted && blk_is_inserted().

So why do I check blk_is_inserted() in the initialization code? If we just set media_inserted to true, that would be wrong. Imagine an empty drive (no BDS tree), media_inserted will be set to true, then you inserted a BDS tree (blockdev-insert-medium) and the tray will be considered closed without you having executed blockdev-close-tray (media_inserted is true, and now blk_is_inserted() is true as well).

So I have to check whether there's a BDS tree in the initialization code, if there isn't, media_inserted must be false. Why am I not using blk_bs() for that? I tried to explain for the USB patch: blk_bs() returns a BDS, and that's something for the block layer, nobody else should care about that.

How could blk_is_inserted() be wrong here, if blk_bs() is the thing that will work? Correct, if blk_bs() && !bdrv_is_inserted(blk_bs()). However, this will never be the case, because as said above, qemu actually cannot start with a host floppy drive without a medium, so !!blk_is_inserted(blk) == !!blk_bs(blk).

I see the question popping up "Why don't you just add a bool has_bs(BB) { return blk_bs(BB); } and then not add that test to blk_is_inserted()"? I've asked that myself. Answer: Again, anything outside of the block layer should not care about things like BDS trees. But moreover, bdrv_is_inserted() does not only check whether all the host devices represented by BDSs are inserted, but also whether BDS.drv != NULL, which until this series was the sign for an empty drive. Therefore, checking blk_is_inserted() is the logical conclusion of bdrv_is_inserted() (bdrv_is_inserted() tests whether BDS.drv != NULL, blk_is_inserted() tests whether BLK.bs != NULL).

But medium management is now done on the BB level, so a separate function for checking whether there's a BDS tree (because that should be equivalent to "whether there's a medium") seems to have its merits. However, I don't think so. blk_is_inserted() is exactly that function: It's true if there is a BDS tree and, if there is host passthrough, whether all the media are inserted, which is correct.

In theory, guest models should not have to distinguish whether a BB does not have a medium because there is no BDS tree or because we're using passthrough and that BDS does not have a medium. So why does floppy have to distinguish? Because it does not have a real tray, but that's the model we have to be working with. Inserting a medium into a passed-through host drive must result in the tray being considered closed immediately; inserting a medium into a guest device through blockdev-insert-medium must not.

So our bane is that we need tray status for floppy disks but they don't have a tray, so inserting a medium in a host drive does something different than doing the same in a purely virtual drive. Dropping host_floppy probably solves that problem, but I'm too cautious for that.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]