[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()
From: |
Bernhard Beschow |
Subject: |
Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq() |
Date: |
Sat, 13 May 2023 11:53:59 +0000 |
Am 27. April 2023 12:31:10 UTC schrieb Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk>:
>On 26/04/2023 19:25, Bernhard Beschow wrote:
>
>> Am 26. April 2023 11:33:40 UTC schrieb Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk>:
>>> On 22/04/2023 16:07, Bernhard Beschow wrote:
>>>
>>>> isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
>>>> Passing a NULL pointer works but causes the isabus global to be used
>>>> then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
>>>> achieve the same as using isa_get_irq().
>>>>
>>>> This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
>>>> crash when plugging a piix3-ide device into the x-remote machine' which
>>>> allows for cleaning up the ISA API while keeping PIIX IDE functions
>>>> user-createable.
>>>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/ide/piix.c | 23 ++++++++++++++++++++---
>>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>> index 6942b484f9..a3a15dc7db 100644
>>>> --- a/hw/ide/piix.c
>>>> +++ b/hw/ide/piix.c
>>>> @@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
>>>> pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
>>>> }
>>>> -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>>>> +static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>>>> + Error **errp)
>>>> {
>>>> static const struct {
>>>> int iobase;
>>>> @@ -124,7 +125,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned
>>>> i, Error **errp)
>>>> object_get_typename(OBJECT(d)), i);
>>>> return false;
>>>> }
>>>> - ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL,
>>>> port_info[i].isairq));
>>>> + ide_bus_init_output_irq(&d->bus[i],
>>>> + isa_bus_get_irq(isa_bus,
>>>> port_info[i].isairq));
>>>
>>> I don't think is the right solution here, since ultimately we want to move
>>> the IRQ routing out of the device itself and into the PCI-ISA bridge. I'd
>>> go for the same solution as you've done for VIA IDE in patch 2, i.e. update
>>> the PIIX interrupt handler to set the legacy_irqs in PCIIDEState, and then
>>> wire them up to the ISA IRQs 14 and 15 similar to as Phil as done in his
>>> patches:
>>
>> The problem is user-creatable PIIX-IDE. IMO we should stick to our
>> deprecation process before going this step because this will break it.
>
>Thomas posted some links from previous discussions where it seems that this
>hack is still in use:
>
>https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg00780.html
>https://lists.gnu.org/archive/html/qemu-block/2021-04/msg00746.html
>
>So it seems we can't even deprecate this, as it's working around missing
>functionality in q35 :(
>
>Certainly it seems that we should add a check that will fail the machine if
>there is more than one -device piix3-ide on the command line, since I can't
>see that could ever work properly.
>
>I'm leaning towards adding a device property that must be set to enabled in
>order for PIIX IDE realize() to succeed, leave it disabled by default and only
>enable it for the q35 machine. Does that seem like a reasonable solution?
I'd rather declare this to be out of scope of this series. First, this series
contains a lot of material already. Second, this patch attempts to preserve
current behavior.
This patch is actually a preparation for the next one. In the next patch the
(non-obvious) check for presence of the ISABus get removed so we need this
patch to preserve behavior. Otherwise machines without an ISA bus will crash if
piix3-ide gets user-created. One machine that would crash is the "remote"
machine IIRC.
Best regards,
Bernhard
>
>>> https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-4-philmd@linaro.org/
>>>
>>> https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-5-philmd@linaro.org/
>>>
>>> This also reminds me, given that the first patch above is doing wiring in
>>> pc_init1() then we are still missing part of your tidy-up series :/
>>>
>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>> ide_bus_register_restart_cb(&d->bus[i]);
>>>> @@ -136,14 +138,29 @@ static void pci_piix_ide_realize(PCIDevice *dev,
>>>> Error **errp)
>>>> {
>>>> PCIIDEState *d = PCI_IDE(dev);
>>>> uint8_t *pci_conf = dev->config;
>>>> + ISABus *isa_bus;
>>>> + bool ambiguous;
>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>> bmdma_init_ops(d, &piix_bmdma_ops);
>>>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops);
>>>> + isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS,
>>>> &ambiguous));
>>>> + if (ambiguous) {
>>>> + error_setg(errp,
>>>> + "More than one ISA bus found while %s supports only
>>>> one",
>>>> + object_get_typename(OBJECT(d)));
>>>> + return;
>>>> + }
>>>> + if (!isa_bus) {
>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>> + object_get_typename(OBJECT(d)));
>>>> + return;
>>>> + }
>>>
>>> Again I think this should go away with using PCIIDEState's legacy_irqs,
>>> since you simply let the board wire them up to the ISABus (or not) as
>>> required.
>>
>> Same here: This breaks user-creatable PIIX-IDE.
>>
>>>
>>>> for (unsigned i = 0; i < 2; i++) {
>>>> - if (!pci_piix_init_bus(d, i, errp)) {
>>>> + if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>>> return;
>>>> }
>>>> }
>
>
>ATB,
>
>Mark.
- Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq(),
Bernhard Beschow <=