[PATCH 0/6] QOM'ify PIIX southbridge creation

Bernhard Beschow posted 6 patches 1 year, 11 months ago
There is a newer version of this series
hw/i386/pc_piix.c             |  7 ++-
hw/isa/piix3.c                | 98 +++++++++++++++++++----------------
hw/isa/piix4.c                | 91 +++++++++++++-------------------
hw/mips/malta.c               |  9 +++-
include/hw/isa/isa.h          |  2 -
include/hw/southbridge/piix.h |  6 +--
6 files changed, 105 insertions(+), 108 deletions(-)
[PATCH 0/6] QOM'ify PIIX southbridge creation
Posted by Bernhard Beschow 1 year, 11 months ago
The piix3 and piix4 southbridge devices still rely on create() functions which
are deprecated. This series resolves these functions piece by piece to
modernize the code.

Both devices are modified in lockstep where possible to provide more context.

Testing done:
* `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
* `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`

In both cases the system booted successfully and it was possible to shut down
the system using the `poweroff` command.

Bernhard Beschow (6):
  include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
  hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
  hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
  hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
  hw/isa/piix4: Factor out SM bus initialization from create() function
  hw/isa/piix{3,4}: Inline and remove create() functions

 hw/i386/pc_piix.c             |  7 ++-
 hw/isa/piix3.c                | 98 +++++++++++++++++++----------------
 hw/isa/piix4.c                | 91 +++++++++++++-------------------
 hw/mips/malta.c               |  9 +++-
 include/hw/isa/isa.h          |  2 -
 include/hw/southbridge/piix.h |  6 +--
 6 files changed, 105 insertions(+), 108 deletions(-)

-- 
2.36.1
Re: [PATCH 0/6] QOM'ify PIIX southbridge creation
Posted by Mark Cave-Ayland 1 year, 11 months ago
On 13/05/2022 18:54, Bernhard Beschow wrote:

> The piix3 and piix4 southbridge devices still rely on create() functions which
> are deprecated. This series resolves these functions piece by piece to
> modernize the code.
> 
> Both devices are modified in lockstep where possible to provide more context.
> 
> Testing done:
> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso`
> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"`
> 
> In both cases the system booted successfully and it was possible to shut down
> the system using the `poweroff` command.
> 
> Bernhard Beschow (6):
>    include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
>    hw/isa/piix4: Factor out SM bus initialization from create() function
>    hw/isa/piix{3,4}: Inline and remove create() functions
> 
>   hw/i386/pc_piix.c             |  7 ++-
>   hw/isa/piix3.c                | 98 +++++++++++++++++++----------------
>   hw/isa/piix4.c                | 91 +++++++++++++-------------------
>   hw/mips/malta.c               |  9 +++-
>   include/hw/isa/isa.h          |  2 -
>   include/hw/southbridge/piix.h |  6 +--
>   6 files changed, 105 insertions(+), 108 deletions(-)

I've just reviewed these, and other than a couple of minor issues these look good to 
me and definitely help to improve the code.

One thing reading over this patches has made me realise is that there is quite a 
model violation here in that the PIIX3 and PIIX4 devices (which are the PCI-ISA 
bridges) are actually setting the PCI host bridge IRQs(!). What should happen is that 
the PCI bus IRQs and routing should be done in the PCI host bridge, and gpios used in 
the PCI-ISA bridges to pass them up. But that's definitely something outside the 
scope of these improvements.


ATB,

Mark.
Re: [PATCH 0/6] QOM'ify PIIX southbridge creation
Posted by Philippe Mathieu-Daudé via 1 year, 11 months ago
On 21/5/22 10:48, Mark Cave-Ayland wrote:
> On 13/05/2022 18:54, Bernhard Beschow wrote:

>> Bernhard Beschow (6):
>>    include/hw: Move TYPE_PIIX4_PCI_DEVICE to southbridge/piix.h
>>    hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's
>>    hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
>>    hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions
>>    hw/isa/piix4: Factor out SM bus initialization from create() function
>>    hw/isa/piix{3,4}: Inline and remove create() functions

> I've just reviewed these, and other than a couple of minor issues these 
> look good to me and definitely help to improve the code.
> 
> One thing reading over this patches has made me realise is that there is 
> quite a model violation here in that the PIIX3 and PIIX4 devices (which 
> are the PCI-ISA bridges) are actually setting the PCI host bridge 
> IRQs(!). What should happen is that the PCI bus IRQs and routing should 
> be done in the PCI host bridge, and gpios used in the PCI-ISA bridges to 
> pass them up. But that's definitely something outside the scope of these 
> improvements.

Yes. Do you mind opening a GitLab issue?