Patchwork [01/10] ide: Break all non-qdevified controllers

login
register
mail settings
Submitter Markus Armbruster
Date Dec. 17, 2012, 2:05 p.m.
Message ID <1355753160-17544-2-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/206868/
State New
Headers show

Comments

Markus Armbruster - Dec. 17, 2012, 2:05 p.m.
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(-)
Alexander Graf - Dec. 17, 2012, 2:09 p.m.
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
Markus Armbruster - Dec. 17, 2012, 2:43 p.m.
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!
Alexander Graf - Dec. 17, 2012, 2:55 p.m.
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
Markus Armbruster - Dec. 17, 2012, 3:15 p.m.
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.
Alexander Graf - Dec. 17, 2012, 3:18 p.m.
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
Markus Armbruster - Dec. 17, 2012, 3:38 p.m.
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.
Andreas Färber - Dec. 17, 2012, 9:50 p.m.
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
Peter Maydell - Dec. 18, 2012, 12:12 p.m.
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
Markus Armbruster - Dec. 18, 2012, 12:44 p.m.
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
Markus Armbruster - Dec. 18, 2012, 12:55 p.m.
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 :)
Peter Maydell - Dec. 18, 2012, 1:56 p.m.
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

Patch

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