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

[Philippe Mathieu-Daudé]

Hi Elena,

+Mark

You asked to use the next KVM-external call slot to talk about
the ISA bus issues. I haven't scheduled the call because it seems
this thread helped to figure the problems and Markus's analysis
resumed them all. From here it should be clearer to see what has
to be done to go where we want to be from where we are :)

What do you think (in particular, from Markus list)?

On 4/29/21 8:08 AM, Markus Armbruster wrote:
> 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.
>