* [PATCH v2 01/10] softmmu/ioport: Move portio_list_init() in front of portio_list_add()
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 02/10] softmmu/ioport: Merge portio_list_add() into portio_list_init() Bernhard Beschow
` (10 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
This is a preparation for the next patch to keep its diff smaller.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
softmmu/ioport.c | 41 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index cb8adb0b93..215344467b 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -113,27 +113,6 @@ uint32_t cpu_inl(uint32_t addr)
return val;
}
-void portio_list_init(PortioList *piolist,
- Object *owner,
- const MemoryRegionPortio *callbacks,
- void *opaque, const char *name)
-{
- unsigned n = 0;
-
- while (callbacks[n].size) {
- ++n;
- }
-
- piolist->ports = callbacks;
- piolist->nr = 0;
- piolist->regions = g_new0(MemoryRegion *, n);
- piolist->address_space = NULL;
- piolist->opaque = opaque;
- piolist->owner = owner;
- piolist->name = name;
- piolist->flush_coalesced_mmio = false;
-}
-
void portio_list_set_flush_coalesced(PortioList *piolist)
{
piolist->flush_coalesced_mmio = true;
@@ -250,6 +229,26 @@ static void portio_list_add_1(PortioList *piolist,
++piolist->nr;
}
+void portio_list_init(PortioList *piolist, Object *owner,
+ const MemoryRegionPortio *callbacks,
+ void *opaque, const char *name)
+{
+ unsigned n = 0;
+
+ while (callbacks[n].size) {
+ ++n;
+ }
+
+ piolist->ports = callbacks;
+ piolist->nr = 0;
+ piolist->regions = g_new0(MemoryRegion *, n);
+ piolist->address_space = NULL;
+ piolist->opaque = opaque;
+ piolist->owner = owner;
+ piolist->name = name;
+ piolist->flush_coalesced_mmio = false;
+}
+
void portio_list_add(PortioList *piolist,
MemoryRegion *address_space,
uint32_t start)
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 02/10] softmmu/ioport: Merge portio_list_add() into portio_list_init()
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 01/10] softmmu/ioport: Move portio_list_init() in front of portio_list_add() Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-02-05 21:34 ` Mark Cave-Ayland
2023-01-26 21:17 ` [PATCH v2 03/10] softmmu/ioport: Remove unused functions Bernhard Beschow
` (9 subsequent siblings)
11 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow, 20210518215545.1793947-9-philmd
Both functions are always used together and in the same order. Let's
reflect this in the API.
Inspired-by: <20210518215545.1793947-9-philmd@redhat.com>
'hw/isa: Extract bus part from isa_register_portio_list()'
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/exec/ioport.h | 6 ++----
hw/audio/adlib.c | 4 ++--
hw/display/qxl.c | 5 ++---
hw/display/vga.c | 8 ++++----
hw/dma/i82374.c | 6 ++----
hw/isa/isa-bus.c | 6 ++----
hw/watchdog/wdt_ib700.c | 4 ++--
softmmu/ioport.c | 19 +++++++------------
8 files changed, 23 insertions(+), 35 deletions(-)
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index e34f668998..ec3e8e5942 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -64,12 +64,10 @@ typedef struct PortioList {
void portio_list_init(PortioList *piolist, Object *owner,
const struct MemoryRegionPortio *callbacks,
- void *opaque, const char *name);
+ void *opaque, const char *name,
+ MemoryRegion *address_space_io, uint16_t start);
void portio_list_set_flush_coalesced(PortioList *piolist);
void portio_list_destroy(PortioList *piolist);
-void portio_list_add(PortioList *piolist,
- struct MemoryRegion *address_space,
- uint32_t addr);
void portio_list_del(PortioList *piolist);
#endif /* IOPORT_H */
diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 5f979b1487..957abe3da7 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -291,8 +291,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
adlib_portio_list[0].offset = s->port;
adlib_portio_list[1].offset = s->port + 8;
- portio_list_init (&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib");
- portio_list_add (&s->port_list, isa_address_space_io(&s->parent_obj), 0);
+ portio_list_init(&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib",
+ isa_address_space_io(&s->parent_obj), 0);
}
static Property adlib_properties[] = {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..6d5a931425 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2224,10 +2224,9 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
}
vga_init(vga, OBJECT(dev),
pci_address_space(dev), pci_address_space_io(dev), false);
- portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
- vga, "vga");
+ portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list, vga,
+ "vga", pci_address_space_io(dev), 0x3b0);
portio_list_set_flush_coalesced(&qxl->vga_port_list);
- portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
qxl->have_vga = true;
vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 7a5fdff649..2b606a526c 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2309,12 +2309,12 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
1);
memory_region_set_coalescing(vga_io_memory);
if (init_vga_ports) {
- portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
+ portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga",
+ address_space_io, 0x3b0);
portio_list_set_flush_coalesced(&s->vga_port_list);
- portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
}
if (vbe_ports) {
- portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
- portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
+ portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe",
+ address_space_io, 0x1ce);
}
}
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 34c3aaf7d3..5914b34079 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -131,10 +131,8 @@ static void i82374_realize(DeviceState *dev, Error **errp)
}
i8257_dma_init(isa_bus, true);
- portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
- "i82374");
- portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
- s->iobase);
+ portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s, "i82374",
+ isa_address_space_io(&s->parent_obj), s->iobase);
memset(s->commands, 0, sizeof(s->commands));
}
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 1bee1a47f1..b3497dad61 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -124,8 +124,6 @@ int isa_register_portio_list(ISADevice *dev,
const MemoryRegionPortio *pio_start,
void *opaque, const char *name)
{
- assert(piolist && !piolist->owner);
-
if (!isabus) {
return -ENODEV;
}
@@ -135,8 +133,8 @@ int isa_register_portio_list(ISADevice *dev,
actually handled e.g. the FDC device. */
isa_init_ioport(dev, start);
- portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
- portio_list_add(piolist, isabus->address_space_io, start);
+ portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name,
+ isabus->address_space_io, start);
return 0;
}
diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index b116c3a3aa..e53e474b83 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -115,8 +115,8 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
- portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700");
- portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
+ portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700",
+ isa_address_space_io(&s->parent_obj), 0);
}
static void wdt_ib700_reset(DeviceState *dev)
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index 215344467b..c92e3cb27d 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -231,8 +231,13 @@ static void portio_list_add_1(PortioList *piolist,
void portio_list_init(PortioList *piolist, Object *owner,
const MemoryRegionPortio *callbacks,
- void *opaque, const char *name)
+ void *opaque, const char *name,
+ MemoryRegion *address_space_io, uint16_t start)
{
+ assert(piolist && !piolist->owner);
+
+ const MemoryRegionPortio *pio, *pio_start = callbacks;
+ unsigned int off_low, off_high, off_last, count;
unsigned n = 0;
while (callbacks[n].size) {
@@ -242,21 +247,11 @@ void portio_list_init(PortioList *piolist, Object *owner,
piolist->ports = callbacks;
piolist->nr = 0;
piolist->regions = g_new0(MemoryRegion *, n);
- piolist->address_space = NULL;
+ piolist->address_space = address_space_io;
piolist->opaque = opaque;
piolist->owner = owner;
piolist->name = name;
piolist->flush_coalesced_mmio = false;
-}
-
-void portio_list_add(PortioList *piolist,
- MemoryRegion *address_space,
- uint32_t start)
-{
- const MemoryRegionPortio *pio, *pio_start = piolist->ports;
- unsigned int off_low, off_high, off_last, count;
-
- piolist->address_space = address_space;
/* Handle the first entry specially. */
off_last = off_low = pio_start->offset;
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 02/10] softmmu/ioport: Merge portio_list_add() into portio_list_init()
2023-01-26 21:17 ` [PATCH v2 02/10] softmmu/ioport: Merge portio_list_add() into portio_list_init() Bernhard Beschow
@ 2023-02-05 21:34 ` Mark Cave-Ayland
0 siblings, 0 replies; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 21:34 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, 20210518215545.1793947-9-philmd
On 26/01/2023 21:17, Bernhard Beschow wrote:
> Both functions are always used together and in the same order. Let's
> reflect this in the API.
>
> Inspired-by: <20210518215545.1793947-9-philmd@redhat.com>
> 'hw/isa: Extract bus part from isa_register_portio_list()'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/exec/ioport.h | 6 ++----
> hw/audio/adlib.c | 4 ++--
> hw/display/qxl.c | 5 ++---
> hw/display/vga.c | 8 ++++----
> hw/dma/i82374.c | 6 ++----
> hw/isa/isa-bus.c | 6 ++----
> hw/watchdog/wdt_ib700.c | 4 ++--
> softmmu/ioport.c | 19 +++++++------------
> 8 files changed, 23 insertions(+), 35 deletions(-)
>
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index e34f668998..ec3e8e5942 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -64,12 +64,10 @@ typedef struct PortioList {
>
> void portio_list_init(PortioList *piolist, Object *owner,
> const struct MemoryRegionPortio *callbacks,
> - void *opaque, const char *name);
> + void *opaque, const char *name,
> + MemoryRegion *address_space_io, uint16_t start);
> void portio_list_set_flush_coalesced(PortioList *piolist);
> void portio_list_destroy(PortioList *piolist);
> -void portio_list_add(PortioList *piolist,
> - struct MemoryRegion *address_space,
> - uint32_t addr);
> void portio_list_del(PortioList *piolist);
>
> #endif /* IOPORT_H */
> diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
> index 5f979b1487..957abe3da7 100644
> --- a/hw/audio/adlib.c
> +++ b/hw/audio/adlib.c
> @@ -291,8 +291,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
>
> adlib_portio_list[0].offset = s->port;
> adlib_portio_list[1].offset = s->port + 8;
> - portio_list_init (&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib");
> - portio_list_add (&s->port_list, isa_address_space_io(&s->parent_obj), 0);
> + portio_list_init(&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib",
> + isa_address_space_io(&s->parent_obj), 0);
> }
>
> static Property adlib_properties[] = {
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index ec712d3ca2..6d5a931425 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2224,10 +2224,9 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
> }
> vga_init(vga, OBJECT(dev),
> pci_address_space(dev), pci_address_space_io(dev), false);
> - portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
> - vga, "vga");
> + portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list, vga,
> + "vga", pci_address_space_io(dev), 0x3b0);
> portio_list_set_flush_coalesced(&qxl->vga_port_list);
> - portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
> qxl->have_vga = true;
>
> vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 7a5fdff649..2b606a526c 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2309,12 +2309,12 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
> 1);
> memory_region_set_coalescing(vga_io_memory);
> if (init_vga_ports) {
> - portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> + portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga",
> + address_space_io, 0x3b0);
> portio_list_set_flush_coalesced(&s->vga_port_list);
> - portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> }
> if (vbe_ports) {
> - portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> - portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
> + portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe",
> + address_space_io, 0x1ce);
> }
> }
> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> index 34c3aaf7d3..5914b34079 100644
> --- a/hw/dma/i82374.c
> +++ b/hw/dma/i82374.c
> @@ -131,10 +131,8 @@ static void i82374_realize(DeviceState *dev, Error **errp)
> }
> i8257_dma_init(isa_bus, true);
>
> - portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
> - "i82374");
> - portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
> - s->iobase);
> + portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s, "i82374",
> + isa_address_space_io(&s->parent_obj), s->iobase);
>
> memset(s->commands, 0, sizeof(s->commands));
> }
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 1bee1a47f1..b3497dad61 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -124,8 +124,6 @@ int isa_register_portio_list(ISADevice *dev,
> const MemoryRegionPortio *pio_start,
> void *opaque, const char *name)
> {
> - assert(piolist && !piolist->owner);
> -
> if (!isabus) {
> return -ENODEV;
> }
> @@ -135,8 +133,8 @@ int isa_register_portio_list(ISADevice *dev,
> actually handled e.g. the FDC device. */
> isa_init_ioport(dev, start);
>
> - portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
> - portio_list_add(piolist, isabus->address_space_io, start);
> + portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name,
> + isabus->address_space_io, start);
>
> return 0;
> }
> diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
> index b116c3a3aa..e53e474b83 100644
> --- a/hw/watchdog/wdt_ib700.c
> +++ b/hw/watchdog/wdt_ib700.c
> @@ -115,8 +115,8 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
>
> s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
>
> - portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700");
> - portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
> + portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700",
> + isa_address_space_io(&s->parent_obj), 0);
> }
>
> static void wdt_ib700_reset(DeviceState *dev)
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index 215344467b..c92e3cb27d 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -231,8 +231,13 @@ static void portio_list_add_1(PortioList *piolist,
>
> void portio_list_init(PortioList *piolist, Object *owner,
> const MemoryRegionPortio *callbacks,
> - void *opaque, const char *name)
> + void *opaque, const char *name,
> + MemoryRegion *address_space_io, uint16_t start)
> {
> + assert(piolist && !piolist->owner);
> +
> + const MemoryRegionPortio *pio, *pio_start = callbacks;
> + unsigned int off_low, off_high, off_last, count;
> unsigned n = 0;
>
> while (callbacks[n].size) {
> @@ -242,21 +247,11 @@ void portio_list_init(PortioList *piolist, Object *owner,
> piolist->ports = callbacks;
> piolist->nr = 0;
> piolist->regions = g_new0(MemoryRegion *, n);
> - piolist->address_space = NULL;
> + piolist->address_space = address_space_io;
> piolist->opaque = opaque;
> piolist->owner = owner;
> piolist->name = name;
> piolist->flush_coalesced_mmio = false;
> -}
> -
> -void portio_list_add(PortioList *piolist,
> - MemoryRegion *address_space,
> - uint32_t start)
> -{
> - const MemoryRegionPortio *pio, *pio_start = piolist->ports;
> - unsigned int off_low, off_high, off_last, count;
> -
> - piolist->address_space = address_space;
>
> /* Handle the first entry specially. */
> off_last = off_low = pio_start->offset;
As a general comment: is portio used outside of ISA bus devices at all? It seems to
me that portio is really a wrapper around the the old x86 IO port functionality, and
so I could see PortioList being moved exclusively to within ISADevice which could
allow the address_space, opaque and owner to be resolved automatically with the
PortioList being owned by ISADevice.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 03/10] softmmu/ioport: Remove unused functions
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 01/10] softmmu/ioport: Move portio_list_init() in front of portio_list_add() Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 02/10] softmmu/ioport: Merge portio_list_add() into portio_list_init() Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-02-05 21:37 ` Mark Cave-Ayland
2023-01-26 21:17 ` [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq() Bernhard Beschow
` (8 subsequent siblings)
11 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/exec/ioport.h | 2 --
softmmu/ioport.c | 24 ------------------------
2 files changed, 26 deletions(-)
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index ec3e8e5942..1ef5aebba3 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -67,7 +67,5 @@ void portio_list_init(PortioList *piolist, Object *owner,
void *opaque, const char *name,
MemoryRegion *address_space_io, uint16_t start);
void portio_list_set_flush_coalesced(PortioList *piolist);
-void portio_list_destroy(PortioList *piolist);
-void portio_list_del(PortioList *piolist);
#endif /* IOPORT_H */
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index c92e3cb27d..0a55d39196 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -118,19 +118,6 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
piolist->flush_coalesced_mmio = true;
}
-void portio_list_destroy(PortioList *piolist)
-{
- MemoryRegionPortioList *mrpio;
- unsigned i;
-
- for (i = 0; i < piolist->nr; ++i) {
- mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
- object_unparent(OBJECT(&mrpio->mr));
- g_free(mrpio);
- }
- g_free(piolist->regions);
-}
-
static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
uint64_t offset, unsigned size,
bool write)
@@ -280,14 +267,3 @@ void portio_list_init(PortioList *piolist, Object *owner,
/* There will always be an open sub-list. */
portio_list_add_1(piolist, pio_start, count, start, off_low, off_high);
}
-
-void portio_list_del(PortioList *piolist)
-{
- MemoryRegionPortioList *mrpio;
- unsigned i;
-
- for (i = 0; i < piolist->nr; ++i) {
- mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
- memory_region_del_subregion(piolist->address_space, &mrpio->mr);
- }
-}
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 03/10] softmmu/ioport: Remove unused functions
2023-01-26 21:17 ` [PATCH v2 03/10] softmmu/ioport: Remove unused functions Bernhard Beschow
@ 2023-02-05 21:37 ` Mark Cave-Ayland
2023-02-06 0:20 ` Bernhard Beschow
0 siblings, 1 reply; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 21:37 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On 26/01/2023 21:17, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/exec/ioport.h | 2 --
> softmmu/ioport.c | 24 ------------------------
> 2 files changed, 26 deletions(-)
>
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index ec3e8e5942..1ef5aebba3 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -67,7 +67,5 @@ void portio_list_init(PortioList *piolist, Object *owner,
> void *opaque, const char *name,
> MemoryRegion *address_space_io, uint16_t start);
> void portio_list_set_flush_coalesced(PortioList *piolist);
> -void portio_list_destroy(PortioList *piolist);
> -void portio_list_del(PortioList *piolist);
>
> #endif /* IOPORT_H */
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index c92e3cb27d..0a55d39196 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -118,19 +118,6 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
> piolist->flush_coalesced_mmio = true;
> }
>
> -void portio_list_destroy(PortioList *piolist)
> -{
> - MemoryRegionPortioList *mrpio;
> - unsigned i;
> -
> - for (i = 0; i < piolist->nr; ++i) {
> - mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
> - object_unparent(OBJECT(&mrpio->mr));
> - g_free(mrpio);
> - }
> - g_free(piolist->regions);
> -}
> -
> static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
> uint64_t offset, unsigned size,
> bool write)
> @@ -280,14 +267,3 @@ void portio_list_init(PortioList *piolist, Object *owner,
> /* There will always be an open sub-list. */
> portio_list_add_1(piolist, pio_start, count, start, off_low, off_high);
> }
> -
> -void portio_list_del(PortioList *piolist)
> -{
> - MemoryRegionPortioList *mrpio;
> - unsigned i;
> -
> - for (i = 0; i < piolist->nr; ++i) {
> - mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
> - memory_region_del_subregion(piolist->address_space, &mrpio->mr);
> - }
> -}
I think it may be worth leaving these functions. There were previous discussions
around the cmd646 and via PCI-IDE interfaces which have a bit in PCI configuration
space that switches the chip between compatibility (ISA) mode and PCI mode. I could
see that switching the device to PCI mode would require removal of the old ISA ports,
for example, as in PCI mode the registers would be accessed exclusively via the PCI BAR.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 03/10] softmmu/ioport: Remove unused functions
2023-02-05 21:37 ` Mark Cave-Ayland
@ 2023-02-06 0:20 ` Bernhard Beschow
0 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-06 0:20 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
Am 5. Februar 2023 21:37:01 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 26/01/2023 21:17, Bernhard Beschow wrote:
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> include/exec/ioport.h | 2 --
>> softmmu/ioport.c | 24 ------------------------
>> 2 files changed, 26 deletions(-)
>>
>> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
>> index ec3e8e5942..1ef5aebba3 100644
>> --- a/include/exec/ioport.h
>> +++ b/include/exec/ioport.h
>> @@ -67,7 +67,5 @@ void portio_list_init(PortioList *piolist, Object *owner,
>> void *opaque, const char *name,
>> MemoryRegion *address_space_io, uint16_t start);
>> void portio_list_set_flush_coalesced(PortioList *piolist);
>> -void portio_list_destroy(PortioList *piolist);
>> -void portio_list_del(PortioList *piolist);
>> #endif /* IOPORT_H */
>> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
>> index c92e3cb27d..0a55d39196 100644
>> --- a/softmmu/ioport.c
>> +++ b/softmmu/ioport.c
>> @@ -118,19 +118,6 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
>> piolist->flush_coalesced_mmio = true;
>> }
>> -void portio_list_destroy(PortioList *piolist)
>> -{
>> - MemoryRegionPortioList *mrpio;
>> - unsigned i;
>> -
>> - for (i = 0; i < piolist->nr; ++i) {
>> - mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>> - object_unparent(OBJECT(&mrpio->mr));
>> - g_free(mrpio);
>> - }
>> - g_free(piolist->regions);
>> -}
>> -
>> static const MemoryRegionPortio *find_portio(MemoryRegionPortioList *mrpio,
>> uint64_t offset, unsigned size,
>> bool write)
>> @@ -280,14 +267,3 @@ void portio_list_init(PortioList *piolist, Object *owner,
>> /* There will always be an open sub-list. */
>> portio_list_add_1(piolist, pio_start, count, start, off_low, off_high);
>> }
>> -
>> -void portio_list_del(PortioList *piolist)
>> -{
>> - MemoryRegionPortioList *mrpio;
>> - unsigned i;
>> -
>> - for (i = 0; i < piolist->nr; ++i) {
>> - mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>> - memory_region_del_subregion(piolist->address_space, &mrpio->mr);
>> - }
>> -}
>
>I think it may be worth leaving these functions. There were previous discussions around the cmd646 and via PCI-IDE interfaces which have a bit in PCI configuration space that switches the chip between compatibility (ISA) mode and PCI mode. I could see that switching the device to PCI mode would require removal of the old ISA ports, for example, as in PCI mode the registers would be accessed exclusively via the PCI BAR.
Sure, I can skip this patch.
BR,
Bernhard
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq()
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (2 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 03/10] softmmu/ioport: Remove unused functions Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-02-05 21:54 ` Mark Cave-Ayland
2023-01-26 21:17 ` [PATCH v2 05/10] Revert "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine" Bernhard Beschow
` (7 subsequent siblings)
11 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
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 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 | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 267dbf37db..a6646d9657 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
}
-static int pci_piix_init_ports(PCIIDEState *d)
+static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
{
static const struct {
int iobase;
@@ -145,7 +145,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
if (ret) {
return ret;
}
- ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+ ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
@@ -159,6 +159,8 @@ 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;
int rc;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -168,7 +170,20 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
- rc = pci_piix_init_ports(d);
+ 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(dev)));
+ return;
+ }
+ if (!isa_bus) {
+ error_setg(errp, "No ISA bus found while %s requires one",
+ object_get_typename(OBJECT(dev)));
+ return;
+ }
+
+ rc = pci_piix_init_ports(d, isa_bus);
if (rc) {
error_setg_errno(errp, -rc, "Failed to realize %s",
object_get_typename(OBJECT(dev)));
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq()
2023-01-26 21:17 ` [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq() Bernhard Beschow
@ 2023-02-05 21:54 ` Mark Cave-Ayland
0 siblings, 0 replies; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 21:54 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On 26/01/2023 21:17, 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 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 | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 267dbf37db..a6646d9657 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
> pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
> }
>
> -static int pci_piix_init_ports(PCIIDEState *d)
> +static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
> {
> static const struct {
> int iobase;
> @@ -145,7 +145,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
> if (ret) {
> return ret;
> }
> - ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> + ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
> d->bmdma[i].bus = &d->bus[i];
> @@ -159,6 +159,8 @@ 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;
> int rc;
>
> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
> @@ -168,7 +170,20 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>
> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>
> - rc = pci_piix_init_ports(d);
> + 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(dev)));
> + return;
> + }
> + if (!isa_bus) {
> + error_setg(errp, "No ISA bus found while %s requires one",
> + object_get_typename(OBJECT(dev)));
> + return;
> + }
> +
> + rc = pci_piix_init_ports(d, isa_bus);
> if (rc) {
> error_setg_errno(errp, -rc, "Failed to realize %s",
> object_get_typename(OBJECT(dev)));
I think the approach here to allow the PCI-ISA bridge to locate the ISABus is a good
one, but I think it's worth keeping isa_get_irq() to avoid exposing the internals to
devices.
For me the problem here is that isa_get_irq() accepts a NULL argument for ISADevice.
I'd expect the function to look something like isa_bus_get_irq(ISABus *isa_bus,
unsigned isairq) and then it is the responsibility of the caller to locate and
specify the correct ISABus to avoid falling back to the isabus global.
In particular I can see a PCIDevice should be able to attempt a pci_get_isa_bus() or
similar which would locate the PCI-ISA bridge and return the child ISA bus, which
again is related to the cmd646/via compatibility mode feature.
This is along the lines of the similar approach I discussed in
https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg01443.html which is an
evolution of the ideas discussed in the original thread a couple of years earlier at
https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 05/10] Revert "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine"
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (3 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq() Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[] Bernhard Beschow
` (6 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
Now that the PIIX IDE device models check for presence of an ISABus before
using it, this fix isn't needed any longer.
This reverts commit 9405d87be25db6dff4d7b5ab48a81bbf6d083e47.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/ide/internal.h | 2 +-
include/hw/isa/isa.h | 13 +++++--------
hw/ide/ioport.c | 16 ++++++----------
hw/ide/piix.c | 20 +++++---------------
hw/isa/isa-bus.c | 14 ++++----------
5 files changed, 21 insertions(+), 44 deletions(-)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index fc0aa81a88..42c49414f4 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -628,7 +628,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
int chs_trans, Error **errp);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_exit(IDEState *s);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
void ide_register_restart_cb(IDEBus *bus);
void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 6c8a8a92cb..8dd2953211 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -114,15 +114,12 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
* @portio: the ports, sorted by offset.
* @opaque: passed into the portio callbacks.
* @name: passed into memory_region_init_io.
- *
- * Returns: 0 on success, negative error code otherwise (e.g. if the
- * ISA bus is not available)
*/
-int isa_register_portio_list(ISADevice *dev,
- PortioList *piolist,
- uint16_t start,
- const MemoryRegionPortio *portio,
- void *opaque, const char *name);
+void isa_register_portio_list(ISADevice *dev,
+ PortioList *piolist,
+ uint16_t start,
+ const MemoryRegionPortio *portio,
+ void *opaque, const char *name);
static inline ISABus *isa_bus_from_device(ISADevice *d)
{
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e6caa537fa..b613ff3bba 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,19 +50,15 @@ static const MemoryRegionPortio ide_portio2_list[] = {
PORTIO_END_OF_LIST(),
};
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
{
- int ret;
-
/* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
bridge has been setup properly to always register with ISA. */
- ret = isa_register_portio_list(dev, &bus->portio_list,
- iobase, ide_portio_list, bus, "ide");
+ isa_register_portio_list(dev, &bus->portio_list,
+ iobase, ide_portio_list, bus, "ide");
- if (ret == 0 && iobase2) {
- ret = isa_register_portio_list(dev, &bus->portio2_list,
- iobase2, ide_portio2_list, bus, "ide");
+ if (iobase2) {
+ isa_register_portio_list(dev, &bus->portio2_list,
+ iobase2, ide_portio2_list, bus, "ide");
}
-
- return ret;
}
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a6646d9657..5980045db0 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
}
-static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
+static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
{
static const struct {
int iobase;
@@ -136,23 +136,18 @@ static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
{0x1f0, 0x3f6, 14},
{0x170, 0x376, 15},
};
- int i, ret;
+ int i;
for (i = 0; i < 2; i++) {
ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
- ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
- port_info[i].iobase2);
- if (ret) {
- return ret;
- }
+ ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+ port_info[i].iobase2);
ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
ide_register_restart_cb(&d->bus[i]);
}
-
- return 0;
}
static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
@@ -161,7 +156,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
uint8_t *pci_conf = dev->config;
ISABus *isa_bus;
bool ambiguous;
- int rc;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -183,11 +177,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
return;
}
- rc = pci_piix_init_ports(d, isa_bus);
- if (rc) {
- error_setg_errno(errp, -rc, "Failed to realize %s",
- object_get_typename(OBJECT(dev)));
- }
+ pci_piix_init_ports(d, isa_bus);
}
static void pci_piix_ide_exitfn(PCIDevice *dev)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index b3497dad61..d3e2d9de35 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -119,15 +119,11 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
isa_init_ioport(dev, start);
}
-int isa_register_portio_list(ISADevice *dev,
- PortioList *piolist, uint16_t start,
- const MemoryRegionPortio *pio_start,
- void *opaque, const char *name)
+void isa_register_portio_list(ISADevice *dev,
+ PortioList *piolist, uint16_t start,
+ const MemoryRegionPortio *pio_start,
+ void *opaque, const char *name)
{
- if (!isabus) {
- return -ENODEV;
- }
-
/* START is how we should treat DEV, regardless of the actual
contents of the portio array. This is how the old code
actually handled e.g. the FDC device. */
@@ -135,8 +131,6 @@ int isa_register_portio_list(ISADevice *dev,
portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name,
isabus->address_space_io, start);
-
- return 0;
}
ISADevice *isa_new(const char *name)
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[]
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (4 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 05/10] Revert "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine" Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-30 17:00 ` Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances Bernhard Beschow
` (5 subsequent siblings)
11 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
These legacy ISA IRQs allow the PIIX IDE functions to be wired up in
their south bridges and the VIA IDE functions to disuse
PCI_INTERRUPT_LINE as outlined in https://lists.nongnu.org/archive/html/
qemu-devel/2020-03/msg01707.html .
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/ide/pci.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 2a6284acac..24c0b7a2dd 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -49,6 +49,7 @@ struct PCIIDEState {
IDEBus bus[2];
BMDMAState bmdma[2];
+ qemu_irq isa_irqs[2];
uint32_t secondary; /* used only for cmd646 */
MemoryRegion bmdma_bar;
MemoryRegion cmd_bar[2];
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[]
2023-01-26 21:17 ` [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[] Bernhard Beschow
@ 2023-01-30 17:00 ` Bernhard Beschow
2023-02-26 21:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-30 17:00 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Mark Cave-Ayland
Am 26. Januar 2023 21:17:36 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>These legacy ISA IRQs allow the PIIX IDE functions to be wired up in
>their south bridges and the VIA IDE functions to disuse
>PCI_INTERRUPT_LINE as outlined in https://lists.nongnu.org/archive/html/
>qemu-devel/2020-03/msg01707.html .
>
Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>---
> include/hw/ide/pci.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>index 2a6284acac..24c0b7a2dd 100644
>--- a/include/hw/ide/pci.h
>+++ b/include/hw/ide/pci.h
>@@ -49,6 +49,7 @@ struct PCIIDEState {
>
> IDEBus bus[2];
> BMDMAState bmdma[2];
>+ qemu_irq isa_irqs[2];
> uint32_t secondary; /* used only for cmd646 */
> MemoryRegion bmdma_bar;
> MemoryRegion cmd_bar[2];
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[]
2023-01-30 17:00 ` Bernhard Beschow
@ 2023-02-26 21:26 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-26 21:26 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini, Mark Cave-Ayland
On 30/1/23 18:00, Bernhard Beschow wrote:
>
>
> Am 26. Januar 2023 21:17:36 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> These legacy ISA IRQs allow the PIIX IDE functions to be wired up in
>> their south bridges and the VIA IDE functions to disuse
>> PCI_INTERRUPT_LINE as outlined in https://lists.nongnu.org/archive/html/
>> qemu-devel/2020-03/msg01707.html .
>>
>
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> include/hw/ide/pci.h | 1 +
>> 1 file changed, 1 insertion(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (5 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 06/10] hw/ide/pci: Add PCIIDEState::isa_irqs[] Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-02-05 21:58 ` Mark Cave-Ayland
2023-02-24 16:20 ` Michael S. Tsirkin
2023-01-26 21:17 ` [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice Bernhard Beschow
` (4 subsequent siblings)
11 siblings, 2 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
Internal instances now defer interrupt wiring to the caller which
decouples them from the ISABus. User-created devices still fish out the
ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
The latter mechanism is considered a workaround and intended to be
removed once a deprecation period for user-created PIIX IDE devices is
over.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/ide/pci.h | 1 +
hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
hw/isa/piix.c | 5 ++++
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 24c0b7a2dd..ee2c8781b7 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -54,6 +54,7 @@ struct PCIIDEState {
MemoryRegion bmdma_bar;
MemoryRegion cmd_bar[2];
MemoryRegion data_bar[2];
+ bool user_created;
};
static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 5980045db0..f0d95761ac 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
}
}
+static void piix_ide_set_irq(void *opaque, int n, int level)
+{
+ PCIIDEState *d = opaque;
+
+ qemu_set_irq(d->isa_irqs[n], level);
+}
+
static void piix_ide_reset(DeviceState *dev)
{
PCIIDEState *d = PCI_IDE(dev);
@@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
};
int i;
+ if (isa_bus) {
+ d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
+ d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
+ } else {
+ qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
+ }
+
for (i = 0; i < 2; i++) {
ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
port_info[i].iobase2);
- ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
+ ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
@@ -154,8 +168,7 @@ 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;
+ ISABus *isa_bus = NULL;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
- 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(dev)));
- return;
- }
- if (!isa_bus) {
- error_setg(errp, "No ISA bus found while %s requires one",
- object_get_typename(OBJECT(dev)));
- return;
+ if (d->user_created) {
+ bool ambiguous;
+
+ 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(dev)));
+ return;
+ }
+
+ if (!isa_bus) {
+ error_setg(errp, "No ISA bus found while %s requires one",
+ object_get_typename(OBJECT(dev)));
+ return;
+ }
}
pci_piix_init_ports(d, isa_bus);
}
+static void pci_piix_ide_init(Object *obj)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
+}
+
static void pci_piix_ide_exitfn(PCIDevice *dev)
{
PCIIDEState *d = PCI_IDE(dev);
@@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
}
}
+static Property piix_ide_properties[] = {
+ DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
+ 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)
{
@@ -205,11 +237,13 @@ 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 = {
.name = TYPE_PIIX3_IDE,
.parent = TYPE_PCI_IDE,
+ .instance_init = pci_piix_ide_init,
.class_init = piix3_ide_class_init,
};
@@ -227,11 +261,13 @@ 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 = {
.name = TYPE_PIIX4_IDE,
.parent = TYPE_PCI_IDE,
+ .instance_init = pci_piix_ide_init,
.class_init = piix4_ide_class_init,
};
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 54a1246a9d..f9974c2a77 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
/* IDE */
qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
+ qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
return;
}
+ qdev_connect_gpio_out(DEVICE(&d->ide), 0,
+ qdev_get_gpio_in(DEVICE(&d->pic), 14));
+ qdev_connect_gpio_out(DEVICE(&d->ide), 1,
+ qdev_get_gpio_in(DEVICE(&d->pic), 15));
/* USB */
if (d->has_usb) {
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-01-26 21:17 ` [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances Bernhard Beschow
@ 2023-02-05 21:58 ` Mark Cave-Ayland
2023-02-05 22:21 ` BALATON Zoltan
2023-02-24 16:20 ` Michael S. Tsirkin
1 sibling, 1 reply; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 21:58 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On 26/01/2023 21:17, Bernhard Beschow wrote:
> Internal instances now defer interrupt wiring to the caller which
> decouples them from the ISABus. User-created devices still fish out the
> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
> The latter mechanism is considered a workaround and intended to be
> removed once a deprecation period for user-created PIIX IDE devices is
> over.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/ide/pci.h | 1 +
> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
> hw/isa/piix.c | 5 ++++
> 3 files changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index 24c0b7a2dd..ee2c8781b7 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -54,6 +54,7 @@ struct PCIIDEState {
> MemoryRegion bmdma_bar;
> MemoryRegion cmd_bar[2];
> MemoryRegion data_bar[2];
> + bool user_created;
> };
>
> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 5980045db0..f0d95761ac 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
> }
> }
>
> +static void piix_ide_set_irq(void *opaque, int n, int level)
> +{
> + PCIIDEState *d = opaque;
> +
> + qemu_set_irq(d->isa_irqs[n], level);
> +}
> +
> static void piix_ide_reset(DeviceState *dev)
> {
> PCIIDEState *d = PCI_IDE(dev);
> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
> };
> int i;
>
> + if (isa_bus) {
> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
> + } else {
> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
> + }
> +
> for (i = 0; i < 2; i++) {
> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> port_info[i].iobase2);
> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
> d->bmdma[i].bus = &d->bus[i];
> @@ -154,8 +168,7 @@ 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;
> + ISABus *isa_bus = NULL;
>
> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>
> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>
> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>
> - 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(dev)));
> - return;
> - }
> - if (!isa_bus) {
> - error_setg(errp, "No ISA bus found while %s requires one",
> - object_get_typename(OBJECT(dev)));
> - return;
> + if (d->user_created) {
> + bool ambiguous;
> +
> + 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(dev)));
> + return;
> + }
> +
> + if (!isa_bus) {
> + error_setg(errp, "No ISA bus found while %s requires one",
> + object_get_typename(OBJECT(dev)));
> + return;
> + }
> }
>
> pci_piix_init_ports(d, isa_bus);
> }
>
> +static void pci_piix_ide_init(Object *obj)
> +{
> + DeviceState *dev = DEVICE(obj);
> +
> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
> +}
> +
> static void pci_piix_ide_exitfn(PCIDevice *dev)
> {
> PCIIDEState *d = PCI_IDE(dev);
> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
> }
> }
>
> +static Property piix_ide_properties[] = {
> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
> + 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)
> {
> @@ -205,11 +237,13 @@ 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 = {
> .name = TYPE_PIIX3_IDE,
> .parent = TYPE_PCI_IDE,
> + .instance_init = pci_piix_ide_init,
> .class_init = piix3_ide_class_init,
> };
>
> @@ -227,11 +261,13 @@ 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 = {
> .name = TYPE_PIIX4_IDE,
> .parent = TYPE_PCI_IDE,
> + .instance_init = pci_piix_ide_init,
> .class_init = piix4_ide_class_init,
> };
>
> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
> index 54a1246a9d..f9974c2a77 100644
> --- a/hw/isa/piix.c
> +++ b/hw/isa/piix.c
> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>
> /* IDE */
> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
> return;
> }
> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>
> /* USB */
> if (d->has_usb) {
I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via
PCI-IDE interfaces in that there will be a PCI configuration register that will
switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So
it would be the device configuration that would specify PCI or ISA mode, rather than
the presence of an ISABus.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-05 21:58 ` Mark Cave-Ayland
@ 2023-02-05 22:21 ` BALATON Zoltan
2023-02-05 22:32 ` Mark Cave-Ayland
0 siblings, 1 reply; 40+ messages in thread
From: BALATON Zoltan @ 2023-02-05 22:21 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Bernhard Beschow, qemu-devel, Marcel Apfelbaum,
Hervé Poussineau, Gerd Hoffmann, Michael S. Tsirkin,
Aurelien Jarno, Philippe Mathieu-Daudé, David Hildenbrand,
Peter Xu, qemu-ppc, qemu-block, John Snow, Paolo Bonzini
On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
> On 26/01/2023 21:17, Bernhard Beschow wrote:
>> Internal instances now defer interrupt wiring to the caller which
>> decouples them from the ISABus. User-created devices still fish out the
>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>> The latter mechanism is considered a workaround and intended to be
>> removed once a deprecation period for user-created PIIX IDE devices is
>> over.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> include/hw/ide/pci.h | 1 +
>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>> hw/isa/piix.c | 5 ++++
>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 24c0b7a2dd..ee2c8781b7 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>> MemoryRegion bmdma_bar;
>> MemoryRegion cmd_bar[2];
>> MemoryRegion data_bar[2];
>> + bool user_created;
>> };
>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 5980045db0..f0d95761ac 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>> }
>> }
>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>> +{
>> + PCIIDEState *d = opaque;
>> +
>> + qemu_set_irq(d->isa_irqs[n], level);
>> +}
>> +
>> static void piix_ide_reset(DeviceState *dev)
>> {
>> PCIIDEState *d = PCI_IDE(dev);
>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d,
>> ISABus *isa_bus)
>> };
>> int i;
>> + if (isa_bus) {
>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>> + } else {
>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>> + }
>> +
>> for (i = 0; i < 2; i++) {
>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>> port_info[i].iobase2);
>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>> d->bmdma[i].bus = &d->bus[i];
>> @@ -154,8 +168,7 @@ 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;
>> + ISABus *isa_bus = NULL;
>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev,
>> Error **errp)
>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>> - 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(dev)));
>> - return;
>> - }
>> - if (!isa_bus) {
>> - error_setg(errp, "No ISA bus found while %s requires one",
>> - object_get_typename(OBJECT(dev)));
>> - return;
>> + if (d->user_created) {
>> + bool ambiguous;
>> +
>> + 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(dev)));
>> + return;
>> + }
>> +
>> + if (!isa_bus) {
>> + error_setg(errp, "No ISA bus found while %s requires one",
>> + object_get_typename(OBJECT(dev)));
>> + return;
>> + }
>> }
>> pci_piix_init_ports(d, isa_bus);
>> }
>> +static void pci_piix_ide_init(Object *obj)
>> +{
>> + DeviceState *dev = DEVICE(obj);
>> +
>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>> +}
>> +
>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>> {
>> PCIIDEState *d = PCI_IDE(dev);
>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>> }
>> }
>> +static Property piix_ide_properties[] = {
>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>> + 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)
>> {
>> @@ -205,11 +237,13 @@ 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 = {
>> .name = TYPE_PIIX3_IDE,
>> .parent = TYPE_PCI_IDE,
>> + .instance_init = pci_piix_ide_init,
>> .class_init = piix3_ide_class_init,
>> };
>> @@ -227,11 +261,13 @@ 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 = {
>> .name = TYPE_PIIX4_IDE,
>> .parent = TYPE_PCI_IDE,
>> + .instance_init = pci_piix_ide_init,
>> .class_init = piix4_ide_class_init,
>> };
>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>> index 54a1246a9d..f9974c2a77 100644
>> --- a/hw/isa/piix.c
>> +++ b/hw/isa/piix.c
>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const
>> char *uhci_type,
>> /* IDE */
>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>> return;
>> }
>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>> /* USB */
>> if (d->has_usb) {
>
> I haven't checked the datasheet, but I suspect this will be similar to the
> cmd646/via PCI-IDE interfaces in that there will be a PCI configuration
> register that will switch between ISA compatibility mode (and ISA irqs) and
> PCI mode (with PCI IRQs). So it would be the device configuration that would
> specify PCI or ISA mode, rather than the presence of an ISABus.
I forgot about this topic already and haven't follwed this series either
so what I say may not fully make sense but I think CMD646 and via-ide are
different. CMD646 is a PCI device and should use PCI interrupts while
via-ide is part of a southbridge/superio complex and connected to the ISA
PICs within that southbride, so I think via-ide always uses ISA IRQs and
the ISA btidge within the same chip may convert that to PCI IRQs or not
(that part is where I'm lost also because we may not actually model it
that way). After a long debate we managed to find a solution back then
that works for every guest we use it for now so I think we don't want to
touch it now until some real need arises. It does not worth the trouble
and added complexity to model something that is not used just for the sake
of correctness. By the time we find a use for that, the ISA emulation may
evolve so it's easier to implement the missing switching between isa and
native mode or we may want to do it differently (such as we do things
differently now compared to what we did years ago). So I think it does not
worth keeping the ISA model from being simplified for some theoretical
uses in the future which we may not actually do any time soon. But I don't
want to get into this again so just shared my thoughts and feel free to
ignore it. I don't care where these patches go as long as the VIA model
keeps working for me.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-05 22:21 ` BALATON Zoltan
@ 2023-02-05 22:32 ` Mark Cave-Ayland
2023-02-06 6:51 ` Philippe Mathieu-Daudé
2023-02-06 23:40 ` Bernhard Beschow
0 siblings, 2 replies; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 22:32 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Bernhard Beschow, qemu-devel, Marcel Apfelbaum,
Hervé Poussineau, Gerd Hoffmann, Michael S. Tsirkin,
Aurelien Jarno, Philippe Mathieu-Daudé, David Hildenbrand,
Peter Xu, qemu-ppc, qemu-block, John Snow, Paolo Bonzini
On 05/02/2023 22:21, BALATON Zoltan wrote:
> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>> Internal instances now defer interrupt wiring to the caller which
>>> decouples them from the ISABus. User-created devices still fish out the
>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>> The latter mechanism is considered a workaround and intended to be
>>> removed once a deprecation period for user-created PIIX IDE devices is
>>> over.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> include/hw/ide/pci.h | 1 +
>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>> hw/isa/piix.c | 5 ++++
>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>> index 24c0b7a2dd..ee2c8781b7 100644
>>> --- a/include/hw/ide/pci.h
>>> +++ b/include/hw/ide/pci.h
>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>> MemoryRegion bmdma_bar;
>>> MemoryRegion cmd_bar[2];
>>> MemoryRegion data_bar[2];
>>> + bool user_created;
>>> };
>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>> index 5980045db0..f0d95761ac 100644
>>> --- a/hw/ide/piix.c
>>> +++ b/hw/ide/piix.c
>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>> }
>>> }
>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>> +{
>>> + PCIIDEState *d = opaque;
>>> +
>>> + qemu_set_irq(d->isa_irqs[n], level);
>>> +}
>>> +
>>> static void piix_ide_reset(DeviceState *dev)
>>> {
>>> PCIIDEState *d = PCI_IDE(dev);
>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus
>>> *isa_bus)
>>> };
>>> int i;
>>> + if (isa_bus) {
>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>> + } else {
>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>> + }
>>> +
>>> for (i = 0; i < 2; i++) {
>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>> port_info[i].iobase2);
>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>> d->bmdma[i].bus = &d->bus[i];
>>> @@ -154,8 +168,7 @@ 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;
>>> + ISABus *isa_bus = NULL;
>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
>>> **errp)
>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>> - 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(dev)));
>>> - return;
>>> - }
>>> - if (!isa_bus) {
>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>> - object_get_typename(OBJECT(dev)));
>>> - return;
>>> + if (d->user_created) {
>>> + bool ambiguous;
>>> +
>>> + 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(dev)));
>>> + return;
>>> + }
>>> +
>>> + if (!isa_bus) {
>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>> + object_get_typename(OBJECT(dev)));
>>> + return;
>>> + }
>>> }
>>> pci_piix_init_ports(d, isa_bus);
>>> }
>>> +static void pci_piix_ide_init(Object *obj)
>>> +{
>>> + DeviceState *dev = DEVICE(obj);
>>> +
>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>> +}
>>> +
>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>> {
>>> PCIIDEState *d = PCI_IDE(dev);
>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>> }
>>> }
>>> +static Property piix_ide_properties[] = {
>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>> + 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)
>>> {
>>> @@ -205,11 +237,13 @@ 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 = {
>>> .name = TYPE_PIIX3_IDE,
>>> .parent = TYPE_PCI_IDE,
>>> + .instance_init = pci_piix_ide_init,
>>> .class_init = piix3_ide_class_init,
>>> };
>>> @@ -227,11 +261,13 @@ 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 = {
>>> .name = TYPE_PIIX4_IDE,
>>> .parent = TYPE_PCI_IDE,
>>> + .instance_init = pci_piix_ide_init,
>>> .class_init = piix4_ide_class_init,
>>> };
>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>> index 54a1246a9d..f9974c2a77 100644
>>> --- a/hw/isa/piix.c
>>> +++ b/hw/isa/piix.c
>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char
>>> *uhci_type,
>>> /* IDE */
>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>> return;
>>> }
>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>> /* USB */
>>> if (d->has_usb) {
>>
>> I haven't checked the datasheet, but I suspect this will be similar to the
>> cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register
>> that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with
>> PCI IRQs). So it would be the device configuration that would specify PCI or ISA
>> mode, rather than the presence of an ISABus.
>
> I forgot about this topic already and haven't follwed this series either so what I
> say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is
> a PCI device and should use PCI interrupts while via-ide is part of a
> southbridge/superio complex and connected to the ISA PICs within that southbride, so
> I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may
> convert that to PCI IRQs or not (that part is where I'm lost also because we may not
> actually model it that way). After a long debate we managed to find a solution back
> then that works for every guest we use it for now so I think we don't want to touch
> it now until some real need arises. It does not worth the trouble and added
> complexity to model something that is not used just for the sake of correctness. By
> the time we find a use for that, the ISA emulation may evolve so it's easier to
> implement the missing switching between isa and native mode or we may want to do it
> differently (such as we do things differently now compared to what we did years ago).
> So I think it does not worth keeping the ISA model from being simplified for some
> theoretical uses in the future which we may not actually do any time soon. But I
> don't want to get into this again so just shared my thoughts and feel free to ignore
> it. I don't care where these patches go as long as the VIA model keeps working for me.
I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA
specification, but it has been a while since I last looked.
Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the
cmd646 datasheet specifies that ISA mode or PCI mode is determined by register
PROG_IF (0x9) in PCI configuration space.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-05 22:32 ` Mark Cave-Ayland
@ 2023-02-06 6:51 ` Philippe Mathieu-Daudé
2023-02-06 9:15 ` Mark Cave-Ayland
2023-02-06 23:40 ` Bernhard Beschow
1 sibling, 1 reply; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-06 6:51 UTC (permalink / raw)
To: Bernhard Beschow, Mark Cave-Ayland, John Snow, BALATON Zoltan,
Eduardo Habkost
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, Paolo Bonzini,
Thomas Huth, Peter Maydell, Alistair Francis, Edgar E. Iglesias
On 5/2/23 23:32, Mark Cave-Ayland wrote:
> On 05/02/2023 22:21, BALATON Zoltan wrote:
>
>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>> Internal instances now defer interrupt wiring to the caller which
>>>> decouples them from the ISABus. User-created devices still fish out the
>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>> The latter mechanism is considered a workaround and intended to be
>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>> over.
>>>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> include/hw/ide/pci.h | 1 +
>>>> hw/ide/piix.c | 64
>>>> ++++++++++++++++++++++++++++++++++----------
>>>> hw/isa/piix.c | 5 ++++
>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>> I haven't checked the datasheet, but I suspect this will be similar
>>> to the cmd646/via PCI-IDE interfaces in that there will be a PCI
>>> configuration register that will switch between ISA compatibility
>>> mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the
>>> device configuration that would specify PCI or ISA mode, rather than
>>> the presence of an ISABus.
>>
>> I forgot about this topic already and haven't follwed this series
>> either so what I say may not fully make sense but I think CMD646 and
>> via-ide are different. CMD646 is a PCI device and should use PCI
>> interrupts while via-ide is part of a southbridge/superio complex and
>> connected to the ISA PICs within that southbride, so I think via-ide
>> always uses ISA IRQs and the ISA btidge within the same chip may
>> convert that to PCI IRQs or not (that part is where I'm lost also
>> because we may not actually model it that way). After a long debate we
>> managed to find a solution back then that works for every guest we use
>> it for now so I think we don't want to touch it now until some real
>> need arises. It does not worth the trouble and added complexity to
>> model something that is not used just for the sake of correctness. By
>> the time we find a use for that, the ISA emulation may evolve so it's
>> easier to implement the missing switching between isa and native mode
>> or we may want to do it differently (such as we do things differently
>> now compared to what we did years ago). So I think it does not worth
>> keeping the ISA model from being simplified for some theoretical uses
>> in the future which we may not actually do any time soon. But I don't
>> want to get into this again so just shared my thoughts and feel free
>> to ignore it. I don't care where these patches go as long as the VIA
>> model keeps working for me.
>
> I have a vague memory that ISA compatibility mode was part of the
> original PCI-BMDMA specification, but it has been a while since I last
> looked.
>
> Bernhard, is there any mention of this in the PIIX datasheet(s)? For
> reference the cmd646 datasheet specifies that ISA mode or PCI mode is
> determined by register PROG_IF (0x9) in PCI configuration space.
Orthogonal to this discussion, one problem I see here is a device
exposing 2 interfaces: ISA and PCI. QOM does support interfaces,
but ISA and PCI aren't QOM interfaces. The QOM cast macros are
written as a QOM object can only inherit one parent. Should we
stick to QDev and convert ISA/PCI as QOM interfaces? That could
solve some QDev IDE limitations...
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-06 6:51 ` Philippe Mathieu-Daudé
@ 2023-02-06 9:15 ` Mark Cave-Ayland
0 siblings, 0 replies; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-06 9:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Bernhard Beschow, John Snow,
BALATON Zoltan, Eduardo Habkost
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, Paolo Bonzini,
Thomas Huth, Peter Maydell, Alistair Francis, Edgar E. Iglesias
On 06/02/2023 06:51, Philippe Mathieu-Daudé wrote:
> On 5/2/23 23:32, Mark Cave-Ayland wrote:
>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>
>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>> The latter mechanism is considered a workaround and intended to be
>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>> over.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> include/hw/ide/pci.h | 1 +
>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>> hw/isa/piix.c | 5 ++++
>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>
>>>> I haven't checked the datasheet, but I suspect this will be similar to the
>>>> cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register
>>>> that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with
>>>> PCI IRQs). So it would be the device configuration that would specify PCI or ISA
>>>> mode, rather than the presence of an ISABus.
>>>
>>> I forgot about this topic already and haven't follwed this series either so what I
>>> say may not fully make sense but I think CMD646 and via-ide are different. CMD646
>>> is a PCI device and should use PCI interrupts while via-ide is part of a
>>> southbridge/superio complex and connected to the ISA PICs within that southbride,
>>> so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip
>>> may convert that to PCI IRQs or not (that part is where I'm lost also because we
>>> may not actually model it that way). After a long debate we managed to find a
>>> solution back then that works for every guest we use it for now so I think we
>>> don't want to touch it now until some real need arises. It does not worth the
>>> trouble and added complexity to model something that is not used just for the sake
>>> of correctness. By the time we find a use for that, the ISA emulation may evolve
>>> so it's easier to implement the missing switching between isa and native mode or
>>> we may want to do it differently (such as we do things differently now compared to
>>> what we did years ago). So I think it does not worth keeping the ISA model from
>>> being simplified for some theoretical uses in the future which we may not actually
>>> do any time soon. But I don't want to get into this again so just shared my
>>> thoughts and feel free to ignore it. I don't care where these patches go as long
>>> as the VIA model keeps working for me.
>>
>> I have a vague memory that ISA compatibility mode was part of the original
>> PCI-BMDMA specification, but it has been a while since I last looked.
>>
>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the
>> cmd646 datasheet specifies that ISA mode or PCI mode is determined by register
>> PROG_IF (0x9) in PCI configuration space.
>
> Orthogonal to this discussion, one problem I see here is a device
> exposing 2 interfaces: ISA and PCI. QOM does support interfaces,
> but ISA and PCI aren't QOM interfaces. The QOM cast macros are
> written as a QOM object can only inherit one parent. Should we
> stick to QDev and convert ISA/PCI as QOM interfaces? That could
> solve some QDev IDE limitations...
The normal approach to this is to encapsulate the chip functionality as a set of
common functions, for example see esp.c and then have separate files such as
esp-pci.c and esp-isa.c which can be compiled accordingly using Kconfig dependencies.
FWIW the ability to support a legacy mode is only something that would be present in
the generation of mixed PCI/ISA motherboards, and so probably not something that is
worth the effort of reworking separate PCI and ISA QOM interfaces.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-05 22:32 ` Mark Cave-Ayland
2023-02-06 6:51 ` Philippe Mathieu-Daudé
@ 2023-02-06 23:40 ` Bernhard Beschow
2023-02-07 9:03 ` Philippe Mathieu-Daudé
2023-02-07 20:52 ` Mark Cave-Ayland
1 sibling, 2 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-06 23:40 UTC (permalink / raw)
To: Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 05/02/2023 22:21, BALATON Zoltan wrote:
>
>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>> Internal instances now defer interrupt wiring to the caller which
>>>> decouples them from the ISABus. User-created devices still fish out the
>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>> The latter mechanism is considered a workaround and intended to be
>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>> over.
>>>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> include/hw/ide/pci.h | 1 +
>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>> hw/isa/piix.c | 5 ++++
>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>> --- a/include/hw/ide/pci.h
>>>> +++ b/include/hw/ide/pci.h
>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>> MemoryRegion bmdma_bar;
>>>> MemoryRegion cmd_bar[2];
>>>> MemoryRegion data_bar[2];
>>>> + bool user_created;
>>>> };
>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>> index 5980045db0..f0d95761ac 100644
>>>> --- a/hw/ide/piix.c
>>>> +++ b/hw/ide/piix.c
>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>> }
>>>> }
>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>> +{
>>>> + PCIIDEState *d = opaque;
>>>> +
>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>> +}
>>>> +
>>>> static void piix_ide_reset(DeviceState *dev)
>>>> {
>>>> PCIIDEState *d = PCI_IDE(dev);
>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>> };
>>>> int i;
>>>> + if (isa_bus) {
>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>> + } else {
>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>> + }
>>>> +
>>>> for (i = 0; i < 2; i++) {
>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>> port_info[i].iobase2);
>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>> d->bmdma[i].bus = &d->bus[i];
>>>> @@ -154,8 +168,7 @@ 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;
>>>> + ISABus *isa_bus = NULL;
>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>> - 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(dev)));
>>>> - return;
>>>> - }
>>>> - if (!isa_bus) {
>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>> - object_get_typename(OBJECT(dev)));
>>>> - return;
>>>> + if (d->user_created) {
>>>> + bool ambiguous;
>>>> +
>>>> + 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(dev)));
>>>> + return;
>>>> + }
>>>> +
>>>> + if (!isa_bus) {
>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>> + object_get_typename(OBJECT(dev)));
>>>> + return;
>>>> + }
>>>> }
>>>> pci_piix_init_ports(d, isa_bus);
>>>> }
>>>> +static void pci_piix_ide_init(Object *obj)
>>>> +{
>>>> + DeviceState *dev = DEVICE(obj);
>>>> +
>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>> +}
>>>> +
>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>> {
>>>> PCIIDEState *d = PCI_IDE(dev);
>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>> }
>>>> }
>>>> +static Property piix_ide_properties[] = {
>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>> + 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)
>>>> {
>>>> @@ -205,11 +237,13 @@ 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 = {
>>>> .name = TYPE_PIIX3_IDE,
>>>> .parent = TYPE_PCI_IDE,
>>>> + .instance_init = pci_piix_ide_init,
>>>> .class_init = piix3_ide_class_init,
>>>> };
>>>> @@ -227,11 +261,13 @@ 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 = {
>>>> .name = TYPE_PIIX4_IDE,
>>>> .parent = TYPE_PCI_IDE,
>>>> + .instance_init = pci_piix_ide_init,
>>>> .class_init = piix4_ide_class_init,
>>>> };
>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>> index 54a1246a9d..f9974c2a77 100644
>>>> --- a/hw/isa/piix.c
>>>> +++ b/hw/isa/piix.c
>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>> /* IDE */
>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>> return;
>>>> }
>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>> /* USB */
>>>> if (d->has_usb) {
>>>
>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>
>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>
>I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>
>Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
I've found the following:
"Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
And:
"PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
Does this perhaps mean that piix-ide does indeed have no ISA bus?
Best regards,
Bernhard
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-06 23:40 ` Bernhard Beschow
@ 2023-02-07 9:03 ` Philippe Mathieu-Daudé
2023-02-07 20:52 ` Mark Cave-Ayland
1 sibling, 0 replies; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-07 9:03 UTC (permalink / raw)
To: Bernhard Beschow, Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On 7/2/23 00:40, Bernhard Beschow wrote:
> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>> The latter mechanism is considered a workaround and intended to be
>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>> over.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> include/hw/ide/pci.h | 1 +
>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>> hw/isa/piix.c | 5 ++++
>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>
>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>
>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>
>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>
> I've found the following:
>
> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>
> And:
>
> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>
> Does this perhaps mean that piix-ide does indeed have no ISA bus?
Yes :)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-06 23:40 ` Bernhard Beschow
2023-02-07 9:03 ` Philippe Mathieu-Daudé
@ 2023-02-07 20:52 ` Mark Cave-Ayland
2023-02-08 0:18 ` Bernhard Beschow
2023-02-23 20:46 ` Bernhard Beschow
1 sibling, 2 replies; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-07 20:52 UTC (permalink / raw)
To: Bernhard Beschow, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
On 06/02/2023 23:40, Bernhard Beschow wrote:
> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>
>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>> The latter mechanism is considered a workaround and intended to be
>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>> over.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> include/hw/ide/pci.h | 1 +
>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>> hw/isa/piix.c | 5 ++++
>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>> --- a/include/hw/ide/pci.h
>>>>> +++ b/include/hw/ide/pci.h
>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>> MemoryRegion bmdma_bar;
>>>>> MemoryRegion cmd_bar[2];
>>>>> MemoryRegion data_bar[2];
>>>>> + bool user_created;
>>>>> };
>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>> index 5980045db0..f0d95761ac 100644
>>>>> --- a/hw/ide/piix.c
>>>>> +++ b/hw/ide/piix.c
>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>> }
>>>>> }
>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>> +{
>>>>> + PCIIDEState *d = opaque;
>>>>> +
>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>> +}
>>>>> +
>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>> {
>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>> };
>>>>> int i;
>>>>> + if (isa_bus) {
>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>> + } else {
>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>> + }
>>>>> +
>>>>> for (i = 0; i < 2; i++) {
>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>> port_info[i].iobase2);
>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>> @@ -154,8 +168,7 @@ 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;
>>>>> + ISABus *isa_bus = NULL;
>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>> - 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(dev)));
>>>>> - return;
>>>>> - }
>>>>> - if (!isa_bus) {
>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>> - object_get_typename(OBJECT(dev)));
>>>>> - return;
>>>>> + if (d->user_created) {
>>>>> + bool ambiguous;
>>>>> +
>>>>> + 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(dev)));
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if (!isa_bus) {
>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>> + object_get_typename(OBJECT(dev)));
>>>>> + return;
>>>>> + }
>>>>> }
>>>>> pci_piix_init_ports(d, isa_bus);
>>>>> }
>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>> +{
>>>>> + DeviceState *dev = DEVICE(obj);
>>>>> +
>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>> +}
>>>>> +
>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>> {
>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>> }
>>>>> }
>>>>> +static Property piix_ide_properties[] = {
>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>> + 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)
>>>>> {
>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>> .name = TYPE_PIIX3_IDE,
>>>>> .parent = TYPE_PCI_IDE,
>>>>> + .instance_init = pci_piix_ide_init,
>>>>> .class_init = piix3_ide_class_init,
>>>>> };
>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>> .name = TYPE_PIIX4_IDE,
>>>>> .parent = TYPE_PCI_IDE,
>>>>> + .instance_init = pci_piix_ide_init,
>>>>> .class_init = piix4_ide_class_init,
>>>>> };
>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>> --- a/hw/isa/piix.c
>>>>> +++ b/hw/isa/piix.c
>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>> /* IDE */
>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>> return;
>>>>> }
>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>> /* USB */
>>>>> if (d->has_usb) {
>>>>
>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>
>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>
>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>
>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>
> I've found the following:
>
> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>
> And:
>
> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>
> Does this perhaps mean that piix-ide does indeed have no ISA bus?
I'd be amazed if that were the case: certainly when the first motherboards came out
with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since
BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA
ioports directly. From memory the OF PCI specification has mention of workarounds
such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so
I'd be surprised if there wasn't something similar for IDE.
The wording above is a bit ambiguous because I can see the above statements would be
true if the PCI-IDE device were already switched to PCI mode, and what we're looking
for is whether a switch between the two is supported or possible.
The cmd646 datasheet section 1.4 has a fleeting mention of a document called "PCI IDE
Controller Specification Revision 1.0" which if you can find it, may provide more
information as to whether this ability is specific to the cmd646 or whether it is
also relevant to generic PCI-IDE controllers such as the PIIX series too.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-07 20:52 ` Mark Cave-Ayland
@ 2023-02-08 0:18 ` Bernhard Beschow
2023-02-08 0:43 ` BALATON Zoltan
2023-02-08 11:22 ` Philippe Mathieu-Daudé
2023-02-23 20:46 ` Bernhard Beschow
1 sibling, 2 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-08 0:18 UTC (permalink / raw)
To: Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 06/02/2023 23:40, Bernhard Beschow wrote:
>
>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>
>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>> over.
>>>>>>
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> include/hw/ide/pci.h | 1 +
>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>> hw/isa/piix.c | 5 ++++
>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>> --- a/include/hw/ide/pci.h
>>>>>> +++ b/include/hw/ide/pci.h
>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>> MemoryRegion bmdma_bar;
>>>>>> MemoryRegion cmd_bar[2];
>>>>>> MemoryRegion data_bar[2];
>>>>>> + bool user_created;
>>>>>> };
>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>> --- a/hw/ide/piix.c
>>>>>> +++ b/hw/ide/piix.c
>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>> }
>>>>>> }
>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>> +{
>>>>>> + PCIIDEState *d = opaque;
>>>>>> +
>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>> +}
>>>>>> +
>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>> {
>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>> };
>>>>>> int i;
>>>>>> + if (isa_bus) {
>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>> + } else {
>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>> + }
>>>>>> +
>>>>>> for (i = 0; i < 2; i++) {
>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>> port_info[i].iobase2);
>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>> + ISABus *isa_bus = NULL;
>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>> - 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(dev)));
>>>>>> - return;
>>>>>> - }
>>>>>> - if (!isa_bus) {
>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>> - return;
>>>>>> + if (d->user_created) {
>>>>>> + bool ambiguous;
>>>>>> +
>>>>>> + 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(dev)));
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + if (!isa_bus) {
>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>> + return;
>>>>>> + }
>>>>>> }
>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>> }
>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>> +{
>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>> +
>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>> +}
>>>>>> +
>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>> {
>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>> }
>>>>>> }
>>>>>> +static Property piix_ide_properties[] = {
>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>> + 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)
>>>>>> {
>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>> .parent = TYPE_PCI_IDE,
>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>> .class_init = piix3_ide_class_init,
>>>>>> };
>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>> .parent = TYPE_PCI_IDE,
>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>> .class_init = piix4_ide_class_init,
>>>>>> };
>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>> --- a/hw/isa/piix.c
>>>>>> +++ b/hw/isa/piix.c
>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>> /* IDE */
>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>> return;
>>>>>> }
>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>> /* USB */
>>>>>> if (d->has_usb) {
>>>>>
>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>
>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>
>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>
>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>
>> I've found the following:
>>
>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>
>> And:
>>
>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>
>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>
>I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>
>The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
PIIX4's description of PROG_IF (0x9):
"Programming Interface (PI). 80h=Capable of IDE bus master operation."
VT82C686B in comparison:
7 Master IDE Capability...........fixed at 1 (Supported)
3 Programmable Indicator - Secondary......fixed at 1 Supports both modes
1 Programmable Indicator - Primary..........fixed at 1 Supports both modes
So VT82C686B can switch modes while PIIX can't. Right?
Furthermore, from the PIIX4 documentation of bit 2 of the bus master status register BMISX (IO):
"IDE Interrupt Status (IDEINTS)—R/WC. This bit, when set to a 1, indicates when an IDE device has asserted its interrupt line. When bit 2=1, all read data from the IDE device has been transferred to main memory and all write data has been transferred to the IDE device. Software sets this bit to a 0 by writing a 1 to it. IRQ14 is used for the primary channel and IRQ15 is used for the secondary channel. Note that, if the interrupt status bit is set to a 0 by writing a 1 to this bit while the interrupt line is still at the active level, this bit remains 0 until another assertion edge is detected on the interrupt line."
So the legacy ISA IRQs seem to be used always.
The VIA documentation offers a control register where "native" or "legacy" mode interrupt routing can be selected. I haven't found a similar register for PIIX4.
So it seems to me that PIIX can't switch modes...
>
>The cmd646 datasheet section 1.4 has a fleeting mention of a document called "PCI IDE Controller Specification Revision 1.0" which if you can find it, may provide more information as to whether this ability is specific to the cmd646 or whether it is also relevant to generic PCI-IDE controllers such as the PIIX series too.
I used VT82C686B above instead of cmd646 if that's okay. I'd need to read the IDE controller specification still. But perhaps we're already wiser?
Best regards,
Bernhard
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-08 0:18 ` Bernhard Beschow
@ 2023-02-08 0:43 ` BALATON Zoltan
2023-02-08 7:09 ` Philippe Mathieu-Daudé
2023-02-08 11:22 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 40+ messages in thread
From: BALATON Zoltan @ 2023-02-08 0:43 UTC (permalink / raw)
To: Bernhard Beschow
Cc: Mark Cave-Ayland, qemu-devel, Marcel Apfelbaum,
Hervé Poussineau, Gerd Hoffmann, Michael S. Tsirkin,
Aurelien Jarno, Philippe Mathieu-Daudé, David Hildenbrand,
Peter Xu, qemu-ppc, qemu-block, John Snow, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 15099 bytes --]
On Wed, 8 Feb 2023, Bernhard Beschow wrote:
> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>>
>>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>>> over.
>>>>>>>
>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> ---
>>>>>>> include/hw/ide/pci.h | 1 +
>>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>>> hw/isa/piix.c | 5 ++++
>>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>>> --- a/include/hw/ide/pci.h
>>>>>>> +++ b/include/hw/ide/pci.h
>>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>>> MemoryRegion bmdma_bar;
>>>>>>> MemoryRegion cmd_bar[2];
>>>>>>> MemoryRegion data_bar[2];
>>>>>>> + bool user_created;
>>>>>>> };
>>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>>> --- a/hw/ide/piix.c
>>>>>>> +++ b/hw/ide/piix.c
>>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>>> }
>>>>>>> }
>>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>>> +{
>>>>>>> + PCIIDEState *d = opaque;
>>>>>>> +
>>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>>> };
>>>>>>> int i;
>>>>>>> + if (isa_bus) {
>>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>>> + } else {
>>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < 2; i++) {
>>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>>> port_info[i].iobase2);
>>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>>> + ISABus *isa_bus = NULL;
>>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>>> - 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(dev)));
>>>>>>> - return;
>>>>>>> - }
>>>>>>> - if (!isa_bus) {
>>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>>> - return;
>>>>>>> + if (d->user_created) {
>>>>>>> + bool ambiguous;
>>>>>>> +
>>>>>>> + 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(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (!isa_bus) {
>>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> }
>>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>>> }
>>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>>> +{
>>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>>> +
>>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> }
>>>>>>> }
>>>>>>> +static Property piix_ide_properties[] = {
>>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>>> + 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)
>>>>>>> {
>>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix3_ide_class_init,
>>>>>>> };
>>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix4_ide_class_init,
>>>>>>> };
>>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>>> --- a/hw/isa/piix.c
>>>>>>> +++ b/hw/isa/piix.c
>>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>>> /* IDE */
>>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>>> return;
>>>>>>> }
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>>> /* USB */
>>>>>>> if (d->has_usb) {
>>>>>>
>>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>>
>>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently
(such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>>
>>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>>
>>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>>
>>> I've found the following:
>>>
>>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>>
>>> And:
>>>
>>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>>
>>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>>
>> I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>>
>> The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
>
> PIIX4's description of PROG_IF (0x9):
> "Programming Interface (PI). 80h=Capable of IDE bus master operation."
>
> VT82C686B in comparison:
> 7 Master IDE Capability...........fixed at 1 (Supported)
> 3 Programmable Indicator - Secondary......fixed at 1 Supports both modes
> 1 Programmable Indicator - Primary..........fixed at 1 Supports both modes
>
> So VT82C686B can switch modes while PIIX can't. Right?
Unless there's some other bit somewhere... These all-in-one chips can be
confusing.
> Furthermore, from the PIIX4 documentation of bit 2 of the bus master status register BMISX (IO):
>
> "IDE Interrupt Status (IDEINTS)—R/WC. This bit, when set to a 1, indicates when an IDE device has asserted its interrupt line. When bit 2=1, all read data from the IDE device has been transferred to main memory and all write data has been transferred to the IDE device. Software sets this bit to a 0 by writing a 1 to it. IRQ14 is used for the primary channel and IRQ15 is used for the secondary channel. Note that, if the interrupt status bit is set to a 0 by writing a 1 to this bit while the interrupt line is still at the active level, this bit remains 0 until another assertion edge is detected on the interrupt line."
>
> So the legacy ISA IRQs seem to be used always.
>
> The VIA documentation offers a control register where "native" or "legacy" mode interrupt routing can be selected. I haven't found a similar register for PIIX4.
While from the docs it looks like that we have found before that on some
machines this does not work and even after writing some value the control
reg irqs remain set to legacy and software expects that. This is the case
on pegasos2 where the firmware does write the reg but then OSes still use
IRQ 14/15 for ide. Not sure about the fuloong2e but I think different
Linux kernels did different things on that machine. We have spent a lot of
time with this back then and I think found that just strapping the IRQ to
legacy and setting the regs on reset as we set now works for all guests we
tried so I'd be wary to touch this again. Out cuttent model may be
incomplete only modeling one mode of the chip but this mode is what's used
so I don't think it's worth trying to umplement an unused state that will
be reset by the firmware anyway so nothing will use that.
(We're spamming a lot of people with this, maybe you could prune the cc
if this is not any more relevant to the original patch.)
Regards,
BALATON Zoltan
> So it seems to me that PIIX can't switch modes...
>
>>
>> The cmd646 datasheet section 1.4 has a fleeting mention of a document called "PCI IDE Controller Specification Revision 1.0" which if you can find it, may provide more information as to whether this ability is specific to the cmd646 or whether it is also relevant to generic PCI-IDE controllers such as the PIIX series too.
>
> I used VT82C686B above instead of cmd646 if that's okay. I'd need to read the IDE controller specification still. But perhaps we're already wiser?
>
> Best regards,
> Bernhard
>
>>
>>
>> ATB,
>>
>> Mark.
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-08 0:43 ` BALATON Zoltan
@ 2023-02-08 7:09 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-08 7:09 UTC (permalink / raw)
To: BALATON Zoltan, Bernhard Beschow
Cc: Mark Cave-Ayland, qemu-devel, Marcel Apfelbaum,
Hervé Poussineau, Gerd Hoffmann, Michael S. Tsirkin,
Aurelien Jarno, David Hildenbrand, Peter Xu, qemu-ppc, qemu-block,
John Snow, Paolo Bonzini
On 8/2/23 01:43, BALATON Zoltan wrote:
> On Wed, 8 Feb 2023, Bernhard Beschow wrote:
>> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk>:
>>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland
>>>> <mark.cave-ayland@ilande.co.uk>:
>>>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>>>> decouples them from the ISABus. User-created devices still fish
>>>>>>>> out the
>>>>>>>> ISABus from the QOM tree and the interrupt wiring remains in
>>>>>>>> PIIX IDE.
>>>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>>>> removed once a deprecation period for user-created PIIX IDE
>>>>>>>> devices is
>>>>>>>> over.
>>>>>>>>
>>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>>> ---
>>>>>>>> include/hw/ide/pci.h | 1 +
>>>>>>>> hw/ide/piix.c | 64
>>>>>>>> ++++++++++++++++++++++++++++++++++----------
>>>>>>>> hw/isa/piix.c | 5 ++++
>>>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>>>> --- a/hw/isa/piix.c
>>>>>>>> +++ b/hw/isa/piix.c
>>>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice
>>>>>>>> *dev, const char *uhci_type,
>>>>>>>> /* IDE */
>>>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn +
>>>>>>>> 1);
>>>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>>>> return;
>>>>>>>> }
>>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>>>> /* USB */
>>>>>>>> if (d->has_usb) {
>>>>>>>
>>>>>>> I haven't checked the datasheet, but I suspect this will be
>>>>>>> similar to the cmd646/via PCI-IDE interfaces in that there will
>>>>>>> be a PCI configuration register that will switch between ISA
>>>>>>> compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs).
>>>>>>> So it would be the device configuration that would specify PCI or
>>>>>>> ISA mode, rather than the presence of an ISABus.
>>>>>>
>>>>>> I forgot about this topic already and haven't follwed this series
>>>>>> either so what I say may not fully make sense but I think CMD646
>>>>>> and via-ide are different. CMD646 is a PCI device and should use
>>>>>> PCI interrupts while via-ide is part of a southbridge/superio
>>>>>> complex and connected to the ISA PICs within that southbride, so I
>>>>>> think via-ide always uses ISA IRQs and the ISA btidge within the
>>>>>> same chip may convert that to PCI IRQs or not (that part is where
>>>>>> I'm lost also because we may not actually model it that way).
>>>>>> After a long debate we managed to find a solution back then that
>>>>>> works for every guest we use it for now so I think we don't want
>>>>>> to touch it now until some real need arises. It does not worth the
>>>>>> trouble and added complexity to model something that is not used
>>>>>> just for the sake of correctness. By the time we find a use for
>>>>>> that, the ISA emulation may evolve so it's easier to implement the
>>>>>> missing switching between isa and native mode or we may want to do
>>>>>> it differently
> (such as we do things differently now compared to what we did years
> ago). So I think it does not worth keeping the ISA model from being
> simplified for some theoretical uses in the future which we may not
> actually do any time soon.
Indeed we don't want (and have the time) to model features we don't need
or will never use. However taking the time to correctly understand these
devices help us to correctly model them. In particular when design flaws
have been identified in some models.
> But I don't want to get into this again so
> just shared my thoughts and feel free to ignore it. I don't care where
> these patches go as long as the VIA model keeps working for me.
We certainly want to keep what we currently have working :)
>>>>> I have a vague memory that ISA compatibility mode was part of the
>>>>> original PCI-BMDMA specification, but it has been a while since I
>>>>> last looked.
>>>>>
>>>>> Bernhard, is there any mention of this in the PIIX datasheet(s)?
>>>>> For reference the cmd646 datasheet specifies that ISA mode or PCI
>>>>> mode is determined by register PROG_IF (0x9) in PCI configuration
>>>>> space.
>>>>
>>>> I've found the following:
>>>>
>>>> "Only PCI masters have access to the IDE port. ISA Bus masters
>>>> cannot access the IDE I/O port addresses. Memory targeted by the IDE
>>>> interface acting as a PCI Bus master on behalf of IDE DMA slaves
>>>> must reside on PCI, usually main memory implemented by the
>>>> host-to-PCI bridge."
>>>>
>>>> And:
>>>>
>>>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave
>>>> device."
>>>>
>>>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>>>
>>> I'd be amazed if that were the case: certainly when the first
>>> motherboards came out with PCI and ISA slots, I'd expect the IDE
>>> legacy mode to be enabled by default since BIOSes and OSs such as DOS
>>> wouldn't have been PCI aware and would access the ISA ioports
>>> directly. From memory the OF PCI specification has mention of
>>> workarounds such as mapping the old VGA memory to PCI MMIO space for
>>> compatibility reasons, so I'd be surprised if there wasn't something
>>> similar for IDE.
>>>
>>> The wording above is a bit ambiguous because I can see the above
>>> statements would be true if the PCI-IDE device were already switched
>>> to PCI mode, and what we're looking for is whether a switch between
>>> the two is supported or possible.
>>
>> PIIX4's description of PROG_IF (0x9):
>> "Programming Interface (PI). 80h=Capable of IDE bus master operation."
>>
>> VT82C686B in comparison:
>> 7 Master IDE Capability...........fixed at 1 (Supported)
>> 3 Programmable Indicator - Secondary......fixed at 1 Supports both modes
>> 1 Programmable Indicator - Primary..........fixed at 1 Supports both
>> modes
>>
>> So VT82C686B can switch modes while PIIX can't. Right?
>
> Unless there's some other bit somewhere... These all-in-one chips can be
> confusing.
>
>> Furthermore, from the PIIX4 documentation of bit 2 of the bus master
>> status register BMISX (IO):
>>
>> "IDE Interrupt Status (IDEINTS)—R/WC. This bit, when set to a 1,
>> indicates when an IDE device has asserted its interrupt line. When bit
>> 2=1, all read data from the IDE device has been transferred to main
>> memory and all write data has been transferred to the IDE device.
>> Software sets this bit to a 0 by writing a 1 to it. IRQ14 is used for
>> the primary channel and IRQ15 is used for the secondary channel. Note
>> that, if the interrupt status bit is set to a 0 by writing a 1 to this
>> bit while the interrupt line is still at the active level, this bit
>> remains 0 until another assertion edge is detected on the interrupt
>> line."
>>
>> So the legacy ISA IRQs seem to be used always.
>>
>> The VIA documentation offers a control register where "native" or
>> "legacy" mode interrupt routing can be selected. I haven't found a
>> similar register for PIIX4.
>
> While from the docs it looks like that we have found before that on some
> machines this does not work and even after writing some value the
> control reg irqs remain set to legacy and software expects that. This is
> the case on pegasos2 where the firmware does write the reg but then OSes
> still use IRQ 14/15 for ide. Not sure about the fuloong2e but I think
> different Linux kernels did different things on that machine. We have
> spent a lot of time with this back then and I think found that just
> strapping the IRQ to legacy and setting the regs on reset as we set now
> works for all guests we tried so I'd be wary to touch this again. Out
> cuttent model may be incomplete only modeling one mode of the chip but
> this mode is what's used so I don't think it's worth trying to umplement
> an unused state that will be reset by the firmware anyway so nothing
> will use that.
>
> (We're spamming a lot of people with this, maybe you could prune the cc
> if this is not any more relevant to the original patch.)
Don't worry too much about that, it is easier to filter out message than
figuring you are interested and try to filter in everything. Better safe
than sorry!
>> So it seems to me that PIIX can't switch modes...
>>
>>>
>>> The cmd646 datasheet section 1.4 has a fleeting mention of a document
>>> called "PCI IDE Controller Specification Revision 1.0" which if you
>>> can find it, may provide more information as to whether this ability
>>> is specific to the cmd646 or whether it is also relevant to generic
>>> PCI-IDE controllers such as the PIIX series too.
>>
>> I used VT82C686B above instead of cmd646 if that's okay. I'd need to
>> read the IDE controller specification still. But perhaps we're already
>> wiser?
>>
>> Best regards,
>> Bernhard
>>
>>>
>>>
>>> ATB,
>>>
>>> Mark.
>>
>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-08 0:18 ` Bernhard Beschow
2023-02-08 0:43 ` BALATON Zoltan
@ 2023-02-08 11:22 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-08 11:22 UTC (permalink / raw)
To: Bernhard Beschow, Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On 8/2/23 01:18, Bernhard Beschow wrote:
>
>
> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>
>>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>>
>>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>>> over.
>>>>>>>
>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> ---
>>>>>>> include/hw/ide/pci.h | 1 +
>>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>>> hw/isa/piix.c | 5 ++++
>>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>>> --- a/include/hw/ide/pci.h
>>>>>>> +++ b/include/hw/ide/pci.h
>>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>>> MemoryRegion bmdma_bar;
>>>>>>> MemoryRegion cmd_bar[2];
>>>>>>> MemoryRegion data_bar[2];
>>>>>>> + bool user_created;
>>>>>>> };
>>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>>> --- a/hw/ide/piix.c
>>>>>>> +++ b/hw/ide/piix.c
>>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>>> }
>>>>>>> }
>>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>>> +{
>>>>>>> + PCIIDEState *d = opaque;
>>>>>>> +
>>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>>> };
>>>>>>> int i;
>>>>>>> + if (isa_bus) {
>>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>>> + } else {
>>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < 2; i++) {
>>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>>> port_info[i].iobase2);
>>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>>> + ISABus *isa_bus = NULL;
>>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>>> - 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(dev)));
>>>>>>> - return;
>>>>>>> - }
>>>>>>> - if (!isa_bus) {
>>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>>> - return;
>>>>>>> + if (d->user_created) {
>>>>>>> + bool ambiguous;
>>>>>>> +
>>>>>>> + 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(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (!isa_bus) {
>>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> }
>>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>>> }
>>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>>> +{
>>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>>> +
>>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> }
>>>>>>> }
>>>>>>> +static Property piix_ide_properties[] = {
>>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>>> + 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)
>>>>>>> {
>>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix3_ide_class_init,
>>>>>>> };
>>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix4_ide_class_init,
>>>>>>> };
>>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>>> --- a/hw/isa/piix.c
>>>>>>> +++ b/hw/isa/piix.c
>>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>>> /* IDE */
>>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>>> return;
>>>>>>> }
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>>> /* USB */
>>>>>>> if (d->has_usb) {
>>>>>>
>>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>>
>>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>>
>>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>>
>>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>>
>>> I've found the following:
>>>
>>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>>
>>> And:
>>>
>>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>>
>>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>>
>> I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>>
>> The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
>
> PIIX4's description of PROG_IF (0x9):
> "Programming Interface (PI). 80h=Capable of IDE bus master operation."
>
> VT82C686B in comparison:
> 7 Master IDE Capability...........fixed at 1 (Supported)
> 3 Programmable Indicator - Secondary......fixed at 1 Supports both modes
> 1 Programmable Indicator - Primary..........fixed at 1 Supports both modes
>
> So VT82C686B can switch modes while PIIX can't. Right?
>
> Furthermore, from the PIIX4 documentation of bit 2 of the bus master status register BMISX (IO):
>
> "IDE Interrupt Status (IDEINTS)—R/WC. This bit, when set to a 1, indicates when an IDE device has asserted its interrupt line. When bit 2=1, all read data from the IDE device has been transferred to main memory and all write data has been transferred to the IDE device. Software sets this bit to a 0 by writing a 1 to it. IRQ14 is used for the primary channel and IRQ15 is used for the secondary channel. Note that, if the interrupt status bit is set to a 0 by writing a 1 to this bit while the interrupt line is still at the active level, this bit remains 0 until another assertion edge is detected on the interrupt line."
>
> So the legacy ISA IRQs seem to be used always.
See also:
11.3.4. DEVICE LOCATION ON PCI BUS OR ISA BUS
Most of the peripheral devices have the capability to exist on the PCI
Bus or the ISA/EIO Bus. However, PIIX4 does not support RTC or Keyboard
Controller to be resided on the PCI Bus. The Device Activity Monitor is
watching cycles on the PCI bus to generate activity events. The device
monitors also can be enabled to forward cycles to the device’s enabled
addresses to the ISA bus. Devices that reside on the ISA Bus must have
both address ranges selected and enabled AND the ISA/EIO forwarding
enabled. [...]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-07 20:52 ` Mark Cave-Ayland
2023-02-08 0:18 ` Bernhard Beschow
@ 2023-02-23 20:46 ` Bernhard Beschow
2023-03-01 16:42 ` Mark Cave-Ayland
1 sibling, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-23 20:46 UTC (permalink / raw)
To: Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 06/02/2023 23:40, Bernhard Beschow wrote:
>
>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>
>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>> over.
>>>>>>
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> include/hw/ide/pci.h | 1 +
>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>> hw/isa/piix.c | 5 ++++
>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>> --- a/include/hw/ide/pci.h
>>>>>> +++ b/include/hw/ide/pci.h
>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>> MemoryRegion bmdma_bar;
>>>>>> MemoryRegion cmd_bar[2];
>>>>>> MemoryRegion data_bar[2];
>>>>>> + bool user_created;
>>>>>> };
>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>> --- a/hw/ide/piix.c
>>>>>> +++ b/hw/ide/piix.c
>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>> }
>>>>>> }
>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>> +{
>>>>>> + PCIIDEState *d = opaque;
>>>>>> +
>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>> +}
>>>>>> +
>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>> {
>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>> };
>>>>>> int i;
>>>>>> + if (isa_bus) {
>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>> + } else {
>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>> + }
>>>>>> +
>>>>>> for (i = 0; i < 2; i++) {
>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>> port_info[i].iobase2);
>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>> + ISABus *isa_bus = NULL;
>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>> - 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(dev)));
>>>>>> - return;
>>>>>> - }
>>>>>> - if (!isa_bus) {
>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>> - return;
>>>>>> + if (d->user_created) {
>>>>>> + bool ambiguous;
>>>>>> +
>>>>>> + 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(dev)));
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + if (!isa_bus) {
>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>> + return;
>>>>>> + }
>>>>>> }
>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>> }
>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>> +{
>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>> +
>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>> +}
>>>>>> +
>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>> {
>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>> }
>>>>>> }
>>>>>> +static Property piix_ide_properties[] = {
>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>> + 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)
>>>>>> {
>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>> .parent = TYPE_PCI_IDE,
>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>> .class_init = piix3_ide_class_init,
>>>>>> };
>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>> .parent = TYPE_PCI_IDE,
>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>> .class_init = piix4_ide_class_init,
>>>>>> };
>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>> --- a/hw/isa/piix.c
>>>>>> +++ b/hw/isa/piix.c
>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>> /* IDE */
>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>> return;
>>>>>> }
>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>> /* USB */
>>>>>> if (d->has_usb) {
>>>>>
>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>
>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>
>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>
>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>
>> I've found the following:
>>
>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>
>> And:
>>
>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>
>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>
>I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>
>The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
A switch is definitely impossible: The PIIX IDE function has the 0x3c (interrupt line) and 0x3d (interrupt pin) registers reserved. It's fixed in legacy mode (prog-if is 0x80).
Best regards,
Bernhard
>
>The cmd646 datasheet section 1.4 has a fleeting mention of a document called "PCI IDE Controller Specification Revision 1.0" which if you can find it, may provide more information as to whether this ability is specific to the cmd646 or whether it is also relevant to generic PCI-IDE controllers such as the PIIX series too.
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-02-23 20:46 ` Bernhard Beschow
@ 2023-03-01 16:42 ` Mark Cave-Ayland
2023-03-01 21:12 ` Bernhard Beschow
0 siblings, 1 reply; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-03-01 16:42 UTC (permalink / raw)
To: Bernhard Beschow, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
On 23/02/2023 20:46, Bernhard Beschow wrote:
>
>
> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>
>>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>>
>>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>>> over.
>>>>>>>
>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> ---
>>>>>>> include/hw/ide/pci.h | 1 +
>>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>>> hw/isa/piix.c | 5 ++++
>>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>>> --- a/include/hw/ide/pci.h
>>>>>>> +++ b/include/hw/ide/pci.h
>>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>>> MemoryRegion bmdma_bar;
>>>>>>> MemoryRegion cmd_bar[2];
>>>>>>> MemoryRegion data_bar[2];
>>>>>>> + bool user_created;
>>>>>>> };
>>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>>> --- a/hw/ide/piix.c
>>>>>>> +++ b/hw/ide/piix.c
>>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>>> }
>>>>>>> }
>>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>>> +{
>>>>>>> + PCIIDEState *d = opaque;
>>>>>>> +
>>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>>> };
>>>>>>> int i;
>>>>>>> + if (isa_bus) {
>>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>>> + } else {
>>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < 2; i++) {
>>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>>> port_info[i].iobase2);
>>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>>> + ISABus *isa_bus = NULL;
>>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>>> - 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(dev)));
>>>>>>> - return;
>>>>>>> - }
>>>>>>> - if (!isa_bus) {
>>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>>> - return;
>>>>>>> + if (d->user_created) {
>>>>>>> + bool ambiguous;
>>>>>>> +
>>>>>>> + 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(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (!isa_bus) {
>>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>>> + return;
>>>>>>> + }
>>>>>>> }
>>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>>> }
>>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>>> +{
>>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>>> +
>>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> {
>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>> }
>>>>>>> }
>>>>>>> +static Property piix_ide_properties[] = {
>>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>>> + 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)
>>>>>>> {
>>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix3_ide_class_init,
>>>>>>> };
>>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>> .class_init = piix4_ide_class_init,
>>>>>>> };
>>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>>> --- a/hw/isa/piix.c
>>>>>>> +++ b/hw/isa/piix.c
>>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>>> /* IDE */
>>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>>> return;
>>>>>>> }
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>>> /* USB */
>>>>>>> if (d->has_usb) {
>>>>>>
>>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>>
>>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>>
>>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>>
>>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>>
>>> I've found the following:
>>>
>>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>>
>>> And:
>>>
>>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>>
>>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>>
>> I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>>
>> The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
>
> A switch is definitely impossible: The PIIX IDE function has the 0x3c (interrupt line) and 0x3d (interrupt pin) registers reserved. It's fixed in legacy mode (prog-if is 0x80).
>
> Best regards,
> Bernhard
Thanks for checking this. If you have any upcoming patches that touch this file, it's
worth adding a comment explaining this with a reference to the relevant part of the
datasheet, and also update wmask accordingly.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-03-01 16:42 ` Mark Cave-Ayland
@ 2023-03-01 21:12 ` Bernhard Beschow
0 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-03-01 21:12 UTC (permalink / raw)
To: Mark Cave-Ayland, BALATON Zoltan
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Michael S. Tsirkin, Aurelien Jarno,
Philippe Mathieu-Daudé, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini
Am 1. März 2023 16:42:16 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 23/02/2023 20:46, Bernhard Beschow wrote:
>>
>>
>> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>>
>>>> Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>>> On 05/02/2023 22:21, BALATON Zoltan wrote:
>>>>>
>>>>>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>>>>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>>>>>> Internal instances now defer interrupt wiring to the caller which
>>>>>>>> decouples them from the ISABus. User-created devices still fish out the
>>>>>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>>>>>> The latter mechanism is considered a workaround and intended to be
>>>>>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>>>>>> over.
>>>>>>>>
>>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>>> ---
>>>>>>>> include/hw/ide/pci.h | 1 +
>>>>>>>> hw/ide/piix.c | 64 ++++++++++++++++++++++++++++++++++----------
>>>>>>>> hw/isa/piix.c | 5 ++++
>>>>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>>>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>>>>>> --- a/include/hw/ide/pci.h
>>>>>>>> +++ b/include/hw/ide/pci.h
>>>>>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>>>>> MemoryRegion bmdma_bar;
>>>>>>>> MemoryRegion cmd_bar[2];
>>>>>>>> MemoryRegion data_bar[2];
>>>>>>>> + bool user_created;
>>>>>>>> };
>>>>>>>> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>>>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>>>>>> index 5980045db0..f0d95761ac 100644
>>>>>>>> --- a/hw/ide/piix.c
>>>>>>>> +++ b/hw/ide/piix.c
>>>>>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>>>>> }
>>>>>>>> }
>>>>>>>> +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>>>>>> +{
>>>>>>>> + PCIIDEState *d = opaque;
>>>>>>>> +
>>>>>>>> + qemu_set_irq(d->isa_irqs[n], level);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static void piix_ide_reset(DeviceState *dev)
>>>>>>>> {
>>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>>>>>>> };
>>>>>>>> int i;
>>>>>>>> + if (isa_bus) {
>>>>>>>> + d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>>>>>> + d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>>>>>> + } else {
>>>>>>>> + qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> for (i = 0; i < 2; i++) {
>>>>>>>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>>>>> ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>>>>> port_info[i].iobase2);
>>>>>>>> - ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>>>>>> + ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>>>>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>>>>> d->bmdma[i].bus = &d->bus[i];
>>>>>>>> @@ -154,8 +168,7 @@ 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;
>>>>>>>> + ISABus *isa_bus = NULL;
>>>>>>>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>>>>> @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>>>>>>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>>>>> - 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(dev)));
>>>>>>>> - return;
>>>>>>>> - }
>>>>>>>> - if (!isa_bus) {
>>>>>>>> - error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>>> - object_get_typename(OBJECT(dev)));
>>>>>>>> - return;
>>>>>>>> + if (d->user_created) {
>>>>>>>> + bool ambiguous;
>>>>>>>> +
>>>>>>>> + 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(dev)));
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + if (!isa_bus) {
>>>>>>>> + error_setg(errp, "No ISA bus found while %s requires one",
>>>>>>>> + object_get_typename(OBJECT(dev)));
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> }
>>>>>>>> pci_piix_init_ports(d, isa_bus);
>>>>>>>> }
>>>>>>>> +static void pci_piix_ide_init(Object *obj)
>>>>>>>> +{
>>>>>>>> + DeviceState *dev = DEVICE(obj);
>>>>>>>> +
>>>>>>>> + qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>>> {
>>>>>>>> PCIIDEState *d = PCI_IDE(dev);
>>>>>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>>>>> }
>>>>>>>> }
>>>>>>>> +static Property piix_ide_properties[] = {
>>>>>>>> + DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>>>>>> + 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)
>>>>>>>> {
>>>>>>>> @@ -205,11 +237,13 @@ 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 = {
>>>>>>>> .name = TYPE_PIIX3_IDE,
>>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>>> .class_init = piix3_ide_class_init,
>>>>>>>> };
>>>>>>>> @@ -227,11 +261,13 @@ 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 = {
>>>>>>>> .name = TYPE_PIIX4_IDE,
>>>>>>>> .parent = TYPE_PCI_IDE,
>>>>>>>> + .instance_init = pci_piix_ide_init,
>>>>>>>> .class_init = piix4_ide_class_init,
>>>>>>>> };
>>>>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>>>>>> index 54a1246a9d..f9974c2a77 100644
>>>>>>>> --- a/hw/isa/piix.c
>>>>>>>> +++ b/hw/isa/piix.c
>>>>>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>>>>>>> /* IDE */
>>>>>>>> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>>>>>> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>>>>> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>>>>> return;
>>>>>>>> }
>>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>>>>>> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>>>>>> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>>>>> /* USB */
>>>>>>>> if (d->has_usb) {
>>>>>>>
>>>>>>> I haven't checked the datasheet, but I suspect this will be similar to the cmd646/via PCI-IDE interfaces in that there will be a PCI configuration register that will switch between ISA compatibility mode (and ISA irqs) and PCI mode (with PCI IRQs). So it would be the device configuration that would specify PCI or ISA mode, rather than the presence of an ISABus.
>>>>>>
>>>>>> I forgot about this topic already and haven't follwed this series either so what I say may not fully make sense but I think CMD646 and via-ide are different. CMD646 is a PCI device and should use PCI interrupts while via-ide is part of a southbridge/superio complex and connected to the ISA PICs within that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge within the same chip may convert that to PCI IRQs or not (that part is where I'm lost also because we may not actually model it that way). After a long debate we managed to find a solution back then that works for every guest we use it for now so I think we don't want to touch it now until some real need arises. It does not worth the trouble and added complexity to model something that is not used just for the sake of correctness. By the time we find a use for that, the ISA emulation may evolve so it's easier to implement the missing switching between isa and native mode or we may want to do it differently (such as we do things differently now compared to what we did years ago). So I think it does not worth keeping the ISA model from being simplified for some theoretical uses in the future which we may not actually do any time soon. But I don't want to get into this again so just shared my thoughts and feel free to ignore it. I don't care where these patches go as long as the VIA model keeps working for me.
>>>>>
>>>>> I have a vague memory that ISA compatibility mode was part of the original PCI-BMDMA specification, but it has been a while since I last looked.
>>>>>
>>>>> Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference the cmd646 datasheet specifies that ISA mode or PCI mode is determined by register PROG_IF (0x9) in PCI configuration space.
>>>>
>>>> I've found the following:
>>>>
>>>> "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the host-to-PCI bridge."
>>>>
>>>> And:
>>>>
>>>> "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."
>>>>
>>>> Does this perhaps mean that piix-ide does indeed have no ISA bus?
>>>
>>> I'd be amazed if that were the case: certainly when the first motherboards came out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would access the ISA ioports directly. From memory the OF PCI specification has mention of workarounds such as mapping the old VGA memory to PCI MMIO space for compatibility reasons, so I'd be surprised if there wasn't something similar for IDE.
>>>
>>> The wording above is a bit ambiguous because I can see the above statements would be true if the PCI-IDE device were already switched to PCI mode, and what we're looking for is whether a switch between the two is supported or possible.
>>
>> A switch is definitely impossible: The PIIX IDE function has the 0x3c (interrupt line) and 0x3d (interrupt pin) registers reserved. It's fixed in legacy mode (prog-if is 0x80).
>>
>> Best regards,
>> Bernhard
>
>Thanks for checking this.
Thanks for the valuable pointer... interesting and very comprehensive read!
>If you have any upcoming patches that touch this file, it's worth adding a comment explaining this with a reference to the relevant part of the datasheet, and also update wmask accordingly.
Will do -- if I ever get to it...
Phil, you have an alternate series. Would you be able to do this, too?
Thanks,
Bernhard
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
2023-01-26 21:17 ` [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances Bernhard Beschow
2023-02-05 21:58 ` Mark Cave-Ayland
@ 2023-02-24 16:20 ` Michael S. Tsirkin
1 sibling, 0 replies; 40+ messages in thread
From: Michael S. Tsirkin @ 2023-02-24 16:20 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On Thu, Jan 26, 2023 at 10:17:37PM +0100, Bernhard Beschow wrote:
> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
> index 54a1246a9d..f9974c2a77 100644
> --- a/hw/isa/piix.c
> +++ b/hw/isa/piix.c
> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>
> /* IDE */
> qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
> + qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
> if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
> return;
> }
> + qdev_connect_gpio_out(DEVICE(&d->ide), 0,
> + qdev_get_gpio_in(DEVICE(&d->pic), 14));
> + qdev_connect_gpio_out(DEVICE(&d->ide), 1,
> + qdev_get_gpio_in(DEVICE(&d->pic), 15));
>
OK, but I think we should prefix this with "x-" so we don't commit
to this as a stable API.
> /* USB */
> if (d->has_usb) {
> --
> 2.39.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (6 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-27 0:27 ` Philippe Mathieu-Daudé
2023-02-05 22:02 ` Mark Cave-Ayland
2023-01-26 21:17 ` [PATCH v2 09/10] hw/isa: Remove use of global isa bus Bernhard Beschow
` (3 subsequent siblings)
11 siblings, 2 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow, 20210518215545.1793947-10-philmd
Both callers to ide_init_ioport() have access to the I/O memory region
of the ISA bus, so can pass it directly. This allows ide_init_ioport()
to directly call portio_list_init().
Note, now the callers become the owner of the PortioList.
Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/ide/internal.h | 3 ++-
hw/ide/ioport.c | 15 ++++++++-------
hw/ide/isa.c | 4 +++-
hw/ide/piix.c | 8 ++++++--
4 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 42c49414f4..c3e4d192fa 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -628,7 +628,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
int chs_trans, Error **errp);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_exit(IDEState *s);
-void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
+ int iobase, int iobase2);
void ide_register_restart_cb(IDEBus *bus);
void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..00e9baf0d1 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
PORTIO_END_OF_LIST(),
};
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
+ int iobase, int iobase2)
{
- /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
- bridge has been setup properly to always register with ISA. */
- isa_register_portio_list(dev, &bus->portio_list,
- iobase, ide_portio_list, bus, "ide");
+ assert(address_space_io);
+
+ portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
+ address_space_io, iobase);
if (iobase2) {
- isa_register_portio_list(dev, &bus->portio2_list,
- iobase2, ide_portio2_list, bus, "ide");
+ portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
+ "ide", address_space_io, iobase2);
}
}
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 8bedbd13f1..cab5d0a07a 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
{
ISADevice *isadev = ISA_DEVICE(dev);
ISAIDEState *s = ISA_IDE(dev);
+ ISABus *isabus = isa_bus_from_device(isadev);
ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
- ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+ ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
+ s->iobase, s->iobase2);
s->irq = isa_get_irq(isadev, s->isairq);
ide_init2(&s->bus, s->irq);
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index f0d95761ac..236b5b7416 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -29,6 +29,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qemu/module.h"
@@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
{0x1f0, 0x3f6, 14},
{0x170, 0x376, 15},
};
+ PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
int i;
+ assert(pci_bus);
+
if (isa_bus) {
d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
@@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
for (i = 0; i < 2; i++) {
ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
- ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
- port_info[i].iobase2);
+ ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
+ port_info[i].iobase, port_info[i].iobase2);
ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
2023-01-26 21:17 ` [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice Bernhard Beschow
@ 2023-01-27 0:27 ` Philippe Mathieu-Daudé
2023-02-05 22:02 ` Mark Cave-Ayland
1 sibling, 0 replies; 40+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-27 0:27 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, David Hildenbrand, Peter Xu,
qemu-ppc, qemu-block, John Snow, Paolo Bonzini,
20210518215545.1793947-10-philmd
On 26/1/23 22:17, Bernhard Beschow wrote:
> Both callers to ide_init_ioport() have access to the I/O memory region
> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
> to directly call portio_list_init().
>
> Note, now the callers become the owner of the PortioList.
>
> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
> 'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/ide/internal.h | 3 ++-
> hw/ide/ioport.c | 15 ++++++++-------
> hw/ide/isa.c | 4 +++-
> hw/ide/piix.c | 8 ++++++--
> 4 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 42c49414f4..c3e4d192fa 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -628,7 +628,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
> int chs_trans, Error **errp);
> void ide_init2(IDEBus *bus, qemu_irq irq);
> void ide_exit(IDEState *s);
> -void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> + int iobase, int iobase2);
> void ide_register_restart_cb(IDEBus *bus);
>
> void ide_exec_cmd(IDEBus *bus, uint32_t val);
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..00e9baf0d1 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
> PORTIO_END_OF_LIST(),
> };
>
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> + int iobase, int iobase2)
> {
Eh I have almost the same change locally :)
$ git diff d2968dc5d0940799~..629b479f0b8f082688
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index fcfaa00cb9..f82e00076a 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,20 +50,21 @@ static const MemoryRegionPortio ide_portio2_list[] = {
PORTIO_END_OF_LIST(),
};
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_init_ioport(IDEBus *bus, Object *owner, ISADevice *dev,
+ MemoryRegion *isa_io, int iobase, int iobase2)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
2023-01-26 21:17 ` [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice Bernhard Beschow
2023-01-27 0:27 ` Philippe Mathieu-Daudé
@ 2023-02-05 22:02 ` Mark Cave-Ayland
2023-04-22 16:23 ` Bernhard Beschow
1 sibling, 1 reply; 40+ messages in thread
From: Mark Cave-Ayland @ 2023-02-05 22:02 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, 20210518215545.1793947-10-philmd
On 26/01/2023 21:17, Bernhard Beschow wrote:
> Both callers to ide_init_ioport() have access to the I/O memory region
> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
> to directly call portio_list_init().
>
> Note, now the callers become the owner of the PortioList.
>
> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
> 'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/ide/internal.h | 3 ++-
> hw/ide/ioport.c | 15 ++++++++-------
> hw/ide/isa.c | 4 +++-
> hw/ide/piix.c | 8 ++++++--
> 4 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 42c49414f4..c3e4d192fa 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -628,7 +628,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
> int chs_trans, Error **errp);
> void ide_init2(IDEBus *bus, qemu_irq irq);
> void ide_exit(IDEState *s);
> -void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> + int iobase, int iobase2);
> void ide_register_restart_cb(IDEBus *bus);
>
> void ide_exec_cmd(IDEBus *bus, uint32_t val);
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..00e9baf0d1 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
> PORTIO_END_OF_LIST(),
> };
>
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> + int iobase, int iobase2)
> {
> - /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
> - bridge has been setup properly to always register with ISA. */
> - isa_register_portio_list(dev, &bus->portio_list,
> - iobase, ide_portio_list, bus, "ide");
> + assert(address_space_io);
> +
> + portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
> + address_space_io, iobase);
>
> if (iobase2) {
> - isa_register_portio_list(dev, &bus->portio2_list,
> - iobase2, ide_portio2_list, bus, "ide");
> + portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
> + "ide", address_space_io, iobase2);
> }
> }
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 8bedbd13f1..cab5d0a07a 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
> {
> ISADevice *isadev = ISA_DEVICE(dev);
> ISAIDEState *s = ISA_IDE(dev);
> + ISABus *isabus = isa_bus_from_device(isadev);
>
> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
> - ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
> + ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
> + s->iobase, s->iobase2);
> s->irq = isa_get_irq(isadev, s->isairq);
> ide_init2(&s->bus, s->irq);
> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index f0d95761ac..236b5b7416 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -29,6 +29,7 @@
>
> #include "qemu/osdep.h"
> #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> #include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> @@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
> {0x1f0, 0x3f6, 14},
> {0x170, 0x376, 15},
> };
> + PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
> int i;
>
> + assert(pci_bus);
> +
> if (isa_bus) {
> d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
> d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
> @@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>
> for (i = 0; i < 2; i++) {
> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> - ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> - port_info[i].iobase2);
> + ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
> + port_info[i].iobase, port_info[i].iobase2);
> ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
Again, given that I suspect ioports are specific to x86 I'd be inclined to leave this
as a reference to ISA. I could see there being a function that exists such as
isa_get_address_space_io(ISADevice *isa) in the same way as pci_address_space_io(),
for example.
ATB,
Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
2023-02-05 22:02 ` Mark Cave-Ayland
@ 2023-04-22 16:23 ` Bernhard Beschow
0 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-04-22 16:23 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, 20210518215545.1793947-10-philmd
Am 5. Februar 2023 22:02:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 26/01/2023 21:17, Bernhard Beschow wrote:
>
>> Both callers to ide_init_ioport() have access to the I/O memory region
>> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
>> to directly call portio_list_init().
>>
>> Note, now the callers become the owner of the PortioList.
>>
>> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
>> 'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> include/hw/ide/internal.h | 3 ++-
>> hw/ide/ioport.c | 15 ++++++++-------
>> hw/ide/isa.c | 4 +++-
>> hw/ide/piix.c | 8 ++++++--
>> 4 files changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 42c49414f4..c3e4d192fa 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -628,7 +628,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> int chs_trans, Error **errp);
>> void ide_init2(IDEBus *bus, qemu_irq irq);
>> void ide_exit(IDEState *s);
>> -void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
>> + int iobase, int iobase2);
>> void ide_register_restart_cb(IDEBus *bus);
>> void ide_exec_cmd(IDEBus *bus, uint32_t val);
>> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
>> index b613ff3bba..00e9baf0d1 100644
>> --- a/hw/ide/ioport.c
>> +++ b/hw/ide/ioport.c
>> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>> PORTIO_END_OF_LIST(),
>> };
>> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
>> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
>> + int iobase, int iobase2)
>> {
>> - /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
>> - bridge has been setup properly to always register with ISA. */
>> - isa_register_portio_list(dev, &bus->portio_list,
>> - iobase, ide_portio_list, bus, "ide");
>> + assert(address_space_io);
>> +
>> + portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
>> + address_space_io, iobase);
>> if (iobase2) {
>> - isa_register_portio_list(dev, &bus->portio2_list,
>> - iobase2, ide_portio2_list, bus, "ide");
>> + portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
>> + "ide", address_space_io, iobase2);
>> }
>> }
>> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
>> index 8bedbd13f1..cab5d0a07a 100644
>> --- a/hw/ide/isa.c
>> +++ b/hw/ide/isa.c
>> @@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
>> {
>> ISADevice *isadev = ISA_DEVICE(dev);
>> ISAIDEState *s = ISA_IDE(dev);
>> + ISABus *isabus = isa_bus_from_device(isadev);
>> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
>> - ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
>> + ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
>> + s->iobase, s->iobase2);
>> s->irq = isa_get_irq(isadev, s->isairq);
>> ide_init2(&s->bus, s->irq);
>> vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index f0d95761ac..236b5b7416 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -29,6 +29,7 @@
>> #include "qemu/osdep.h"
>> #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>> #include "migration/vmstate.h"
>> #include "qapi/error.h"
>> #include "qemu/module.h"
>> @@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>> {0x1f0, 0x3f6, 14},
>> {0x170, 0x376, 15},
>> };
>> + PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
>> int i;
>> + assert(pci_bus);
>> +
>> if (isa_bus) {
>> d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>> d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>> @@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>> for (i = 0; i < 2; i++) {
>> ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> - ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>> - port_info[i].iobase2);
>> + ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
>> + port_info[i].iobase, port_info[i].iobase2);
>> ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>> bmdma_init(&d->bus[i], &d->bmdma[i], d);
>
>Again, given that I suspect ioports are specific to x86 I'd be inclined to leave this as a reference to ISA. I could see there being a function that exists such as isa_get_address_space_io(ISADevice *isa) in the same way as pci_address_space_io(), for example.
Hi Mark,
I've just posted a series resolving the same problem in a different way.
For some reason I thought you were the maintainer of cmd646. So I was surprised not to see you in the recipients list there...
Best regards,
Bernhard
>
>
>ATB,
>
>Mark.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 09/10] hw/isa: Remove use of global isa bus
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (7 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-26 21:17 ` [PATCH v2 10/10] hw/isa/isa-bus: Resolve isabus global Bernhard Beschow
` (2 subsequent siblings)
11 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Philippe Mathieu-Daudé, Bernhard Beschow
From: Philippe Mathieu-Daudé <philmd@redhat.com>
In the previous commits we removed all calls to these functions
passing a NULL ISADevice argument. We can simplify and remove
the use of the global isabus object.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210518215545.1793947-11-philmd@redhat.com>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/isa/isa.h | 2 +-
hw/isa/isa-bus.c | 30 +++++++++++++++++++++++-------
2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 8dd2953211..486851e7cb 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -108,7 +108,7 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
* function makes it easy to create multiple MemoryRegions for a single
* device and use the legacy portio routines.
*
- * @dev: the ISADevice against which these are registered; may be NULL.
+ * @dev: the ISADevice against which these are registered
* @piolist: the PortioList associated with the io ports
* @start: the base I/O port against which the portio->offset is applied.
* @portio: the ports, sorted by offset.
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index d3e2d9de35..8bae5cc473 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -81,8 +81,12 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
*/
qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
{
- assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
+ ISABus *isabus;
+
+ assert(dev);
assert(isairq < ISA_NUM_IRQS);
+ isabus = isa_bus_from_device(dev);
+
return isabus->irqs[isairq];
}
@@ -115,6 +119,11 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
{
+ ISABus *isabus;
+
+ assert(dev);
+ isabus = isa_bus_from_device(dev);
+
memory_region_add_subregion(isabus->address_space_io, start, io);
isa_init_ioport(dev, start);
}
@@ -124,6 +133,11 @@ void isa_register_portio_list(ISADevice *dev,
const MemoryRegionPortio *pio_start,
void *opaque, const char *name)
{
+ ISABus *isabus;
+
+ assert(dev);
+ isabus = isa_bus_from_device(dev);
+
/* START is how we should treat DEV, regardless of the actual
contents of the portio array. This is how the old code
actually handled e.g. the FDC device. */
@@ -243,18 +257,20 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
MemoryRegion *isa_address_space(ISADevice *dev)
{
- if (dev) {
- return isa_bus_from_device(dev)->address_space;
- }
+ ISABus *isabus;
+
+ assert(dev);
+ isabus = isa_bus_from_device(dev);
return isabus->address_space;
}
MemoryRegion *isa_address_space_io(ISADevice *dev)
{
- if (dev) {
- return isa_bus_from_device(dev)->address_space_io;
- }
+ ISABus *isabus;
+
+ assert(dev);
+ isabus = isa_bus_from_device(dev);
return isabus->address_space_io;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v2 10/10] hw/isa/isa-bus: Resolve isabus global
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (8 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 09/10] hw/isa: Remove use of global isa bus Bernhard Beschow
@ 2023-01-26 21:17 ` Bernhard Beschow
2023-01-30 17:05 ` [PATCH v2 00/10] " Bernhard Beschow
2023-02-24 16:22 ` Michael S. Tsirkin
11 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-26 21:17 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Bernhard Beschow
Now that only isa_bus_new() accesses the isabus global it can be removed
assuming that all call sites take care of not passing the same address
spaces to different isa_bus_new() invocations.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
hw/isa/isa-bus.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 8bae5cc473..b8c89a1e65 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -26,8 +26,6 @@
#include "hw/isa/isa.h"
#include "hw/acpi/acpi_aml_interface.h"
-static ISABus *isabus;
-
static char *isabus_get_fw_dev_path(DeviceState *dev);
static void isa_bus_class_init(ObjectClass *klass, void *data)
@@ -53,10 +51,8 @@ static const TypeInfo isa_bus_info = {
ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
MemoryRegion *address_space_io, Error **errp)
{
- if (isabus) {
- error_setg(errp, "Can't create a second ISA bus");
- return NULL;
- }
+ ISABus *isabus;
+
if (!dev) {
dev = qdev_new("isabus-bridge");
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
--
2.39.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v2 00/10] Resolve isabus global
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (9 preceding siblings ...)
2023-01-26 21:17 ` [PATCH v2 10/10] hw/isa/isa-bus: Resolve isabus global Bernhard Beschow
@ 2023-01-30 17:05 ` Bernhard Beschow
2023-02-24 16:22 ` Michael S. Tsirkin
11 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-01-30 17:05 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Hervé Poussineau, Gerd Hoffmann,
Michael S. Tsirkin, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini, Mark Cave-Ayland
+ Mark whom I've discussed "multiple ISA buses" before
Am 26. Januar 2023 21:17:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This series resolves the global "isabus" variable and is basically a v2 of [1].
>
>Note that the majority of the work consists of fixing ISA API calls in PIIX IDE
>
>which implicitly rely on the usage of the isabus global.
>
>
>
>Rather than adding an ISABus pointer in PCIIDEState as in [1] this series uses
>
>a qemu_irq array which is roughly the approach outlined in [2]. Moreover, this
>
>series considers backwards compatibility for user-created PIIX IDE
>
>"Frankensten" devices by fishing out TYPE_ISA_BUS from the QOM tree inside
>
>the PIIX IDE device model. This hack can be removed again once a deprecation
>
>period of user-createable PIIX IDE devices is over. This deprecation wasn't
>
>announced yet but now might be a good time.
>
>
>
>This series is structured as follows: The first three patches massage the ISA
>
>code for patch 8. Patches 4-8 change the PIIX IDE device models to not use the
>
>isabus global implicitly. Finally, the last two patches clan up and remove the
>
>isabus singleton.
>
>
>
>Based-on: <20230109172347.1830-1-shentey@gmail.com>
>
> 'Consolidate PIIX south bridges'
>
>
>
>v2:
>
>- Big rework sticking closer to [1], giving it more credit and reusing one patch
>
>- Add io port cleanup
>
>- Rebase onto [4] so changes to PIIX could be done once and centrally
>
>
>
>Testing done:
>
>* `make check`
>
>* `./qemu-system-x86_64 -M x-remote -device piix3-ide` still fails gracefully with
>
> `qemu-system-x86_64: -device piix3-ide: No ISA bus found while piix3-ide requires one`
>
>* `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.3.2-220704-linux515.iso`
>
>* `qemu-system-x86_64 -M q35 -m 2G -device piix4-ide -cdrom \
>
> manjaro-kde-21.3.2-220704-linux515.iso`
>
>* `qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda \
>
> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"`
>
>
>
>[1] https://patchew.org/QEMU/20210518215545.1793947-1-philmd@redhat.com/
>
>[2] https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html
>
>[3] https://people.debian.org/~aurel32/qemu/mips/
>
>[4] https://patchew.org/QEMU/20230109172347.1830-1-shentey@gmail.com/
>
>
>
>Bernhard Beschow (9):
>
> softmmu/ioport: Move portio_list_init() in front of portio_list_add()
>
> softmmu/ioport: Merge portio_list_add() into portio_list_init()
>
> softmmu/ioport: Remove unused functions
>
> hw/ide/piix: Disuse isa_get_irq()
>
> Revert "hw/ide: Fix crash when plugging a piix3-ide device into the
>
> x-remote machine"
>
> hw/ide/pci: Add PCIIDEState::isa_irqs[]
>
> hw/ide/piix: Require an ISABus only for user-created instances
>
> hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of
>
> ISADevice
>
> hw/isa/isa-bus: Resolve isabus global
>
>
>
>Philippe Mathieu-Daudé (1):
>
> hw/isa: Remove use of global isa bus
>
>
>
> include/exec/ioport.h | 8 ++---
>
> include/hw/ide/internal.h | 3 +-
>
> include/hw/ide/pci.h | 2 ++
>
> include/hw/isa/isa.h | 15 ++++----
>
> hw/audio/adlib.c | 4 +--
>
> hw/display/qxl.c | 5 ++-
>
> hw/display/vga.c | 8 ++---
>
> hw/dma/i82374.c | 6 ++--
>
> hw/ide/ioport.c | 19 +++++-----
>
> hw/ide/isa.c | 4 ++-
>
> hw/ide/piix.c | 75 +++++++++++++++++++++++++++++++--------
>
> hw/isa/isa-bus.c | 54 +++++++++++++++-------------
>
> hw/isa/piix.c | 5 +++
>
> hw/watchdog/wdt_ib700.c | 4 +--
>
> softmmu/ioport.c | 70 +++++++++++-------------------------
>
> 15 files changed, 149 insertions(+), 133 deletions(-)
>
>
>
>-- >
>2.39.1
>
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 00/10] Resolve isabus global
2023-01-26 21:17 [PATCH v2 00/10] Resolve isabus global Bernhard Beschow
` (10 preceding siblings ...)
2023-01-30 17:05 ` [PATCH v2 00/10] " Bernhard Beschow
@ 2023-02-24 16:22 ` Michael S. Tsirkin
2023-02-26 20:38 ` Bernhard Beschow
11 siblings, 1 reply; 40+ messages in thread
From: Michael S. Tsirkin @ 2023-02-24 16:22 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
On Thu, Jan 26, 2023 at 10:17:30PM +0100, Bernhard Beschow wrote:
> This series resolves the global "isabus" variable and is basically a v2 of [1].
> Note that the majority of the work consists of fixing ISA API calls in PIIX IDE
> which implicitly rely on the usage of the isabus global.
>
> Rather than adding an ISABus pointer in PCIIDEState as in [1] this series uses
> a qemu_irq array which is roughly the approach outlined in [2]. Moreover, this
> series considers backwards compatibility for user-created PIIX IDE
> "Frankensten" devices by fishing out TYPE_ISA_BUS from the QOM tree inside
> the PIIX IDE device model. This hack can be removed again once a deprecation
> period of user-createable PIIX IDE devices is over. This deprecation wasn't
> announced yet but now might be a good time.
>
> This series is structured as follows: The first three patches massage the ISA
> code for patch 8. Patches 4-8 change the PIIX IDE device models to not use the
> isabus global implicitly. Finally, the last two patches clan up and remove the
> isabus singleton.
I expect there will be a v3 of this, right?
> Based-on: <20230109172347.1830-1-shentey@gmail.com>
> 'Consolidate PIIX south bridges'
>
> v2:
> - Big rework sticking closer to [1], giving it more credit and reusing one patch
> - Add io port cleanup
> - Rebase onto [4] so changes to PIIX could be done once and centrally
>
> Testing done:
> * `make check`
> * `./qemu-system-x86_64 -M x-remote -device piix3-ide` still fails gracefully with
> `qemu-system-x86_64: -device piix3-ide: No ISA bus found while piix3-ide requires one`
> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.3.2-220704-linux515.iso`
> * `qemu-system-x86_64 -M q35 -m 2G -device piix4-ide -cdrom \
> manjaro-kde-21.3.2-220704-linux515.iso`
> * `qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda \
> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"`
>
> [1] https://patchew.org/QEMU/20210518215545.1793947-1-philmd@redhat.com/
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html
> [3] https://people.debian.org/~aurel32/qemu/mips/
> [4] https://patchew.org/QEMU/20230109172347.1830-1-shentey@gmail.com/
>
> Bernhard Beschow (9):
> softmmu/ioport: Move portio_list_init() in front of portio_list_add()
> softmmu/ioport: Merge portio_list_add() into portio_list_init()
> softmmu/ioport: Remove unused functions
> hw/ide/piix: Disuse isa_get_irq()
> Revert "hw/ide: Fix crash when plugging a piix3-ide device into the
> x-remote machine"
> hw/ide/pci: Add PCIIDEState::isa_irqs[]
> hw/ide/piix: Require an ISABus only for user-created instances
> hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of
> ISADevice
> hw/isa/isa-bus: Resolve isabus global
>
> Philippe Mathieu-Daudé (1):
> hw/isa: Remove use of global isa bus
>
> include/exec/ioport.h | 8 ++---
> include/hw/ide/internal.h | 3 +-
> include/hw/ide/pci.h | 2 ++
> include/hw/isa/isa.h | 15 ++++----
> hw/audio/adlib.c | 4 +--
> hw/display/qxl.c | 5 ++-
> hw/display/vga.c | 8 ++---
> hw/dma/i82374.c | 6 ++--
> hw/ide/ioport.c | 19 +++++-----
> hw/ide/isa.c | 4 ++-
> hw/ide/piix.c | 75 +++++++++++++++++++++++++++++++--------
> hw/isa/isa-bus.c | 54 +++++++++++++++-------------
> hw/isa/piix.c | 5 +++
> hw/watchdog/wdt_ib700.c | 4 +--
> softmmu/ioport.c | 70 +++++++++++-------------------------
> 15 files changed, 149 insertions(+), 133 deletions(-)
>
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 00/10] Resolve isabus global
2023-02-24 16:22 ` Michael S. Tsirkin
@ 2023-02-26 20:38 ` Bernhard Beschow
2023-02-27 9:12 ` Bernhard Beschow
0 siblings, 1 reply; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-26 20:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
Am 24. Februar 2023 16:22:48 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Thu, Jan 26, 2023 at 10:17:30PM +0100, Bernhard Beschow wrote:
>> This series resolves the global "isabus" variable and is basically a v2 of [1].
>> Note that the majority of the work consists of fixing ISA API calls in PIIX IDE
>> which implicitly rely on the usage of the isabus global.
>>
>> Rather than adding an ISABus pointer in PCIIDEState as in [1] this series uses
>> a qemu_irq array which is roughly the approach outlined in [2]. Moreover, this
>> series considers backwards compatibility for user-created PIIX IDE
>> "Frankensten" devices by fishing out TYPE_ISA_BUS from the QOM tree inside
>> the PIIX IDE device model. This hack can be removed again once a deprecation
>> period of user-createable PIIX IDE devices is over. This deprecation wasn't
>> announced yet but now might be a good time.
>>
>> This series is structured as follows: The first three patches massage the ISA
>> code for patch 8. Patches 4-8 change the PIIX IDE device models to not use the
>> isabus global implicitly. Finally, the last two patches clan up and remove the
>> isabus singleton.
>
>I expect there will be a v3 of this, right?
I would do it and I could rebase onto master if necessary. However, another series unrelated to this one but doing essentially the same thing with less backwards promises went through various iterations in the meantime. I don't know what the plan is.
Best regards,
Bernhard
>
>
>> Based-on: <20230109172347.1830-1-shentey@gmail.com>
>> 'Consolidate PIIX south bridges'
>>
>> v2:
>> - Big rework sticking closer to [1], giving it more credit and reusing one patch
>> - Add io port cleanup
>> - Rebase onto [4] so changes to PIIX could be done once and centrally
>>
>> Testing done:
>> * `make check`
>> * `./qemu-system-x86_64 -M x-remote -device piix3-ide` still fails gracefully with
>> `qemu-system-x86_64: -device piix3-ide: No ISA bus found while piix3-ide requires one`
>> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.3.2-220704-linux515.iso`
>> * `qemu-system-x86_64 -M q35 -m 2G -device piix4-ide -cdrom \
>> manjaro-kde-21.3.2-220704-linux515.iso`
>> * `qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda \
>> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"`
>>
>> [1] https://patchew.org/QEMU/20210518215545.1793947-1-philmd@redhat.com/
>> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html
>> [3] https://people.debian.org/~aurel32/qemu/mips/
>> [4] https://patchew.org/QEMU/20230109172347.1830-1-shentey@gmail.com/
>>
>> Bernhard Beschow (9):
>> softmmu/ioport: Move portio_list_init() in front of portio_list_add()
>> softmmu/ioport: Merge portio_list_add() into portio_list_init()
>> softmmu/ioport: Remove unused functions
>> hw/ide/piix: Disuse isa_get_irq()
>> Revert "hw/ide: Fix crash when plugging a piix3-ide device into the
>> x-remote machine"
>> hw/ide/pci: Add PCIIDEState::isa_irqs[]
>> hw/ide/piix: Require an ISABus only for user-created instances
>> hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of
>> ISADevice
>> hw/isa/isa-bus: Resolve isabus global
>>
>> Philippe Mathieu-Daudé (1):
>> hw/isa: Remove use of global isa bus
>>
>> include/exec/ioport.h | 8 ++---
>> include/hw/ide/internal.h | 3 +-
>> include/hw/ide/pci.h | 2 ++
>> include/hw/isa/isa.h | 15 ++++----
>> hw/audio/adlib.c | 4 +--
>> hw/display/qxl.c | 5 ++-
>> hw/display/vga.c | 8 ++---
>> hw/dma/i82374.c | 6 ++--
>> hw/ide/ioport.c | 19 +++++-----
>> hw/ide/isa.c | 4 ++-
>> hw/ide/piix.c | 75 +++++++++++++++++++++++++++++++--------
>> hw/isa/isa-bus.c | 54 +++++++++++++++-------------
>> hw/isa/piix.c | 5 +++
>> hw/watchdog/wdt_ib700.c | 4 +--
>> softmmu/ioport.c | 70 +++++++++++-------------------------
>> 15 files changed, 149 insertions(+), 133 deletions(-)
>>
>> --
>> 2.39.1
>>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 00/10] Resolve isabus global
2023-02-26 20:38 ` Bernhard Beschow
@ 2023-02-27 9:12 ` Bernhard Beschow
0 siblings, 0 replies; 40+ messages in thread
From: Bernhard Beschow @ 2023-02-27 9:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Marcel Apfelbaum, Hervé Poussineau,
Gerd Hoffmann, Aurelien Jarno, Philippe Mathieu-Daudé,
David Hildenbrand, Peter Xu, qemu-ppc, qemu-block, John Snow,
Paolo Bonzini
Am 26. Februar 2023 20:38:24 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 24. Februar 2023 16:22:48 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>>On Thu, Jan 26, 2023 at 10:17:30PM +0100, Bernhard Beschow wrote:
>>> This series resolves the global "isabus" variable and is basically a v2 of [1].
>>> Note that the majority of the work consists of fixing ISA API calls in PIIX IDE
>>> which implicitly rely on the usage of the isabus global.
>>>
>>> Rather than adding an ISABus pointer in PCIIDEState as in [1] this series uses
>>> a qemu_irq array which is roughly the approach outlined in [2]. Moreover, this
>>> series considers backwards compatibility for user-created PIIX IDE
>>> "Frankensten" devices by fishing out TYPE_ISA_BUS from the QOM tree inside
>>> the PIIX IDE device model. This hack can be removed again once a deprecation
>>> period of user-createable PIIX IDE devices is over. This deprecation wasn't
>>> announced yet but now might be a good time.
>>>
>>> This series is structured as follows: The first three patches massage the ISA
>>> code for patch 8. Patches 4-8 change the PIIX IDE device models to not use the
>>> isabus global implicitly. Finally, the last two patches clan up and remove the
>>> isabus singleton.
>>
>>I expect there will be a v3 of this, right?
>
>I would do it and I could rebase onto master if necessary. However, another series unrelated to this one but doing essentially the same thing with less backwards promises went through various iterations in the meantime. I don't know what the plan is.
Phil, how shall we proceed? What's holding back the PIIX consolidation? It was fully reviewed at the beginning of the development window. Now the freeze is just a couple of days ahead.
Best regards,
Bernhard
>
>Best regards,
>Bernhard
>
>>
>>
>>> Based-on: <20230109172347.1830-1-shentey@gmail.com>
>>> 'Consolidate PIIX south bridges'
>>>
>>> v2:
>>> - Big rework sticking closer to [1], giving it more credit and reusing one patch
>>> - Add io port cleanup
>>> - Rebase onto [4] so changes to PIIX could be done once and centrally
>>>
>>> Testing done:
>>> * `make check`
>>> * `./qemu-system-x86_64 -M x-remote -device piix3-ide` still fails gracefully with
>>> `qemu-system-x86_64: -device piix3-ide: No ISA bus found while piix3-ide requires one`
>>> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.3.2-220704-linux515.iso`
>>> * `qemu-system-x86_64 -M q35 -m 2G -device piix4-ide -cdrom \
>>> manjaro-kde-21.3.2-220704-linux515.iso`
>>> * `qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda \
>>> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"`
>>>
>>> [1] https://patchew.org/QEMU/20210518215545.1793947-1-philmd@redhat.com/
>>> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html
>>> [3] https://people.debian.org/~aurel32/qemu/mips/
>>> [4] https://patchew.org/QEMU/20230109172347.1830-1-shentey@gmail.com/
>>>
>>> Bernhard Beschow (9):
>>> softmmu/ioport: Move portio_list_init() in front of portio_list_add()
>>> softmmu/ioport: Merge portio_list_add() into portio_list_init()
>>> softmmu/ioport: Remove unused functions
>>> hw/ide/piix: Disuse isa_get_irq()
>>> Revert "hw/ide: Fix crash when plugging a piix3-ide device into the
>>> x-remote machine"
>>> hw/ide/pci: Add PCIIDEState::isa_irqs[]
>>> hw/ide/piix: Require an ISABus only for user-created instances
>>> hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of
>>> ISADevice
>>> hw/isa/isa-bus: Resolve isabus global
>>>
>>> Philippe Mathieu-Daudé (1):
>>> hw/isa: Remove use of global isa bus
>>>
>>> include/exec/ioport.h | 8 ++---
>>> include/hw/ide/internal.h | 3 +-
>>> include/hw/ide/pci.h | 2 ++
>>> include/hw/isa/isa.h | 15 ++++----
>>> hw/audio/adlib.c | 4 +--
>>> hw/display/qxl.c | 5 ++-
>>> hw/display/vga.c | 8 ++---
>>> hw/dma/i82374.c | 6 ++--
>>> hw/ide/ioport.c | 19 +++++-----
>>> hw/ide/isa.c | 4 ++-
>>> hw/ide/piix.c | 75 +++++++++++++++++++++++++++++++--------
>>> hw/isa/isa-bus.c | 54 +++++++++++++++-------------
>>> hw/isa/piix.c | 5 +++
>>> hw/watchdog/wdt_ib700.c | 4 +--
>>> softmmu/ioport.c | 70 +++++++++++-------------------------
>>> 15 files changed, 149 insertions(+), 133 deletions(-)
>>>
>>> --
>>> 2.39.1
>>>
>>
^ permalink raw reply [flat|nested] 40+ messages in thread