Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:

[...]

>> > The approach in this patch is okay but we should keep in mind it only
>> > solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
>> > more instances of this type of bug.
>> >
>> > A qdev fix would address the root cause and make it possible to drop the
>> > backdoor API, but that's probably too much work for little benefit.
>> 
>> What do you mean by backdoor API?  Global @isabus?
>
> Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq)
> accepts dev = NULL as a valid argument.

@isabus is static in hw/isa/isa-bus.c.  Uses:

* Limit isa_bus_new() to one ISA bus.  Arbitrary restriction; multiple
  ISA buses could work with suitable memory mapping and IRQ wiring.
  "Single ISA bus" assumptions could of course hide elsewhere in the
  code.

* Implied argument to isa_get_irq(), isa_register_ioport(),
  isa_register_portio_list(), isa_address_space(),
  isa_address_space_io().

  isa_get_irq() asserts that a non-null @dev is a child of @isabus.
  This means we don't actually need @isabus, except when @dev is null.
  I suspect two separate functions would be cleaner: one taking an
  ISABus * argument, and a wrapper taking an ISADevice * argument.

  isa_address_space() and isa_address_space_io() work the same, less the
  assertion.

  isa_register_ioport() and isa_register_portio_list() take a non-null
  @dev argument.  They don't actually need @isabus.

To eliminate global @isabus, we need to fix up the callers passing null
@dev.  Clean solution: plumb the ISABus returned by isa_bus_new() to the
call sites.  Where that's impractical, we can also get it from QOM, like
build_dsdt_microvm() does.