Re: [PATCH v2 1/6] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Mark Cave-Ayland]

On 29/05/2022 10:23, Bernhard Beschow wrote:

> Am 22. Mai 2022 22:32:06 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Sun, 22 May 2022, Bernhard Beschow wrote:
> > > TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
> > > ones, too.
> > >
> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > > hw/isa/piix3.c                | 3 ---
> > > include/hw/isa/isa.h          | 2 --
> > > include/hw/southbridge/piix.h | 4 ++++
> > > 3 files changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> > > index dab901c9ad..d96ce2b788 100644
> > > --- a/hw/isa/piix3.c
> > > +++ b/hw/isa/piix3.c
> > > @@ -35,9 +35,6 @@
> > >
> > > #define XEN_PIIX_NUM_PIRQS      128ULL
> > >
> > > -#define TYPE_PIIX3_DEVICE "PIIX3"
> > > -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> > > -
> > > static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
> > > {
> > >      qemu_set_irq(piix3->pic[pic_irq],
> > > diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> > > index 034d706ba1..e9fa2f5cea 100644
> > > --- a/include/hw/isa/isa.h
> > > +++ b/include/hw/isa/isa.h
> > > @@ -144,6 +144,4 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
> > >      return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
> > > }
> > >
> > > -#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> > > -
> > > #endif
> > > diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> > > index f63f83e5c6..4d17400dfd 100644
> > > --- a/include/hw/southbridge/piix.h
> > > +++ b/include/hw/southbridge/piix.h
> > > @@ -70,6 +70,10 @@ typedef struct PIIXState PIIX3State;
> > > DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> > >                           TYPE_PIIX3_PCI_DEVICE)
> > >
> > > +#define TYPE_PIIX3_DEVICE "PIIX3"
> > > +#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> > > +#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> >
> > This naming seems to go back to 9b74b190d6c so it's not directly related to 
> > this series but there seems to be a bit of a confusion here that I wonder could 
> > be cleaned up now. We have:
> >
> > #define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
> > and
> > #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> >
> > and both of these have mismatched #define and device name. These are not user 
> > creatable so so the device names don't appear anywhere (except maybe in info 
> > qtree output) but this could still be confusing as normally type defines should 
> > match device names. If these are the ISA bridges then maybe piix?-isa is a good 
> > name so maybe we should have
> >
> > #define TYPE_PIIX3_ISA "piix3-isa"
> > #defint TYPE_PIIX4_ISA "piix4-isa"
> >
> > (then also matching e.g. "via-isa") but I'm not sure changing "pci-piix3" to 
> > "piix3-isa" could break anything (I don't expect changing the defines themselces could cause any 
> > problem).
> >
> > It's just something I've noticed and brought up for consideration but I have no 
> > strong opinion on it so up to you what you do with it.
>
> Hi Zoltan,
>
> yeah, I noticed the naming when moving the defines. I couldn't come up
> with better names which were worth the effort because I can imagine
> where the names are coming from. I also didn't want to go down the
> rabbithole of backwards compatibility (which is a topic I haven't
> covered yet). I think that *-isa is too narrow a name for the
> container device since it bridges a couple of buses to PCI, but other
> than that I don't have strong opinions about the names.

Agreed. They certainly aren't the best of names, but my worry would be that someone 
has done some magic with -global to override the defaults and so renaming them would 
cause something to break.

Certainly this could be discussed as a separate topic with the PC machine maintainers 
if someone feels strongly about it, but I wouldn't want this to hold up the patch 
series unnecessarily.


ATB,

Mark.