From: John Snow <jsnow@redhat.com>
To: "BALATON Zoltan" <balaton@eik.bme.hu>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>,
John G Johnson <john.g.johnson@oracle.com>,
Thomas Huth <thuth@redhat.com>,
Jagannathan Raman <jag.raman@oracle.com>,
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link
Date: Wed, 19 May 2021 17:49:35 -0400 [thread overview]
Message-ID: <3ba44704-6418-4aee-23ad-7d4dcc1fe60d@redhat.com> (raw)
In-Reply-To: <f571a63d-d6a2-2085-740-bcc59b3424e5@eik.bme.hu>
On 5/18/21 7:05 PM, BALATON Zoltan wrote:
> On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
>> IDE bus depends on ISA bus for IRQ/DMA.
>>
>> Add an ISABus reference in PCIIDEState, and add link properties
>> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> include/hw/ide/pci.h | 1 +
>> hw/ide/piix.c | 11 ++++++++++-
>> hw/ide/via.c | 10 +++++++++-
>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index d8384e1c422..e790722ed14 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -47,6 +47,7 @@ struct PCIIDEState {
>> PCIDevice parent_obj;
>> /*< public >*/
>>
>> + ISABus *isa_bus;
>
> I'm not sure that this belongs here. Previously we managed to remove
> device specific fields from this structure so it's now really just holds
> stuff related to PCI IDE (except the remaining "secondary" field
> specific to CMD646). PCI IDE normaly has nothing to do with ISA except
> for those south bridges that have IDE with legacy mode. So this ISABus
> reference should be in those south bridges instead. But that may need a
by "those south bridges" I assume you are referring to the integrated
PIIX and VIA controller implementations.
> new subclass just for this so putting it here is just avoiding
> boilerplate of declaring new subclasses in piix and via-ide. I can
> sympathise with that but I'd still prefer to keep it off here but I
> wonder if there's a way to do that without subclassing and storing an
> ISABus ref? If I understand correctly this ISABus ref is just needed to
> get appropriate ISA irqs. But could we just store a ref to those irqs
It looks like it's just the IRQs, yeah.
> directly so we don't need to keep the ref to the ISA bus? There's
I think the idea actually is to formalize the dependency of these models
on the ISA bus instead of hacking / faking one. I think we DO want the
dependency.
> already a qemu_irq in BMDMAState but I'm not sure how those are set and
> if you could store an isa irq there to simplify this. I don't know the
> details and could not detangle it by a brief look so not sure it can be
> done but conceptually it feels better to keep PCI IDE separate from ISA
> and let it raise either PCI irq or ISA irq as needed. For that a ref to
> the irq should be enough and that can either come from a PCI bus (which
> is normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope
> it makes sense and you get what I'm trying to say. (Longer term we may
> want to make it changeable also after the device is created to allow
> switching between legacy and PCI mode but so far we could get away
> without emulating that so it's not a requirement just something to
> consider when you're changing this. The real problem that prevents
> switching modes is not irq I think but ioports and that ISA devices are
> not configurable after creating them but that would need QOM'ifying ISA
> emulation which probably does not worth the effort unless we come across
> some guest that needs this.)
>
> Regards,
> BALATON Zoltan
>
I assume the idea here is that PCIIDE does not technically need "ISA" to
provide ioport access to the ATA drives, so taxonomically it's odd for
the generic/abstract PCIIDE device to require an ISA bus.
Am I understanding correctly?
>> IDEBus bus[2];
>> BMDMAState bmdma[2];
>> uint32_t secondary; /* used only for cmd646 */
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index b9860e35a5c..48da68da37f 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -30,8 +30,9 @@
>> #include "sysemu/block-backend.h"
>> #include "sysemu/blockdev.h"
>> #include "sysemu/dma.h"
>> -
>> +#include "qapi/error.h"
>> #include "hw/ide/pci.h"
>> +#include "hw/isa/isa.h"
>> #include "trace.h"
>>
>> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
>> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>> }
>> }
>>
>> +static Property piix_ide_properties[] = {
>> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>> + TYPE_ISA_BUS, ISABus *),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>> static void piix3_ide_class_init(ObjectClass *klass, void *data)
>> {
>> @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass
>> *klass, void *data)
>> k->class_id = PCI_CLASS_STORAGE_IDE;
>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> dc->hotpluggable = false;
>> + device_class_set_props(dc, piix_ide_properties);
>> }
>>
>> static const TypeInfo piix3_ide_info = {
>> @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass
>> *klass, void *data)
>> k->class_id = PCI_CLASS_STORAGE_IDE;
>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> dc->hotpluggable = false;
>> + device_class_set_props(dc, piix_ide_properties);
>> }
>>
>> static const TypeInfo piix4_ide_info = {
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index be09912b334..65fdca6dcf4 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -28,8 +28,9 @@
>> #include "hw/pci/pci.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> +#include "qapi/error.h"
>> #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/isa.h"
>> #include "hw/ide/pci.h"
>> #include "trace.h"
>>
>> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>> }
>> }
>>
>> +static Property via_ide_properties[] = {
>> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
>> + TYPE_ISA_BUS, ISABus *),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> static void via_ide_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass,
>> void *data)
>> k->revision = 0x06;
>> k->class_id = PCI_CLASS_STORAGE_IDE;
>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> + device_class_set_props(dc, via_ide_properties);
>> }
>>
>> static const TypeInfo via_ide_info = {
>>
next prev parent reply other threads:[~2021-05-19 21:50 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-18 21:55 [RFC PATCH 00/11] hw/isa: Remove dependencies on ISA bus singleton Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 01/11] hw/isa: Explode pci_create_simple() calls Philippe Mathieu-Daudé
2021-05-21 7:09 ` Markus Armbruster
2021-05-18 21:55 ` [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link Philippe Mathieu-Daudé
2021-05-18 23:05 ` BALATON Zoltan
2021-05-19 21:49 ` John Snow [this message]
2021-05-20 0:46 ` BALATON Zoltan
2021-05-20 8:35 ` Stefan Hajnoczi
2021-05-20 8:56 ` Mark Cave-Ayland
2021-05-20 12:18 ` BALATON Zoltan
2021-05-20 7:41 ` Mark Cave-Ayland
2021-05-20 8:29 ` Mark Cave-Ayland
2021-05-18 21:55 ` [RFC PATCH 03/11] hw/ide/piix: Set the ISA-bus QOM link Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 04/11] hw/ide/via: " Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 05/11] hw/isa: Extract isa_bus_get_irq() from isa_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 06/11] hw/ide: Replace isa_get_irq() by isa_bus_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 07/11] hw/isa: Simplify isa_get_irq() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 08/11] hw/isa: Extract bus part from isa_register_portio_list() Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 09/11] hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device Philippe Mathieu-Daudé
2021-05-18 21:55 ` [RFC PATCH 10/11] hw/isa: Remove use of global isa bus Philippe Mathieu-Daudé
2021-05-19 16:11 ` Stefan Hajnoczi
2021-05-18 21:55 ` [RFC PATCH 11/11] hw/isa: Rename isabus singleton as 'g_isabus' Philippe Mathieu-Daudé
2021-05-19 16:18 ` Stefan Hajnoczi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ba44704-6418-4aee-23ad-7d4dcc1fe60d@redhat.com \
--to=jsnow@redhat.com \
--cc=armbru@redhat.com \
--cc=balaton@eik.bme.hu \
--cc=elena.ufimtseva@oracle.com \
--cc=jag.raman@oracle.com \
--cc=john.g.johnson@oracle.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.