| Message ID | 20220522212431.14598-2-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOM'ify PIIX southbridge creation | expand |
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. Regards, BALATON Zoltan > + > PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus); > > DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus); >
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. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> + >> PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus); >> >> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus); >>
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.
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" + PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus); DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);