[Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up
@ 2012-12-17 14:05 Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
                   ` (10 more replies)
  0 siblings, 11 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

*** Important ***
This *breaks* all non-qdevified controllers, see PATCH 01/10.
Maintainers are cc'ed.

If you want still more time to qdevify your controller, please speak
up now, and tell us how much.

The rest of the series is obvious cleanups enabled by dropping the
special case of a non-qdevified controller.  The block configuration
stuff I'm working on also profits from it, and is real reason I'm
posting this.

Markus Armbruster (10):
  ide: Break all non-qdevified controllers
  ide: Move IDEDevice pointer from IDEBus to IDEState
  ide: Use IDEState member dev for "device connected" test
  ide: Don't block-align IDEState member smart_selftest_data
  ide: Drop redundant IDEState member bs
  ide: Drop redundant IDEState geometry members
  ide: Drop redundant IDEState member version
  ide: Drop redundant IDEState member drive_serial_str
  ide: Drop redundant IDEState member model
  ide: Drop redundant IDEState member wwn

 hw/ide/ahci.c       |  19 ++--
 hw/ide/atapi.c      |  39 ++++----
 hw/ide/core.c       | 278 +++++++++++++++++++++-------------------------------
 hw/ide/internal.h   |  15 +--
 hw/ide/macio.c      |  20 ++--
 hw/ide/microdrive.c |   5 +-
 hw/ide/piix.c       |   1 -
 hw/ide/qdev.c       |  50 +++++-----
 8 files changed, 189 insertions(+), 238 deletions(-)

-- 
1.7.11.7

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:09   ` Alexander Graf
  2012-12-18 12:12   ` Peter Maydell
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 02/10] ide: Move IDEDevice pointer from IDEBus to IDEState Markus Armbruster
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

They complicate IDE data structures and keep getting in the way.
Also, TRIM support (commit d353fb72) is broken for them, because
ide_identify() accesses IDEDevice member conf, but IDEDevice exists
only with qdevified controllers.

The non-qdevified controllers are still there, but attempting to
connect devices to them fails with "IDE controller not qdevified yet;
drive <name> ignored".

Affected machines:

* g3beige's first IDE channel (MacIO)
  -hda, -hdb are on first channel, and no longer work
  -hdc, -hdd are on second channel, and still work
* mac99's second and third IDE channel (MacIO)
  All four IDE drives no longer work
* spitz, borzoi, terrier and tosa (CF microdrive)
  -hda no longer works
  the other IDE drives have always been silently ignored
* r2d's onboard CF (MMIO)
  -hda no longer works
  the other IDE drives have always been silently ignored

The writing has been on the wall for a few years.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c | 43 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c4f93d0..2c0c978 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2057,53 +2057,28 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
     bus->dma = &ide_dma_nop;
 }
 
-/* TODO convert users to qdev and remove */
+/* TODO users are *broken*; convert them to qdev and remove this function */
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq)
 {
-    int i, trans;
+    Location loc;
+    int i;
     DriveInfo *dinfo;
-    uint32_t cyls, heads, secs;
 
+    loc_push_none(&loc);
     for(i = 0; i < 2; i++) {
         dinfo = i == 0 ? hd0 : hd1;
         ide_init1(bus, i);
         if (dinfo) {
-            cyls  = dinfo->cyls;
-            heads = dinfo->heads;
-            secs  = dinfo->secs;
-            trans = dinfo->trans;
-            if (!cyls && !heads && !secs) {
-                hd_geometry_guess(dinfo->bdrv, &cyls, &heads, &secs, &trans);
-            } else if (trans == BIOS_ATA_TRANSLATION_AUTO) {
-                trans = hd_bios_chs_auto_trans(cyls, heads, secs);
-            }
-            if (cyls < 1 || cyls > 65535) {
-                error_report("cyls must be between 1 and 65535");
-                exit(1);
-            }
-            if (heads < 1 || heads > 16) {
-                error_report("heads must be between 1 and 16");
-                exit(1);
-            }
-            if (secs < 1 || secs > 255) {
-                error_report("secs must be between 1 and 255");
-                exit(1);
-            }
-            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
-                               dinfo->media_cd ? IDE_CD : IDE_HD,
-                               NULL, dinfo->serial, NULL, 0,
-                               cyls, heads, secs, trans) < 0) {
-                error_report("Can't set up IDE drive %s", dinfo->id);
-                exit(1);
-            }
-            bdrv_attach_dev_nofail(dinfo->bdrv, &bus->ifs[i]);
-        } else {
-            ide_reset(&bus->ifs[i]);
+            qemu_opts_loc_restore(dinfo->opts);
+            error_report("IDE controller not qdevified yet; drive %s ignored",
+                         dinfo->id);
         }
+        ide_reset(&bus->ifs[i]);
     }
     bus->irq = irq;
     bus->dma = &ide_dma_nop;
+    loc_pop(&loc);
 }
 
 static const MemoryRegionPortio ide_portio_list[] = {
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
@ 2012-12-17 14:09   ` Alexander Graf
  2012-12-17 14:43     ` Markus Armbruster
  2012-12-18 12:12   ` Peter Maydell
  1 sibling, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-17 14:09 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-ppc, qemu-devel


On 17.12.2012, at 15:05, Markus Armbruster wrote:

> They complicate IDE data structures and keep getting in the way.
> Also, TRIM support (commit d353fb72) is broken for them, because
> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
> only with qdevified controllers.
> 
> The non-qdevified controllers are still there, but attempting to
> connect devices to them fails with "IDE controller not qdevified yet;
> drive <name> ignored".
> 
> Affected machines:
> 
> * g3beige's first IDE channel (MacIO)
>  -hda, -hdb are on first channel, and no longer work
>  -hdc, -hdd are on second channel, and still work
> * mac99's second and third IDE channel (MacIO)
>  All four IDE drives no longer work

Nack. This breaks the default targets of qemu-system-ppc and qemu-system-ppc64.


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:09   ` Alexander Graf
@ 2012-12-17 14:43     ` Markus Armbruster
  2012-12-17 14:55       ` Alexander Graf
  2012-12-17 21:50       ` Andreas Färber
  0 siblings, 2 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:43 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kwolf, qemu-ppc, qemu-devel

Alexander Graf <agraf@suse.de> writes:

> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>
>> They complicate IDE data structures and keep getting in the way.
>> Also, TRIM support (commit d353fb72) is broken for them, because
>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>> only with qdevified controllers.
>> 
>> The non-qdevified controllers are still there, but attempting to
>> connect devices to them fails with "IDE controller not qdevified yet;
>> drive <name> ignored".
>> 
>> Affected machines:
>> 
>> * g3beige's first IDE channel (MacIO)
>>  -hda, -hdb are on first channel, and no longer work
>>  -hdc, -hdd are on second channel, and still work
>> * mac99's second and third IDE channel (MacIO)
>>  All four IDE drives no longer work
>
> Nack. This breaks the default targets of qemu-system-ppc and qemu-system-ppc64.

Please tell us how much more time you want to qdevify IDE for these
targets.  Thanks!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:43     ` Markus Armbruster
@ 2012-12-17 14:55       ` Alexander Graf
  2012-12-17 15:15         ` Markus Armbruster
  2012-12-17 21:50       ` Andreas Färber
  1 sibling, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-17 14:55 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-ppc, qemu-devel


On 17.12.2012, at 15:43, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>> 
>>> They complicate IDE data structures and keep getting in the way.
>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>> only with qdevified controllers.
>>> 
>>> The non-qdevified controllers are still there, but attempting to
>>> connect devices to them fails with "IDE controller not qdevified yet;
>>> drive <name> ignored".
>>> 
>>> Affected machines:
>>> 
>>> * g3beige's first IDE channel (MacIO)
>>> -hda, -hdb are on first channel, and no longer work
>>> -hdc, -hdd are on second channel, and still work
>>> * mac99's second and third IDE channel (MacIO)
>>> All four IDE drives no longer work
>> 
>> Nack. This breaks the default targets of qemu-system-ppc and qemu-system-ppc64.
> 
> Please tell us how much more time you want to qdevify IDE for these
> targets.  Thanks!

I don't know. If it's dear to you, just convert it yourself. Apparently you're quite deep into the details here already, so it's a lot easier for you than for me anyways.


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:55       ` Alexander Graf
@ 2012-12-17 15:15         ` Markus Armbruster
  2012-12-17 15:18           ` Alexander Graf
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 15:15 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kwolf, qemu-ppc, qemu-devel

Alexander Graf <agraf@suse.de> writes:

> On 17.12.2012, at 15:43, Markus Armbruster wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>>> 
>>>> They complicate IDE data structures and keep getting in the way.
>>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>>> only with qdevified controllers.
>>>> 
>>>> The non-qdevified controllers are still there, but attempting to
>>>> connect devices to them fails with "IDE controller not qdevified yet;
>>>> drive <name> ignored".
>>>> 
>>>> Affected machines:
>>>> 
>>>> * g3beige's first IDE channel (MacIO)
>>>> -hda, -hdb are on first channel, and no longer work
>>>> -hdc, -hdd are on second channel, and still work
>>>> * mac99's second and third IDE channel (MacIO)
>>>> All four IDE drives no longer work
>>> 
>>> Nack. This breaks the default targets of qemu-system-ppc and
>>> qemu-system-ppc64.
>> 
>> Please tell us how much more time you want to qdevify IDE for these
>> targets.  Thanks!
>
> I don't know. If it's dear to you, just convert it
> yourself. Apparently you're quite deep into the details here already,
> so it's a lot easier for you than for me anyways.

These controllers aren't dear to me, they're in the way.  Have been for
years.  I doubt hacking them is easier for me than for you.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 15:15         ` Markus Armbruster
@ 2012-12-17 15:18           ` Alexander Graf
  2012-12-17 15:38             ` Markus Armbruster
  0 siblings, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-17 15:18 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-ppc, qemu-devel


On 17.12.2012, at 16:15, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 17.12.2012, at 15:43, Markus Armbruster wrote:
>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>>>> 
>>>>> They complicate IDE data structures and keep getting in the way.
>>>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>>>> only with qdevified controllers.
>>>>> 
>>>>> The non-qdevified controllers are still there, but attempting to
>>>>> connect devices to them fails with "IDE controller not qdevified yet;
>>>>> drive <name> ignored".
>>>>> 
>>>>> Affected machines:
>>>>> 
>>>>> * g3beige's first IDE channel (MacIO)
>>>>> -hda, -hdb are on first channel, and no longer work
>>>>> -hdc, -hdd are on second channel, and still work
>>>>> * mac99's second and third IDE channel (MacIO)
>>>>> All four IDE drives no longer work
>>>> 
>>>> Nack. This breaks the default targets of qemu-system-ppc and
>>>> qemu-system-ppc64.
>>> 
>>> Please tell us how much more time you want to qdevify IDE for these
>>> targets.  Thanks!
>> 
>> I don't know. If it's dear to you, just convert it
>> yourself. Apparently you're quite deep into the details here already,
>> so it's a lot easier for you than for me anyways.
> 
> These controllers aren't dear to me, they're in the way.  Have been for
> years.  I doubt hacking them is easier for me than for you.

Windows XP support has been in my way plenty times too. I still don't (heavily) suggest dropping it.


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 15:18           ` Alexander Graf
@ 2012-12-17 15:38             ` Markus Armbruster
  0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 15:38 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kwolf, qemu-ppc, qemu-devel

Alexander Graf <agraf@suse.de> writes:

> On 17.12.2012, at 16:15, Markus Armbruster wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 17.12.2012, at 15:43, Markus Armbruster wrote:
>>> 
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>>>>> 
>>>>>> They complicate IDE data structures and keep getting in the way.
>>>>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>>>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>>>>> only with qdevified controllers.
>>>>>> 
>>>>>> The non-qdevified controllers are still there, but attempting to
>>>>>> connect devices to them fails with "IDE controller not qdevified yet;
>>>>>> drive <name> ignored".
>>>>>> 
>>>>>> Affected machines:
>>>>>> 
>>>>>> * g3beige's first IDE channel (MacIO)
>>>>>> -hda, -hdb are on first channel, and no longer work
>>>>>> -hdc, -hdd are on second channel, and still work
>>>>>> * mac99's second and third IDE channel (MacIO)
>>>>>> All four IDE drives no longer work
>>>>> 
>>>>> Nack. This breaks the default targets of qemu-system-ppc and
>>>>> qemu-system-ppc64.
>>>> 
>>>> Please tell us how much more time you want to qdevify IDE for these
>>>> targets.  Thanks!
>>> 
>>> I don't know. If it's dear to you, just convert it
>>> yourself. Apparently you're quite deep into the details here already,
>>> so it's a lot easier for you than for me anyways.
>> 
>> These controllers aren't dear to me, they're in the way.  Have been for
>> years.  I doubt hacking them is easier for me than for you.
>
> Windows XP support has been in my way plenty times too. I still don't
> (heavily) suggest dropping it.

I'm not suggesting to drop these targets.  I'm suggesting they get the
maintainance they need to keep up with evolving infrastructure.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:43     ` Markus Armbruster
  2012-12-17 14:55       ` Alexander Graf
@ 2012-12-17 21:50       ` Andreas Färber
  2012-12-18 12:55         ` Markus Armbruster
  1 sibling, 1 reply; 24+ messages in thread
From: Andreas Färber @ 2012-12-17 21:50 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: kwolf, Peter Maydell, Alexander Graf, qemu-devel, qemu-ppc,
	Aurelien Jarno

Am 17.12.2012 15:43, schrieb Markus Armbruster:
> Alexander Graf <agraf@suse.de> writes:
> 
>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>>
>>> They complicate IDE data structures and keep getting in the way.
>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>> only with qdevified controllers.
>>>
>>> The non-qdevified controllers are still there, but attempting to
>>> connect devices to them fails with "IDE controller not qdevified yet;
>>> drive <name> ignored".
>>>
>>> Affected machines:
>>>
>>> * g3beige's first IDE channel (MacIO)
>>>  -hda, -hdb are on first channel, and no longer work
>>>  -hdc, -hdd are on second channel, and still work
>>> * mac99's second and third IDE channel (MacIO)
>>>  All four IDE drives no longer work
>>
>> Nack. This breaks the default targets of qemu-system-ppc and qemu-system-ppc64.
> 
> Please tell us how much more time you want to qdevify IDE for these
> targets.  Thanks!

I believe I have a branch with macio QOM'ifications somewhere that I
could revive. Note that I know little about IDE or block layer and
mainly care about consistent infrastructure there; I vaguely remember
something about the mac's IDE channels being mixed together from two
devices unlike real hardware, guess I would be unable to fix that.

As for your question, 2013 and a gentle reminder to all involved would
be nice. :) In particular we have the Soft Freeze coming up shortly
after the holidays, so is this needed for 1.4 Soft Freeze or can it be
deferred to 1.5 or done during the 1.4 Soft Freeze?

If Aurélien (CC'ed) doesn't manage, I can look at r2d as well.
CC'ing Peter and Andrzej for the arm devices.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 21:50       ` Andreas Färber
@ 2012-12-18 12:55         ` Markus Armbruster
  0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-18 12:55 UTC (permalink / raw)
  To: Andreas Färber
  Cc: kwolf, Peter Maydell, Alexander Graf, qemu-devel, qemu-ppc,
	Aurelien Jarno

Andreas Färber <afaerber@suse.de> writes:

> Am 17.12.2012 15:43, schrieb Markus Armbruster:
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 17.12.2012, at 15:05, Markus Armbruster wrote:
>>>
>>>> They complicate IDE data structures and keep getting in the way.
>>>> Also, TRIM support (commit d353fb72) is broken for them, because
>>>> ide_identify() accesses IDEDevice member conf, but IDEDevice exists
>>>> only with qdevified controllers.
>>>>
>>>> The non-qdevified controllers are still there, but attempting to
>>>> connect devices to them fails with "IDE controller not qdevified yet;
>>>> drive <name> ignored".
>>>>
>>>> Affected machines:
>>>>
>>>> * g3beige's first IDE channel (MacIO)
>>>>  -hda, -hdb are on first channel, and no longer work
>>>>  -hdc, -hdd are on second channel, and still work
>>>> * mac99's second and third IDE channel (MacIO)
>>>>  All four IDE drives no longer work
>>>
>>> Nack. This breaks the default targets of qemu-system-ppc and
>>> qemu-system-ppc64.
>> 
>> Please tell us how much more time you want to qdevify IDE for these
>> targets.  Thanks!
>
> I believe I have a branch with macio QOM'ifications somewhere that I
> could revive.

I'd appreciate that.

>               Note that I know little about IDE or block layer and
> mainly care about consistent infrastructure there; I vaguely remember
> something about the mac's IDE channels being mixed together from two
> devices unlike real hardware, guess I would be unable to fix that.

Yes, g3beige's first IDE channel is MacIO (not qdevified), second is
CMD646 (qdevified), and mac99's first IDE channel is unimplemented,
second and third are MacIO (not qdevified).  Inhowfar that matches real
hardware I don't know.

> As for your question, 2013 and a gentle reminder to all involved would
> be nice. :)

In my experience with IDE qdevification, gentle reminders do not work.
But I'd be delighted to be proven wrong.

>             In particular we have the Soft Freeze coming up shortly
> after the holidays, so is this needed for 1.4 Soft Freeze or can it be
> deferred to 1.5 or done during the 1.4 Soft Freeze?
>
> If Aurélien (CC'ed) doesn't manage, I can look at r2d as well.
> CC'ing Peter and Andrzej for the arm devices.

In time for 1.4 would be good, because it's in the way of the block
backend configuration work I'd like to get into 1.4.  If I can pull it
off in time.  Having to hack around IDE silliness that cannot be cleaned
up while we still have non-qdevified controllers isn't helping :)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
  2012-12-17 14:09   ` Alexander Graf
@ 2012-12-18 12:12   ` Peter Maydell
  2012-12-18 12:44     ` Markus Armbruster
  1 sibling, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2012-12-18 12:12 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-ppc, qemu-devel, agraf

On 17 December 2012 14:05, Markus Armbruster <armbru@redhat.com> wrote:
> The writing has been on the wall for a few years.

...behind a filing cabinet in a disused lavatory with a sign on the door
saying "beware of the leopard"?

We really need a better way to mark devices as "obsolete, will be
removed/broken/etc in a future version"...

-- PMM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-18 12:12   ` Peter Maydell
@ 2012-12-18 12:44     ` Markus Armbruster
  2012-12-18 13:56       ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2012-12-18 12:44 UTC (permalink / raw)
  To: Peter Maydell; +Cc: kwolf, qemu-ppc, qemu-devel, agraf

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

> On 17 December 2012 14:05, Markus Armbruster <armbru@redhat.com> wrote:
>> The writing has been on the wall for a few years.
>
> ...behind a filing cabinet in a disused lavatory with a sign on the door
> saying "beware of the leopard"?
>
> We really need a better way to mark devices as "obsolete, will be
> removed/broken/etc in a future version"...

Yes, we do.

These devices, however, are a slightly different case: "need
maintenance, will be removed unless they get it".  I've asked for them
to be updated on several occasions, on the list[*], and in person at the
last three KVM Forums.

Having your board in the tree is a privilege, not a right.  You earn the
privilege by doing your share of the work.  In my opinion, that includes
moving them off obsolete infrastructure in a timely manner, at least
when keeping the obsolete infrastructure around just for you becomes a
drag.  Which it definitely has been in case of IDE.

[*] http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00621.html

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers
  2012-12-18 12:44     ` Markus Armbruster
@ 2012-12-18 13:56       ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2012-12-18 13:56 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: kwolf, qemu-ppc, qemu-devel, agraf

On 18 December 2012 12:44, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> We really need a better way to mark devices as "obsolete, will be
>> removed/broken/etc in a future version"...
>
> Yes, we do.
>
> These devices, however, are a slightly different case: "need
> maintenance, will be removed unless they get it".  I've asked for them
> to be updated on several occasions, on the list[*], and in person at the
> last three KVM Forums.

My concern is basically that I suspect there's a user community
out there that doesn't necessarily read the list or go to KVM Forum.
I actually have no idea which of the various elderly ARM boards are
really used and which we could just delete. If back in July we'd
marked the relevant boards as "this may disappear in the next version"
we might have got some feedback about if anybody was actually using
tosa, spitz, etc.

> Having your board in the tree is a privilege, not a right.  You earn the
> privilege by doing your share of the work.

I agree completely with this.

-- PMM

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 02/10] ide: Move IDEDevice pointer from IDEBus to IDEState
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 03/10] ide: Use IDEState member dev for "device connected" test Markus Armbruster
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf


Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |  2 +-
 hw/ide/internal.h |  3 +--
 hw/ide/qdev.c     | 12 +++---------
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 2c0c978..1e94dc4 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -79,7 +79,7 @@ static void ide_identify(IDEState *s)
 {
     uint16_t *p;
     unsigned int oldsize;
-    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
+    IDEDevice *dev = s->dev;
 
     if (s->identify_set) {
 	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index bf7d313..52d1642 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -342,6 +342,7 @@ enum ide_dma_cmd {
 /* NOTE: IDEState represents in fact one drive */
 struct IDEState {
     IDEBus *bus;
+    IDEDevice *dev;
     uint8_t unit;
     /* ide config */
     IDEDriveKind drive_kind;
@@ -447,8 +448,6 @@ struct IDEDMA {
 
 struct IDEBus {
     BusState qbus;
-    IDEDevice *master;
-    IDEDevice *slave;
     IDEState ifs[2];
     int bus_id;
     IDEDMA *dma;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f2e4ea4..e322aea 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -74,22 +74,16 @@ static int ide_qdev_init(DeviceState *qdev)
         goto err;
     }
     if (dev->unit == -1) {
-        dev->unit = bus->master ? 1 : 0;
+        dev->unit = bus->ifs[0].dev ? 1 : 0;
     }
     switch (dev->unit) {
     case 0:
-        if (bus->master) {
-            error_report("IDE unit %d is in use", dev->unit);
-            goto err;
-        }
-        bus->master = dev;
-        break;
     case 1:
-        if (bus->slave) {
+        if (bus->ifs[dev->unit].dev) {
             error_report("IDE unit %d is in use", dev->unit);
             goto err;
         }
-        bus->slave = dev;
+        bus->ifs[dev->unit].dev = dev;
         break;
     default:
         error_report("Invalid IDE unit %d", dev->unit);
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 03/10] ide: Use IDEState member dev for "device connected" test
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 02/10] ide: Move IDEDevice pointer from IDEBus to IDEState Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 04/10] ide: Don't block-align IDEState member smart_selftest_data Markus Armbruster
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

Testing dev is more obvious than testing bs, in my opinion.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/ahci.c       |  8 ++++----
 hw/ide/core.c       | 56 ++++++++++++++++++++++++++++-------------------------
 hw/ide/microdrive.c |  5 +++--
 hw/ide/qdev.c       |  2 +-
 4 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 67562db..4249489 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -86,7 +86,7 @@ static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
         val = pr->sig;
         break;
     case PORT_SCR_STAT:
-        if (s->dev[port].port.ifs[0].bs) {
+        if (s->dev[port].port.ifs[0].dev) {
             val = SATA_SCR_SSTATUS_DET_DEV_PRESENT_PHY_UP |
                   SATA_SCR_SSTATUS_SPD_GEN1 | SATA_SCR_SSTATUS_IPM_ACTIVE;
         } else {
@@ -497,7 +497,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     d->init_d2h_sent = 0;
 
     ide_state = &s->dev[port].port.ifs[0];
-    if (!ide_state->bs) {
+    if (!ide_state->dev) {
         return;
     }
 
@@ -523,7 +523,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     }
 
     s->dev[port].port_state = STATE_RUN;
-    if (!ide_state->bs) {
+    if (!ide_state->dev) {
         s->dev[port].port_regs.sig = 0;
         ide_state->status = SEEK_STAT | WRERR_STAT;
     } else if (ide_state->drive_kind == IDE_CD) {
@@ -848,7 +848,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
     /* The device we are working for */
     ide_state = &s->dev[port].port.ifs[0];
 
-    if (!ide_state->bs) {
+    if (!ide_state->dev) {
         DPRINTF(port, "error: guest accessed unused port");
         goto out;
     }
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1e94dc4..5876862 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -312,7 +312,7 @@ static void ide_set_signature(IDEState *s)
     if (s->drive_kind == IDE_CD) {
         s->lcyl = 0x14;
         s->hcyl = 0xeb;
-    } else if (s->bs) {
+    } else if (s->dev) {
         s->lcyl = 0;
         s->hcyl = 0;
     } else {
@@ -780,7 +780,7 @@ static void ide_flush_cb(void *opaque, int ret)
 
 void ide_flush_cache(IDEState *s)
 {
-    if (s->bs == NULL) {
+    if (!s->dev) {
         ide_flush_cb(s, 0);
         return;
     }
@@ -1059,8 +1059,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 #endif
     s = idebus_active_if(bus);
     /* ignore commands to non existent slave */
-    if (s != bus->ifs && !s->bs)
+    if (s != bus->ifs && !s->dev) {
         return;
+    }
 
     /* Only DEVICE RESET is allowed while BSY or/and DRQ are set */
     if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET)
@@ -1074,7 +1075,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
     case WIN_DSM:
         switch (s->feature) {
         case DSM_TRIM:
-            if (!s->bs) {
+            if (!s->dev) {
                 goto abort_cmd;
             }
             ide_sector_start_dma(s, IDE_DMA_TRIM);
@@ -1084,7 +1085,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         }
         break;
     case WIN_IDENTIFY:
-        if (s->bs && s->drive_kind != IDE_CD) {
+        if (s->dev && s->drive_kind != IDE_CD) {
             if (s->drive_kind != IDE_CFATA)
                 ide_identify(s);
             else
@@ -1137,7 +1138,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
             ide_set_signature(s); /* odd, but ATA4 8.27.5.2 requires it */
             goto abort_cmd;
         }
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
 	ide_cmd_lba48_transform(s, lba48);
@@ -1150,7 +1151,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
     case WIN_WRITE_ONCE:
     case CFA_WRITE_SECT_WO_ERASE:
     case WIN_WRITE_VERIFY:
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
 	ide_cmd_lba48_transform(s, lba48);
@@ -1163,7 +1164,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
     case WIN_MULTREAD_EXT:
 	lba48 = 1;
     case WIN_MULTREAD:
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
         if (!s->mult_sectors) {
@@ -1177,7 +1178,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 	lba48 = 1;
     case WIN_MULTWRITE:
     case CFA_WRITE_MULTI_WO_ERASE:
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
         if (!s->mult_sectors) {
@@ -1197,7 +1198,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 	lba48 = 1;
     case WIN_READDMA:
     case WIN_READDMA_ONCE:
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
 	ide_cmd_lba48_transform(s, lba48);
@@ -1207,7 +1208,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 	lba48 = 1;
     case WIN_WRITEDMA:
     case WIN_WRITEDMA_ONCE:
-        if (!s->bs) {
+        if (!s->dev) {
             goto abort_cmd;
         }
 	ide_cmd_lba48_transform(s, lba48);
@@ -1230,7 +1231,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         ide_set_irq(s->bus);
         break;
     case WIN_SETFEATURES:
-        if (!s->bs)
+        if (!s->dev)
             goto abort_cmd;
         /* XXX: valid for CDROM ? */
         switch(s->feature) {
@@ -1599,8 +1600,8 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
         ret = 0xff;
         break;
     case 1:
-        if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
-            (s != bus->ifs && !s->bs))
+        if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+            (s != bus->ifs && !s->dev))
             ret = 0;
         else if (!hob)
             ret = s->error;
@@ -1608,7 +1609,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
 	    ret = s->hob_feature;
         break;
     case 2:
-        if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+        if (!bus->ifs[0].dev && !bus->ifs[1].dev)
             ret = 0;
         else if (!hob)
             ret = s->nsector & 0xff;
@@ -1616,7 +1617,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
 	    ret = s->hob_nsector;
         break;
     case 3:
-        if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+        if (!bus->ifs[0].dev && !bus->ifs[1].dev)
             ret = 0;
         else if (!hob)
             ret = s->sector;
@@ -1624,7 +1625,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
 	    ret = s->hob_sector;
         break;
     case 4:
-        if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+        if (!bus->ifs[0].dev && !bus->ifs[1].dev)
             ret = 0;
         else if (!hob)
             ret = s->lcyl;
@@ -1632,7 +1633,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
 	    ret = s->hob_lcyl;
         break;
     case 5:
-        if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+        if (!bus->ifs[0].dev && !bus->ifs[1].dev)
             ret = 0;
         else if (!hob)
             ret = s->hcyl;
@@ -1640,18 +1641,20 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
 	    ret = s->hob_hcyl;
         break;
     case 6:
-        if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+        if (!bus->ifs[0].dev && !bus->ifs[1].dev) {
             ret = 0;
-        else
+        } else {
             ret = s->select;
+        }
         break;
     default:
     case 7:
-        if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
-            (s != bus->ifs && !s->bs))
+        if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+            (s != bus->ifs && !s->dev)) {
             ret = 0;
-        else
+        } else {
             ret = s->status;
+        }
         qemu_irq_lower(bus->irq);
         break;
     }
@@ -1667,11 +1670,12 @@ uint32_t ide_status_read(void *opaque, uint32_t addr)
     IDEState *s = idebus_active_if(bus);
     int ret;
 
-    if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
-        (s != bus->ifs && !s->bs))
+    if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+        (s != bus->ifs && !s->dev)) {
         ret = 0;
-    else
+    } else {
         ret = s->status;
+    }
 #ifdef DEBUG_IDE
     printf("ide: read status addr=0x%x val=%02x\n", addr, ret);
 #endif
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index 9eee5b5..05c5ab1 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -223,10 +223,11 @@ static uint16_t md_common_read(void *opaque, uint32_t at)
         return ide_ioport_read(&s->bus, 0x1);
     case 0xe:	/* Alternate Status */
         ifs = idebus_active_if(&s->bus);
-        if (ifs->bs)
+        if (ifs->dev) {
             return ifs->status;
-        else
+        } else {
             return 0;
+        }
     case 0xf:	/* Device Address */
         ifs = idebus_active_if(&s->bus);
         return 0xc2 | ((~ifs->select << 2) & 0x3c);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index e322aea..01bbc74 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -111,7 +111,7 @@ int ide_get_geometry(BusState *bus, int unit,
 {
     IDEState *s = &DO_UPCAST(IDEBus, qbus, bus)->ifs[unit];
 
-    if (s->drive_kind != IDE_HD || !s->bs) {
+    if (s->drive_kind != IDE_HD || !s->dev) {
         return -1;
     }
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 04/10] ide: Don't block-align IDEState member smart_selftest_data
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (2 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 03/10] ide: Use IDEState member dev for "device connected" test Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 05/10] ide: Drop redundant IDEState member bs Markus Armbruster
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

All uses are simple array subscripts.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5876862..66e56c2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2006,8 +2006,7 @@ static void ide_init1(IDEBus *bus, int unit)
     s->io_buffer = qemu_memalign(2048, s->io_buffer_total_len);
     memset(s->io_buffer, 0, s->io_buffer_total_len);
 
-    s->smart_selftest_data = qemu_blockalign(s->bs, 512);
-    memset(s->smart_selftest_data, 0, 512);
+    s->smart_selftest_data = g_malloc0(512);
 
     s->sector_write_timer = qemu_new_timer_ns(vm_clock,
                                            ide_sector_write_timer_cb, s);
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 05/10] ide: Drop redundant IDEState member bs
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (3 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 04/10] ide: Don't block-align IDEState member smart_selftest_data Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 06/10] ide: Drop redundant IDEState geometry members Markus Armbruster
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

It's a copy of dev->conf.bs.  The copy was needed for non-qdevified
controllers, which lacked dev.

Note how pci_piix3_xen_ide_unplug() cleared the copy.  We'll get back
to that in the next few commits.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/ahci.c     | 11 +++++-----
 hw/ide/atapi.c    | 37 ++++++++++++++++++---------------
 hw/ide/core.c     | 62 ++++++++++++++++++++++++++++++-------------------------
 hw/ide/internal.h |  3 +--
 hw/ide/macio.c    | 20 ++++++++++--------
 hw/ide/piix.c     |  1 -
 hw/ide/qdev.c     |  2 +-
 7 files changed, 73 insertions(+), 63 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4249489..5fce616 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -732,7 +732,7 @@ static void ncq_cb(void *opaque, int ret)
     DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
             ncq_tfs->tag);
 
-    bdrv_acct_done(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct);
+    bdrv_acct_done(ncq_tfs->drive->port.ifs[0].dev->conf.bs, &ncq_tfs->acct);
     qemu_sglist_destroy(&ncq_tfs->sglist);
     ncq_tfs->used = 0;
 }
@@ -743,6 +743,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
     uint8_t tag = ncq_fis->tag >> 3;
     NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[tag];
+    BlockDriverState *bs = ncq_tfs->drive->port.ifs[0].dev->conf.bs;
 
     if (ncq_tfs->used) {
         /* error - already in use */
@@ -782,9 +783,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             DPRINTF(port, "tag %d aio read %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
 
-            dma_acct_start(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct,
+            dma_acct_start(bs, &ncq_tfs->acct,
                            &ncq_tfs->sglist, BDRV_ACCT_READ);
-            ncq_tfs->aiocb = dma_bdrv_read(ncq_tfs->drive->port.ifs[0].bs,
+            ncq_tfs->aiocb = dma_bdrv_read(bs,
                                            &ncq_tfs->sglist, ncq_tfs->lba,
                                            ncq_cb, ncq_tfs);
             break;
@@ -795,9 +796,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             DPRINTF(port, "tag %d aio write %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
 
-            dma_acct_start(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct,
+            dma_acct_start(bs, &ncq_tfs->acct,
                            &ncq_tfs->sglist, BDRV_ACCT_WRITE);
-            ncq_tfs->aiocb = dma_bdrv_write(ncq_tfs->drive->port.ifs[0].bs,
+            ncq_tfs->aiocb = dma_bdrv_write(bs,
                                             &ncq_tfs->sglist, ncq_tfs->lba,
                                             ncq_cb, ncq_tfs);
             break;
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 861fd2b..d077d18 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -106,18 +106,19 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
 
 static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)
 {
+    BlockDriverState *bs = s->dev->conf.bs;
     int ret;
 
     switch(sector_size) {
     case 2048:
-        bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
-        ret = bdrv_read(s->bs, (int64_t)lba << 2, buf, 4);
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_start(bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+        ret = bdrv_read(bs, (int64_t)lba << 2, buf, 4);
+        bdrv_acct_done(bs, &s->acct);
         break;
     case 2352:
-        bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
-        ret = bdrv_read(s->bs, (int64_t)lba << 2, buf + 16, 4);
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_start(bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+        ret = bdrv_read(bs, (int64_t)lba << 2, buf + 16, 4);
+        bdrv_acct_done(bs, &s->acct);
         if (ret < 0)
             return ret;
         cd_data_to_raw(buf, lba);
@@ -253,7 +254,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
     s->io_buffer_index = 0;
 
     if (s->atapi_dma) {
-        bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
+        bdrv_acct_start(s->dev->conf.bs, &s->acct, size, BDRV_ACCT_READ);
         s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
         s->bus->dma->ops->start_dma(s->bus->dma, s,
                                    ide_atapi_cmd_read_dma_cb);
@@ -349,13 +350,13 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
     s->bus->dma->iov.iov_len = n * 4 * 512;
     qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
 
-    s->bus->dma->aiocb = bdrv_aio_readv(s->bs, (int64_t)s->lba << 2,
+    s->bus->dma->aiocb = bdrv_aio_readv(s->dev->conf.bs, (int64_t)s->lba << 2,
                                        &s->bus->dma->qiov, n * 4,
                                        ide_atapi_cmd_read_dma_cb, s);
     return;
 
 eot:
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->dev->conf.bs, &s->acct);
     s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
     ide_set_inactive(s);
 }
@@ -371,7 +372,8 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
     s->io_buffer_size = 0;
     s->cd_sector_size = sector_size;
 
-    bdrv_acct_start(s->bs, &s->acct, s->packet_transfer_size, BDRV_ACCT_READ);
+    bdrv_acct_start(s->dev->conf.bs, &s->acct, s->packet_transfer_size,
+                    BDRV_ACCT_READ);
 
     /* XXX: check if BUSY_STAT should be set */
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
@@ -504,7 +506,7 @@ static unsigned int event_status_media(IDEState *s,
     media_status = 0;
     if (s->tray_open) {
         media_status = MS_TRAY_OPEN;
-    } else if (bdrv_is_inserted(s->bs)) {
+    } else if (bdrv_is_inserted(s->dev->conf.bs)) {
         media_status = MS_MEDIA_PRESENT;
     }
 
@@ -800,7 +802,7 @@ static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
 static void cmd_prevent_allow_medium_removal(IDEState *s, uint8_t* buf)
 {
     s->tray_locked = buf[4] & 1;
-    bdrv_lock_medium(s->bs, buf[4] & 1);
+    bdrv_lock_medium(s->dev->conf.bs, buf[4] & 1);
     ide_atapi_cmd_ok(s);
 }
 
@@ -884,14 +886,14 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
 
     if (loej) {
         if (!start && !s->tray_open && s->tray_locked) {
-            sense = bdrv_is_inserted(s->bs)
+            sense = bdrv_is_inserted(s->dev->conf.bs)
                 ? NOT_READY : ILLEGAL_REQUEST;
             ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
             return;
         }
 
         if (s->tray_open != !start) {
-            bdrv_eject(s->bs, !start);
+            bdrv_eject(s->dev->conf.bs, !start);
             s->tray_open = !start;
         }
     }
@@ -1094,6 +1096,7 @@ static const struct {
 void ide_atapi_cmd(IDEState *s)
 {
     uint8_t *buf;
+    bool is_inserted;
 
     buf = s->io_buffer;
 #ifdef DEBUG_IDE_ATAPI
@@ -1124,8 +1127,9 @@ void ide_atapi_cmd(IDEState *s)
      * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
      * states rely on this behavior.
      */
+    is_inserted = bdrv_is_inserted(s->dev->conf.bs);
     if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
-        !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
+        !s->tray_open && is_inserted && s->cdrom_changed) {
 
         if (s->cdrom_changed == 1) {
             ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
@@ -1134,13 +1138,12 @@ void ide_atapi_cmd(IDEState *s)
             ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
             s->cdrom_changed = 0;
         }
-
         return;
     }
 
     /* Report a Not Ready condition if appropriate for the command */
     if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
-        (!media_present(s) || !bdrv_is_inserted(s->bs)))
+        (!media_present(s) || !is_inserted))
     {
         ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
         return;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 66e56c2..9f306c6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -148,10 +148,11 @@ static void ide_identify(IDEState *s)
         put_le16(p + 84, (1 << 14) | 0);
     }
     /* 14 = NOP supported, 5=WCACHE enabled, 0=SMART feature set enabled */
-    if (bdrv_enable_write_cache(s->bs))
+    if (bdrv_enable_write_cache(s->dev->conf.bs)) {
          put_le16(p + 85, (1 << 14) | (1 << 5) | 1);
-    else
+    } else {
          put_le16(p + 85, (1 << 14) | 1);
+    }
     /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
     put_le16(p + 86, (1 << 13) | (1 <<12) | (1 << 10));
     /* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
@@ -478,7 +479,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
     s->pio_aiocb = NULL;
     s->status &= ~BUSY_STAT;
 
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->dev->conf.bs, &s->acct);
     if (ret != 0) {
         if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY |
                                 BM_STATUS_RETRY_READ)) {
@@ -502,6 +503,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
 
 void ide_sector_read(IDEState *s)
 {
+    BlockDriverState *bs = s->dev->conf.bs;
     int64_t sector_num;
     int n;
 
@@ -529,8 +531,8 @@ void ide_sector_read(IDEState *s)
     s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&s->qiov, &s->iov, 1);
 
-    bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
-    s->pio_aiocb = bdrv_aio_readv(s->bs, sector_num, &s->qiov, n,
+    bdrv_acct_start(bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+    s->pio_aiocb = bdrv_aio_readv(bs, sector_num, &s->qiov, n,
                                   ide_sector_read_cb, s);
 }
 
@@ -557,7 +559,8 @@ void ide_dma_error(IDEState *s)
 static int ide_handle_rw_error(IDEState *s, int error, int op)
 {
     bool is_read = (op & BM_STATUS_RETRY_READ) != 0;
-    BlockErrorAction action = bdrv_get_error_action(s->bs, is_read, error);
+    BlockErrorAction action = bdrv_get_error_action(s->dev->conf.bs,
+                                                    is_read, error);
 
     if (action == BDRV_ACTION_STOP) {
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
@@ -570,13 +573,14 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
             ide_rw_error(s);
         }
     }
-    bdrv_error_action(s->bs, action, is_read, error);
+    bdrv_error_action(s->dev->conf.bs, action, is_read, error);
     return action != BDRV_ACTION_IGNORE;
 }
 
 void ide_dma_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
+    BlockDriverState *bs = s->dev->conf.bs;
     int n;
     int64_t sector_num;
     bool stay_active = false;
@@ -636,15 +640,15 @@ void ide_dma_cb(void *opaque, int ret)
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        s->bus->dma->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
+        s->bus->dma->aiocb = dma_bdrv_read(bs, &s->sg, sector_num,
                                            ide_dma_cb, s);
         break;
     case IDE_DMA_WRITE:
-        s->bus->dma->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
+        s->bus->dma->aiocb = dma_bdrv_write(bs, &s->sg, sector_num,
                                             ide_dma_cb, s);
         break;
     case IDE_DMA_TRIM:
-        s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+        s->bus->dma->aiocb = dma_bdrv_io(bs, &s->sg, sector_num,
                                          ide_issue_trim, ide_dma_cb, s,
                                          DMA_DIRECTION_TO_DEVICE);
         break;
@@ -653,7 +657,7 @@ void ide_dma_cb(void *opaque, int ret)
 
 eot:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(bs, &s->acct);
     }
     ide_set_inactive(s);
     if (stay_active) {
@@ -670,12 +674,12 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 
     switch (dma_cmd) {
     case IDE_DMA_READ:
-        bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
-                        BDRV_ACCT_READ);
+        bdrv_acct_start(s->dev->conf.bs, &s->acct,
+                        s->nsector * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
         break;
     case IDE_DMA_WRITE:
-        bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
-                        BDRV_ACCT_WRITE);
+        bdrv_acct_start(s->dev->conf.bs, &s->acct,
+                        s->nsector * BDRV_SECTOR_SIZE, BDRV_ACCT_WRITE);
         break;
     default:
         break;
@@ -695,7 +699,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
     IDEState *s = opaque;
     int n;
 
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->dev->conf.bs, &s->acct);
 
     s->pio_aiocb = NULL;
     s->status &= ~BUSY_STAT;
@@ -740,6 +744,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
 
 void ide_sector_write(IDEState *s)
 {
+    BlockDriverState *bs = s->dev->conf.bs;
     int64_t sector_num;
     int n;
 
@@ -757,8 +762,8 @@ void ide_sector_write(IDEState *s)
     s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
     qemu_iovec_init_external(&s->qiov, &s->iov, 1);
 
-    bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
-    s->pio_aiocb = bdrv_aio_writev(s->bs, sector_num, &s->qiov, n,
+    bdrv_acct_start(bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+    s->pio_aiocb = bdrv_aio_writev(bs, sector_num, &s->qiov, n,
                                    ide_sector_write_cb, s);
 }
 
@@ -773,7 +778,7 @@ static void ide_flush_cb(void *opaque, int ret)
         }
     }
 
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->dev->conf.bs, &s->acct);
     s->status = READY_STAT | SEEK_STAT;
     ide_set_irq(s->bus);
 }
@@ -785,8 +790,8 @@ void ide_flush_cache(IDEState *s)
         return;
     }
 
-    bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
-    bdrv_aio_flush(s->bs, ide_flush_cb, s);
+    bdrv_acct_start(s->dev->conf.bs, &s->acct, 0, BDRV_ACCT_FLUSH);
+    bdrv_aio_flush(s->dev->conf.bs, ide_flush_cb, s);
 }
 
 static void ide_cfata_metadata_inquiry(IDEState *s)
@@ -849,7 +854,7 @@ static void ide_cd_change_cb(void *opaque, bool load)
     uint64_t nb_sectors;
 
     s->tray_open = !load;
-    bdrv_get_geometry(s->bs, &nb_sectors);
+    bdrv_get_geometry(s->dev->conf.bs, &nb_sectors);
     s->nb_sectors = nb_sectors;
 
     /*
@@ -1236,14 +1241,14 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         /* XXX: valid for CDROM ? */
         switch(s->feature) {
         case 0x02: /* write cache enable */
-            bdrv_set_enable_write_cache(s->bs, true);
+            bdrv_set_enable_write_cache(s->dev->conf.bs, true);
             identify_data = (uint16_t *)s->identify_data;
             put_le16(identify_data + 85, (1 << 14) | (1 << 5) | 1);
             s->status = READY_STAT | SEEK_STAT;
             ide_set_irq(s->bus);
             break;
         case 0x82: /* write cache disable */
-            bdrv_set_enable_write_cache(s->bs, false);
+            bdrv_set_enable_write_cache(s->dev->conf.bs, false);
             identify_data = (uint16_t *)s->identify_data;
             put_le16(identify_data + 85, (1 << 14) | 1);
             ide_flush_cache(s);
@@ -1923,15 +1928,15 @@ static const BlockDevOps ide_cd_block_ops = {
     .is_medium_locked = ide_cd_is_medium_locked,
 };
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
                    uint64_t wwn,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
                    int chs_trans)
 {
+    BlockDriverState *bs = s->dev->conf.bs;
     uint64_t nb_sectors;
 
-    s->bs = bs;
     s->drive_kind = kind;
 
     bdrv_get_geometry(bs, &nb_sectors);
@@ -1951,7 +1956,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
         bdrv_set_dev_ops(bs, &ide_cd_block_ops, s);
         bdrv_set_buffer_alignment(bs, 2048);
     } else {
-        if (!bdrv_is_inserted(s->bs)) {
+        if (!bdrv_is_inserted(bs)) {
             error_report("Device needs media, but drive is empty");
             return -1;
         }
@@ -2139,7 +2144,8 @@ static int ide_drive_post_load(void *opaque, int version_id)
     IDEState *s = opaque;
 
     if (s->identify_set) {
-        bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
+        bdrv_set_enable_write_cache(s->dev->conf.bs,
+                                    !!(s->identify_data[85] & (1 << 5)));
     }
     return 0;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 52d1642..6058db4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -374,7 +374,6 @@ struct IDEState {
 
     /* set for lba48 access */
     uint8_t lba48;
-    BlockDriverState *bs;
     char version[9];
     /* ATAPI specific */
     struct unreported_events events;
@@ -545,7 +544,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
                    uint64_t wwn,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index d2edcc0..44d31a1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -82,13 +82,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     io->addr += io->len;
     io->len = 0;
 
-    m->aiocb = dma_bdrv_read(s->bs, &s->sg,
+    m->aiocb = dma_bdrv_read(s->dev->conf.bs, &s->sg,
                              (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
                              pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
-    bdrv_acct_done(s->bs, &s->acct);
+    bdrv_acct_done(s->dev->conf.bs, &s->acct);
     io->dma_end(opaque);
 }
 
@@ -97,6 +97,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
+    BlockDriverState *bs = s->dev->conf.bs;
     int n;
     int64_t sector_num;
 
@@ -141,15 +142,15 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
+        m->aiocb = dma_bdrv_read(bs, &s->sg, sector_num,
 		                 pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
-        m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
+        m->aiocb = dma_bdrv_write(bs, &s->sg, sector_num,
 		                  pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
-        m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+        m->aiocb = dma_bdrv_io(bs, &s->sg, sector_num,
                                ide_issue_trim, pmac_ide_transfer_cb, s,
                                DMA_DIRECTION_TO_DEVICE);
         break;
@@ -158,7 +159,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
 done:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
-        bdrv_acct_done(s->bs, &s->acct);
+        bdrv_acct_done(bs, &s->acct);
     }
     io->dma_end(io);
 }
@@ -167,20 +168,21 @@ static void pmac_ide_transfer(DBDMA_io *io)
 {
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
+    BlockDriverState *bs = s->dev->conf.bs;
 
     s->io_buffer_size = 0;
     if (s->drive_kind == IDE_CD) {
-        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
+        bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_READ);
         pmac_ide_atapi_transfer_cb(io, 0);
         return;
     }
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
+        bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_READ);
         break;
     case IDE_DMA_WRITE:
-        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_WRITE);
+        bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_WRITE);
         break;
     default:
         break;
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9431bad..51ee93a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -184,7 +184,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
                 bdrv_detach_dev(di->bdrv, ds);
             }
             bdrv_close(di->bdrv);
-            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
             drive_put_ref(di);
         }
     }
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 01bbc74..dbbc3dd 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -148,7 +148,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
-    if (ide_init_drive(s, dev->conf.bs, kind,
+    if (ide_init_drive(s, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
                        dev->conf.cyls, dev->conf.heads, dev->conf.secs,
                        dev->chs_trans) < 0) {
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 06/10] ide: Drop redundant IDEState geometry members
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (4 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 05/10] ide: Drop redundant IDEState member bs Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 07/10] ide: Drop redundant IDEState member version Markus Armbruster
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

Members cylinders, heads, sectors, chs_trans are copies of
dev->conf.cyls, dev->conf.heads, dev->conf.secs, dev->chs_trans.
Copies were needed for non-qdevified controllers, which lacked dev.

Note that pci_piix3_xen_ide_unplug() did not clear the copies (it only
cleared the copy of bs).  Begs the question whether stale data could
have been used after unplug.  As far as I can tell, the copies were
used only when the copy of bs was non-null, thus no bug there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     | 52 ++++++++++++++++++++++++----------------------------
 hw/ide/internal.h |  5 +----
 hw/ide/qdev.c     | 12 +++++-------
 3 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9f306c6..d0b3ca6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -89,11 +89,11 @@ static void ide_identify(IDEState *s)
     memset(s->io_buffer, 0, 512);
     p = (uint16_t *)s->io_buffer;
     put_le16(p + 0, 0x0040);
-    put_le16(p + 1, s->cylinders);
-    put_le16(p + 3, s->heads);
-    put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
+    put_le16(p + 1, s->dev->conf.cyls);
+    put_le16(p + 3, s->dev->conf.heads);
+    put_le16(p + 4, 512 * s->dev->conf.secs); /* XXX: retired, remove ? */
     put_le16(p + 5, 512); /* XXX: retired, remove ? */
-    put_le16(p + 6, s->sectors);
+    put_le16(p + 6, s->dev->conf.secs);
     padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
     put_le16(p + 20, 3); /* XXX: retired, remove ? */
     put_le16(p + 21, 512); /* cache size in sectors */
@@ -108,10 +108,10 @@ static void ide_identify(IDEState *s)
     put_le16(p + 51, 0x200); /* PIO transfer cycle */
     put_le16(p + 52, 0x200); /* DMA transfer cycle */
     put_le16(p + 53, 1 | (1 << 1) | (1 << 2)); /* words 54-58,64-70,88 are valid */
-    put_le16(p + 54, s->cylinders);
-    put_le16(p + 55, s->heads);
-    put_le16(p + 56, s->sectors);
-    oldsize = s->cylinders * s->heads * s->sectors;
+    put_le16(p + 54, s->dev->conf.cyls);
+    put_le16(p + 55, s->dev->conf.heads);
+    put_le16(p + 56, s->dev->conf.secs);
+    oldsize = s->dev->conf.cyls * s->dev->conf.heads * s->dev->conf.secs;
     put_le16(p + 57, oldsize);
     put_le16(p + 58, oldsize >> 16);
     if (s->mult_sectors)
@@ -249,12 +249,12 @@ static void ide_cfata_identify(IDEState *s)
 
     memset(p, 0, sizeof(s->identify_data));
 
-    cur_sec = s->cylinders * s->heads * s->sectors;
+    cur_sec = s->dev->conf.cyls * s->dev->conf.heads * s->dev->conf.secs;
 
     put_le16(p + 0, 0x848a);			/* CF Storage Card signature */
-    put_le16(p + 1, s->cylinders);		/* Default cylinders */
-    put_le16(p + 3, s->heads);			/* Default heads */
-    put_le16(p + 6, s->sectors);		/* Default sectors per track */
+    put_le16(p + 1, s->dev->conf.cyls);         /* Default cylinders */
+    put_le16(p + 3, s->dev->conf.heads);        /* Default heads */
+    put_le16(p + 6, s->dev->conf.secs);         /* Default sectors per track */
     put_le16(p + 7, s->nb_sectors >> 16);	/* Sectors per card */
     put_le16(p + 8, s->nb_sectors);		/* Sectors per card */
     padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
@@ -270,9 +270,9 @@ static void ide_cfata_identify(IDEState *s)
     put_le16(p + 51, 0x0002);			/* PIO cycle timing mode */
     put_le16(p + 52, 0x0001);			/* DMA cycle timing mode */
     put_le16(p + 53, 0x0003);			/* Translation params valid */
-    put_le16(p + 54, s->cylinders);		/* Current cylinders */
-    put_le16(p + 55, s->heads);			/* Current heads */
-    put_le16(p + 56, s->sectors);		/* Current sectors */
+    put_le16(p + 54, s->dev->conf.cyls);        /* Current cylinders */
+    put_le16(p + 55, s->dev->conf.heads);       /* Current heads */
+    put_le16(p + 56, s->dev->conf.secs);        /* Current sectors */
     put_le16(p + 57, cur_sec);			/* Current capacity */
     put_le16(p + 58, cur_sec >> 16);		/* Current capacity */
     if (s->mult_sectors)			/* Multiple sector setting */
@@ -433,8 +433,10 @@ int64_t ide_get_sector(IDEState *s)
 		((int64_t) s->lcyl << 8) | s->sector;
 	}
     } else {
-        sector_num = ((s->hcyl << 8) | s->lcyl) * s->heads * s->sectors +
-            (s->select & 0x0f) * s->sectors + (s->sector - 1);
+        sector_num =
+            (((s->hcyl << 8) | s->lcyl) * s->dev->conf.heads
+             + (s->select & 0x0f)) * s->dev->conf.secs
+            + (s->sector - 1);
     }
     return sector_num;
 }
@@ -457,12 +459,12 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
 	    s->hob_hcyl = sector_num >> 40;
 	}
     } else {
-        cyl = sector_num / (s->heads * s->sectors);
-        r = sector_num % (s->heads * s->sectors);
+        cyl = sector_num / (s->dev->conf.heads * s->dev->conf.secs);
+        r = sector_num % (s->dev->conf.heads * s->dev->conf.secs);
         s->hcyl = cyl >> 8;
         s->lcyl = cyl;
-        s->select = (s->select & 0xf0) | ((r / s->sectors) & 0x0f);
-        s->sector = (r % s->sectors) + 1;
+        s->select = (s->select & 0xf0) | ((r / s->dev->conf.secs) & 0x0f);
+        s->sector = (r % s->dev->conf.secs) + 1;
     }
 }
 
@@ -1930,9 +1932,7 @@ static const BlockDevOps ide_cd_block_ops = {
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
-                   uint64_t wwn,
-                   uint32_t cylinders, uint32_t heads, uint32_t secs,
-                   int chs_trans)
+                   uint64_t wwn)
 {
     BlockDriverState *bs = s->dev->conf.bs;
     uint64_t nb_sectors;
@@ -1940,10 +1940,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
     s->drive_kind = kind;
 
     bdrv_get_geometry(bs, &nb_sectors);
-    s->cylinders = cylinders;
-    s->heads = heads;
-    s->sectors = secs;
-    s->chs_trans = chs_trans;
     s->nb_sectors = nb_sectors;
     s->wwn = wwn;
     /* The SMART values should be preserved across power cycles
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 6058db4..aa226e4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -346,7 +346,6 @@ struct IDEState {
     uint8_t unit;
     /* ide config */
     IDEDriveKind drive_kind;
-    int cylinders, heads, sectors, chs_trans;
     int64_t nb_sectors;
     int mult_sectors;
     int identify_set;
@@ -546,9 +545,7 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
-                   uint64_t wwn,
-                   uint32_t cylinders, uint32_t heads, uint32_t secs,
-                   int chs_trans);
+                   uint64_t wwn);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index dbbc3dd..82d9ca4 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -115,15 +115,15 @@ int ide_get_geometry(BusState *bus, int unit,
         return -1;
     }
 
-    *cyls = s->cylinders;
-    *heads = s->heads;
-    *secs = s->sectors;
+    *cyls = s->dev->conf.cyls;
+    *heads = s->dev->conf.heads;
+    *secs = s->dev->conf.secs;
     return 0;
 }
 
 int ide_get_bios_chs_trans(BusState *bus, int unit)
 {
-    return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
+    return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].dev->chs_trans;
 }
 
 /* --------------------------------- */
@@ -149,9 +149,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     if (ide_init_drive(s, kind,
-                       dev->version, dev->serial, dev->model, dev->wwn,
-                       dev->conf.cyls, dev->conf.heads, dev->conf.secs,
-                       dev->chs_trans) < 0) {
+                       dev->version, dev->serial, dev->model, dev->wwn) < 0) {
         return -1;
     }
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 07/10] ide: Drop redundant IDEState member version
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (5 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 06/10] ide: Drop redundant IDEState geometry members Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 08/10] ide: Drop redundant IDEState member drive_serial_str Markus Armbruster
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

It's a copy of dev->version.  The copy was needed for non-qdevified
controllers, which lacked dev.

Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs).  Begs the question whether stale data could
have been used after unplug.  As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/atapi.c    |  2 +-
 hw/ide/core.c     | 14 ++++----------
 hw/ide/internal.h |  3 +--
 hw/ide/qdev.c     |  9 +++++----
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index d077d18..240a62c 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -634,7 +634,7 @@ static void cmd_inquiry(IDEState *s, uint8_t *buf)
     buf[7] = 0; /* reserved */
     padstr8(buf + 8, 8, "QEMU");
     padstr8(buf + 16, 16, "QEMU DVD-ROM");
-    padstr8(buf + 32, 4, s->version);
+    padstr8(buf + 32, 4, s->dev->version);
     ide_atapi_cmd_reply(s, 36, max_len);
 }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d0b3ca6..05a2e61 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -98,7 +98,7 @@ static void ide_identify(IDEState *s)
     put_le16(p + 20, 3); /* XXX: retired, remove ? */
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
-    padstr((char *)(p + 23), s->version, 8); /* firmware version */
+    padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
     padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
@@ -202,7 +202,7 @@ static void ide_atapi_identify(IDEState *s)
     put_le16(p + 20, 3); /* buffer type */
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
-    padstr((char *)(p + 23), s->version, 8); /* firmware version */
+    padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
     padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
     put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
 #ifdef USE_DMA_CDROM
@@ -259,7 +259,7 @@ static void ide_cfata_identify(IDEState *s)
     put_le16(p + 8, s->nb_sectors);		/* Sectors per card */
     padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
     put_le16(p + 22, 0x0004);			/* ECC bytes */
-    padstr((char *) (p + 23), s->version, 8);	/* Firmware Revision */
+    padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
     padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
@@ -1931,7 +1931,7 @@ static const BlockDevOps ide_cd_block_ops = {
 };
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model,
+                   const char *serial, const char *model,
                    uint64_t wwn)
 {
     BlockDriverState *bs = s->dev->conf.bs;
@@ -1983,12 +1983,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
         }
     }
 
-    if (version) {
-        pstrcpy(s->version, sizeof(s->version), version);
-    } else {
-        pstrcpy(s->version, sizeof(s->version), qemu_get_version());
-    }
-
     ide_reset(s);
     bdrv_iostatus_enable(bs);
     return 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index aa226e4..544f3b1 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -373,7 +373,6 @@ struct IDEState {
 
     /* set for lba48 access */
     uint8_t lba48;
-    char version[9];
     /* ATAPI specific */
     struct unreported_events events;
     uint8_t sense_key;
@@ -544,7 +543,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model,
+                   const char *serial, const char *model,
                    uint64_t wwn);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 82d9ca4..0139cd1 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -148,14 +148,15 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
+    if (!dev->version) {
+        dev->version = g_strdup(qemu_get_version());
+    }
+
     if (ide_init_drive(s, kind,
-                       dev->version, dev->serial, dev->model, dev->wwn) < 0) {
+                       dev->serial, dev->model, dev->wwn) < 0) {
         return -1;
     }
 
-    if (!dev->version) {
-        dev->version = g_strdup(s->version);
-    }
     if (!dev->serial) {
         dev->serial = g_strdup(s->drive_serial_str);
     }
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 08/10] ide: Drop redundant IDEState member drive_serial_str
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (6 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 07/10] ide: Drop redundant IDEState member version Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 09/10] ide: Drop redundant IDEState member model Markus Armbruster
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

It's a copy of dev->serial.  The copy was needed for non-qdevified
controllers, which lacked dev.

Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs).  Begs the question whether stale data could
have been used after unplug.  As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     | 14 ++++----------
 hw/ide/internal.h |  3 +--
 hw/ide/qdev.c     | 10 +++++-----
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 05a2e61..2608f67 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -94,7 +94,7 @@ static void ide_identify(IDEState *s)
     put_le16(p + 4, 512 * s->dev->conf.secs); /* XXX: retired, remove ? */
     put_le16(p + 5, 512); /* XXX: retired, remove ? */
     put_le16(p + 6, s->dev->conf.secs);
-    padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+    padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
     put_le16(p + 20, 3); /* XXX: retired, remove ? */
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
@@ -198,7 +198,7 @@ static void ide_atapi_identify(IDEState *s)
     p = (uint16_t *)s->io_buffer;
     /* Removable CDROM, 50us response, 12 byte packets */
     put_le16(p + 0, (2 << 14) | (5 << 8) | (1 << 7) | (2 << 5) | (0 << 0));
-    padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+    padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
     put_le16(p + 20, 3); /* buffer type */
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
@@ -257,7 +257,7 @@ static void ide_cfata_identify(IDEState *s)
     put_le16(p + 6, s->dev->conf.secs);         /* Default sectors per track */
     put_le16(p + 7, s->nb_sectors >> 16);	/* Sectors per card */
     put_le16(p + 8, s->nb_sectors);		/* Sectors per card */
-    padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+    padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
     put_le16(p + 22, 0x0004);			/* ECC bytes */
     padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
     padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
@@ -1931,7 +1931,7 @@ static const BlockDevOps ide_cd_block_ops = {
 };
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *serial, const char *model,
+                   const char *model,
                    uint64_t wwn)
 {
     BlockDriverState *bs = s->dev->conf.bs;
@@ -1961,12 +1961,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
             return -1;
         }
     }
-    if (serial) {
-        pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
-    } else {
-        snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
-                 "QM%05d", s->drive_serial);
-    }
     if (model) {
         pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
     } else {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 544f3b1..28b60aa 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -351,7 +351,6 @@ struct IDEState {
     int identify_set;
     uint8_t identify_data[512];
     int drive_serial;
-    char drive_serial_str[21];
     char drive_model_str[41];
     uint64_t wwn;
     /* ide regs */
@@ -543,7 +542,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *serial, const char *model,
+                   const char *model,
                    uint64_t wwn);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 0139cd1..ef7af1f 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -148,19 +148,19 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
+    if (!dev->serial) {
+        dev->serial = g_strdup_printf("QM%05d", s->drive_serial);
+    }
+
     if (!dev->version) {
         dev->version = g_strdup(qemu_get_version());
     }
 
     if (ide_init_drive(s, kind,
-                       dev->serial, dev->model, dev->wwn) < 0) {
+                       dev->model, dev->wwn) < 0) {
         return -1;
     }
 
-    if (!dev->serial) {
-        dev->serial = g_strdup(s->drive_serial_str);
-    }
-
     add_boot_device_path(dev->conf.bootindex, &dev->qdev,
                          dev->unit ? "/disk@1" : "/disk@0");
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 09/10] ide: Drop redundant IDEState member model
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (7 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 08/10] ide: Drop redundant IDEState member drive_serial_str Markus Armbruster
@ 2012-12-17 14:05 ` Markus Armbruster
  2012-12-17 14:06 ` [Qemu-devel] [PATCH 10/10] ide: Drop redundant IDEState member wwn Markus Armbruster
  2012-12-18 13:35 ` [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Anthony Liguori
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

It's a copy of dev->serial.  The copy was needed for non-qdevified
controllers, which lacked dev.

Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs).  Begs the question whether stale data could
have been used after unplug.  As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     | 22 +++-------------------
 hw/ide/internal.h |  2 --
 hw/ide/qdev.c     | 16 +++++++++++++++-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 2608f67..a45a03f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -99,7 +99,7 @@ static void ide_identify(IDEState *s)
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
-    padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
+    padstr((char *)(p + 27), s->dev->model, 40);  /* model */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
 #endif
@@ -203,7 +203,7 @@ static void ide_atapi_identify(IDEState *s)
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
-    padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
+    padstr((char *)(p + 27), s->dev->model, 40);  /* model */
     put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
 #ifdef USE_DMA_CDROM
     put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
@@ -260,7 +260,7 @@ static void ide_cfata_identify(IDEState *s)
     padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
     put_le16(p + 22, 0x0004);			/* ECC bytes */
     padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
-    padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
+    padstr((char *) (p + 27), s->dev->model, 40);  /* Model number */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
 #else
@@ -1931,7 +1931,6 @@ static const BlockDevOps ide_cd_block_ops = {
 };
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *model,
                    uint64_t wwn)
 {
     BlockDriverState *bs = s->dev->conf.bs;
@@ -1961,21 +1960,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
             return -1;
         }
     }
-    if (model) {
-        pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
-    } else {
-        switch (kind) {
-        case IDE_CD:
-            strcpy(s->drive_model_str, "QEMU DVD-ROM");
-            break;
-        case IDE_CFATA:
-            strcpy(s->drive_model_str, "QEMU MICRODRIVE");
-            break;
-        default:
-            strcpy(s->drive_model_str, "QEMU HARDDISK");
-            break;
-        }
-    }
 
     ide_reset(s);
     bdrv_iostatus_enable(bs);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 28b60aa..e5546c1 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -351,7 +351,6 @@ struct IDEState {
     int identify_set;
     uint8_t identify_data[512];
     int drive_serial;
-    char drive_model_str[41];
     uint64_t wwn;
     /* ide regs */
     uint8_t feature;
@@ -542,7 +541,6 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   const char *model,
                    uint64_t wwn);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index ef7af1f..b1d25d7 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -156,8 +156,22 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         dev->version = g_strdup(qemu_get_version());
     }
 
+    if (!dev->model) {
+        switch (kind) {
+        case IDE_CD:
+            dev->model = g_strdup("QEMU DVD-ROM");
+            break;
+        case IDE_CFATA:
+            dev->model = g_strdup("QEMU MICRODRIVE");
+            break;
+        default:
+            dev->model = g_strdup("QEMU HARDDISK");
+            break;
+        }
+    }
+
     if (ide_init_drive(s, kind,
-                       dev->model, dev->wwn) < 0) {
+                       dev->wwn) < 0) {
         return -1;
     }
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Qemu-devel] [PATCH 10/10] ide: Drop redundant IDEState member wwn
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (8 preceding siblings ...)
  2012-12-17 14:05 ` [Qemu-devel] [PATCH 09/10] ide: Drop redundant IDEState member model Markus Armbruster
@ 2012-12-17 14:06 ` Markus Armbruster
  2012-12-18 13:35 ` [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Anthony Liguori
  10 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-17 14:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-ppc, agraf

It's a copy of dev->wwn.  The copy was needed for non-qdevified
controllers, which lacked dev.

Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs).  Begs the question whether stale data could
have been used after unplug.  As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     | 18 ++++++++----------
 hw/ide/internal.h |  4 +---
 hw/ide/qdev.c     |  3 +--
 3 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index a45a03f..a54a64b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -142,7 +142,7 @@ static void ide_identify(IDEState *s)
     /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
     put_le16(p + 83, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
     /* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
-    if (s->wwn) {
+    if (s->dev->wwn) {
         put_le16(p + 84, (1 << 14) | (1 << 8) | 0);
     } else {
         put_le16(p + 84, (1 << 14) | 0);
@@ -156,7 +156,7 @@ static void ide_identify(IDEState *s)
     /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
     put_le16(p + 86, (1 << 13) | (1 <<12) | (1 << 10));
     /* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
-    if (s->wwn) {
+    if (s->dev->wwn) {
         put_le16(p + 87, (1 << 14) | (1 << 8) | 0);
     } else {
         put_le16(p + 87, (1 << 14) | 0);
@@ -170,12 +170,12 @@ static void ide_identify(IDEState *s)
 
     if (dev && dev->conf.physical_block_size)
         put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
-    if (s->wwn) {
+    if (s->dev->wwn) {
         /* LE 16-bit words 111-108 contain 64-bit World Wide Name */
-        put_le16(p + 108, s->wwn >> 48);
-        put_le16(p + 109, s->wwn >> 32);
-        put_le16(p + 110, s->wwn >> 16);
-        put_le16(p + 111, s->wwn);
+        put_le16(p + 108, s->dev->wwn >> 48);
+        put_le16(p + 109, s->dev->wwn >> 32);
+        put_le16(p + 110, s->dev->wwn >> 16);
+        put_le16(p + 111, s->dev->wwn);
     }
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
@@ -1930,8 +1930,7 @@ static const BlockDevOps ide_cd_block_ops = {
     .is_medium_locked = ide_cd_is_medium_locked,
 };
 
-int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   uint64_t wwn)
+int ide_init_drive(IDEState *s, IDEDriveKind kind)
 {
     BlockDriverState *bs = s->dev->conf.bs;
     uint64_t nb_sectors;
@@ -1940,7 +1939,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
 
     bdrv_get_geometry(bs, &nb_sectors);
     s->nb_sectors = nb_sectors;
-    s->wwn = wwn;
     /* The SMART values should be preserved across power cycles
        but they aren't.  */
     s->smart_enabled = 1;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index e5546c1..352c87c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -351,7 +351,6 @@ struct IDEState {
     int identify_set;
     uint8_t identify_data[512];
     int drive_serial;
-    uint64_t wwn;
     /* ide regs */
     uint8_t feature;
     uint8_t error;
@@ -540,8 +539,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, IDEDriveKind kind,
-                   uint64_t wwn);
+int ide_init_drive(IDEState *s, IDEDriveKind kind);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b1d25d7..d7e4f7b 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -170,8 +170,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         }
     }
 
-    if (ide_init_drive(s, kind,
-                       dev->wwn) < 0) {
+    if (ide_init_drive(s, kind) < 0) {
         return -1;
     }
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up
  2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
                   ` (9 preceding siblings ...)
  2012-12-17 14:06 ` [Qemu-devel] [PATCH 10/10] ide: Drop redundant IDEState member wwn Markus Armbruster
@ 2012-12-18 13:35 ` Anthony Liguori
  2012-12-18 15:10   ` Markus Armbruster
  10 siblings, 1 reply; 24+ messages in thread
From: Anthony Liguori @ 2012-12-18 13:35 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: kwolf, qemu-ppc, agraf

Markus Armbruster <armbru@redhat.com> writes:

> *** Important ***
> This *breaks* all non-qdevified controllers, see PATCH 01/10.
> Maintainers are cc'ed.
>
> If you want still more time to qdevify your controller, please speak
> up now, and tell us how much.
>
> The rest of the series is obvious cleanups enabled by dropping the
> special case of a non-qdevified controller.  The block configuration
> stuff I'm working on also profits from it, and is real reason I'm
> posting this.

Breaking is not the right approach.  If you're asserting that the code
is unused and unloved, then remove it entirely from the tree.

Just breaking something is always wrong though.

I only see three users of ide_init2_with_non_qdev_drives.  Is there any
reason you didn't just convert these users to qdev?

Regards,

Anthony Liguori

>
> Markus Armbruster (10):
>   ide: Break all non-qdevified controllers
>   ide: Move IDEDevice pointer from IDEBus to IDEState
>   ide: Use IDEState member dev for "device connected" test
>   ide: Don't block-align IDEState member smart_selftest_data
>   ide: Drop redundant IDEState member bs
>   ide: Drop redundant IDEState geometry members
>   ide: Drop redundant IDEState member version
>   ide: Drop redundant IDEState member drive_serial_str
>   ide: Drop redundant IDEState member model
>   ide: Drop redundant IDEState member wwn
>
>  hw/ide/ahci.c       |  19 ++--
>  hw/ide/atapi.c      |  39 ++++----
>  hw/ide/core.c       | 278 +++++++++++++++++++++-------------------------------
>  hw/ide/internal.h   |  15 +--
>  hw/ide/macio.c      |  20 ++--
>  hw/ide/microdrive.c |   5 +-
>  hw/ide/piix.c       |   1 -
>  hw/ide/qdev.c       |  50 +++++-----
>  8 files changed, 189 insertions(+), 238 deletions(-)
>
> -- 
> 1.7.11.7

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up
  2012-12-18 13:35 ` [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Anthony Liguori
@ 2012-12-18 15:10   ` Markus Armbruster
  0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2012-12-18 15:10 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: kwolf, qemu-ppc, qemu-devel, agraf

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> *** Important ***
>> This *breaks* all non-qdevified controllers, see PATCH 01/10.
>> Maintainers are cc'ed.
>>
>> If you want still more time to qdevify your controller, please speak
>> up now, and tell us how much.
>>
>> The rest of the series is obvious cleanups enabled by dropping the
>> special case of a non-qdevified controller.  The block configuration
>> stuff I'm working on also profits from it, and is real reason I'm
>> posting this.
>
> Breaking is not the right approach.  If you're asserting that the code
> is unused and unloved, then remove it entirely from the tree.

Can do.

> Just breaking something is always wrong though.
>
> I only see three users of ide_init2_with_non_qdev_drives.  Is there any
> reason you didn't just convert these users to qdev?

Yes: it's more work than I can do on the side.

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2012-12-18 15:10 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-17 14:05 [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 01/10] ide: Break all non-qdevified controllers Markus Armbruster
2012-12-17 14:09   ` Alexander Graf
2012-12-17 14:43     ` Markus Armbruster
2012-12-17 14:55       ` Alexander Graf
2012-12-17 15:15         ` Markus Armbruster
2012-12-17 15:18           ` Alexander Graf
2012-12-17 15:38             ` Markus Armbruster
2012-12-17 21:50       ` Andreas Färber
2012-12-18 12:55         ` Markus Armbruster
2012-12-18 12:12   ` Peter Maydell
2012-12-18 12:44     ` Markus Armbruster
2012-12-18 13:56       ` Peter Maydell
2012-12-17 14:05 ` [Qemu-devel] [PATCH 02/10] ide: Move IDEDevice pointer from IDEBus to IDEState Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 03/10] ide: Use IDEState member dev for "device connected" test Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 04/10] ide: Don't block-align IDEState member smart_selftest_data Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 05/10] ide: Drop redundant IDEState member bs Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 06/10] ide: Drop redundant IDEState geometry members Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 07/10] ide: Drop redundant IDEState member version Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 08/10] ide: Drop redundant IDEState member drive_serial_str Markus Armbruster
2012-12-17 14:05 ` [Qemu-devel] [PATCH 09/10] ide: Drop redundant IDEState member model Markus Armbruster
2012-12-17 14:06 ` [Qemu-devel] [PATCH 10/10] ide: Drop redundant IDEState member wwn Markus Armbruster
2012-12-18 13:35 ` [Qemu-devel] [PATCH 00/10] Drop code for non-qdevified IDE, and clean up Anthony Liguori
2012-12-18 15:10   ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).