| Message ID | 20230422150728.176512-12-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up PCI IDE device models | expand |
On Sat, 22 Apr 2023, Bernhard Beschow wrote: > Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a > standard-compliant PCI IDE device. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 2 -- > hw/ide/pci.c | 4 ++-- > hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- > 3 files changed, 20 insertions(+), 36 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 5025df5b82..dbb4b13161 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); > extern MemoryRegionOps bmdma_addr_ioport_ops; > void pci_ide_create_devs(PCIDevice *dev); > > -extern const MemoryRegionOps pci_ide_cmd_le_ops; > -extern const MemoryRegionOps pci_ide_data_le_ops; > #endif > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index b2fcc00a64..97ccc75aa6 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, > ide_ctrl_write(bus, addr + 2, data); > } > > -const MemoryRegionOps pci_ide_cmd_le_ops = { > +static const MemoryRegionOps pci_ide_cmd_le_ops = { > .read = pci_ide_status_read, > .write = pci_ide_ctrl_write, > .endianness = DEVICE_LITTLE_ENDIAN, > @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, > } > } > > -const MemoryRegionOps pci_ide_data_le_ops = { > +static const MemoryRegionOps pci_ide_data_le_ops = { > .read = pci_ide_data_read, > .write = pci_ide_data_write, > .endianness = DEVICE_LITTLE_ENDIAN, > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 0af897a9ef..9cf920369f 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, > val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); > val |= (uint32_t)d->i.bmdma[1].status << 16; > break; > - case 0x80 ... 0x87: > - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); > - break; > - case 0x8a: > - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); > - break; > case 0xa0: > val = d->regs[0].confstat; > break; > - case 0xc0 ... 0xc7: > - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); > - break; > - case 0xca: > - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); > - break; > case 0xe0: > val = d->regs[1].confstat; > break; > @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > case 0x0c ... 0x0f: > bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); > break; > - case 0x80 ... 0x87: > - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); > - break; > - case 0x8a: > - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); > - break; > - case 0xc0 ... 0xc7: > - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); > - break; > - case 0xca: > - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); > - break; > case 0x100: > d->regs[0].scontrol = val & 0xfff; > if (val & 1) { > @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) > pci_config_set_interrupt_pin(dev->config, 1); > pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); > > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); > + > /* BAR5 is in PCI memory space */ > memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, > "sii3112.bar5", 0x200); > @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) > > /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ This patch breaks the above comment but I think you should not mess with BAR0-4 at all and leave to to aliased into BAR5. These have the same registers mirrored and some guests access them via the memory mapped BAR5 while others prefer the io mapped BAR0-4 so removing these from BAR5 would break some guests. If you want to remove something from BAR5 and map subregions implementing those instead then I think only BAR5 needs to be chnaged or I'm not getting what is happening here so a more detailed commit message would be needed. Was this tested? A minimal test might be booting AROS and MorphOS on sam460ex. Regards, BALATON Zoltan > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, > + memory_region_size(&s->data_ops[0])); > + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, > + memory_region_size(&s->cmd_ops[0])); > + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, > + memory_region_size(&s->data_ops[1])); > + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, > + memory_region_size(&s->cmd_ops[1])); > + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); > + > mr = g_new(MemoryRegion, 1); > memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >
Am 22. April 2023 21:10:14 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 22 Apr 2023, Bernhard Beschow wrote: >> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >> standard-compliant PCI IDE device. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 2 -- >> hw/ide/pci.c | 4 ++-- >> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >> 3 files changed, 20 insertions(+), 36 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index 5025df5b82..dbb4b13161 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >> extern MemoryRegionOps bmdma_addr_ioport_ops; >> void pci_ide_create_devs(PCIDevice *dev); >> >> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >> -extern const MemoryRegionOps pci_ide_data_le_ops; >> #endif >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index b2fcc00a64..97ccc75aa6 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >> ide_ctrl_write(bus, addr + 2, data); >> } >> >> -const MemoryRegionOps pci_ide_cmd_le_ops = { >> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >> .read = pci_ide_status_read, >> .write = pci_ide_ctrl_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >> } >> } >> >> -const MemoryRegionOps pci_ide_data_le_ops = { >> +static const MemoryRegionOps pci_ide_data_le_ops = { >> .read = pci_ide_data_read, >> .write = pci_ide_data_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 0af897a9ef..9cf920369f 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >> val |= (uint32_t)d->i.bmdma[1].status << 16; >> break; >> - case 0x80 ... 0x87: >> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >> - break; >> - case 0x8a: >> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >> - break; >> case 0xa0: >> val = d->regs[0].confstat; >> break; >> - case 0xc0 ... 0xc7: >> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >> - break; >> - case 0xca: >> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >> - break; >> case 0xe0: >> val = d->regs[1].confstat; >> break; >> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> case 0x0c ... 0x0f: >> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >> break; >> - case 0x80 ... 0x87: >> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >> - break; >> - case 0x8a: >> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >> - break; >> - case 0xc0 ... 0xc7: >> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >> - break; >> - case 0xca: >> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >> - break; >> case 0x100: >> d->regs[0].scontrol = val & 0xfff; >> if (val & 1) { >> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >> pci_config_set_interrupt_pin(dev->config, 1); >> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >> >> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >> + >> /* BAR5 is in PCI memory space */ >> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >> "sii3112.bar5", 0x200); >> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >> >> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ > >This patch breaks the above comment Indeed. It's now the other way around. >but I think you should not mess with BAR0-4 at all and leave to to aliased into BAR5. These have the same registers mirrored and some guests access them via the memory mapped BAR5 while others prefer the io mapped BAR0-4 so removing these from BAR5 would break some guests. BARs 0-3 are the PCI-native BARs and BAR4 is the BMDMA BAR which are mapped by via and cmd646 already since they support these modes. SIL3112 supports these modes as well but had custom implementations so far while ignoring the attributes of the parent class. Now that the parent class already initializes these attributes we can just reuse them here which in addition makes it very obvious that SIL3112 supports these modes. I'll split this patch and the next one to (hopefully) make more visible what happens. > If you want to remove something from BAR5 and map subregions implementing those instead then I think only BAR5 needs to be chnaged or I'm not getting what is happening here so a more detailed commit message would be needed. Agreed. I'll put wording similar to above into the commit message. > >Was this tested? A minimal test might be booting AROS and MorphOS on sam460ex. I tested with MorphOS on sam460ex. The second ppc test case in the cover letter was actually supposed to show this. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >> + memory_region_size(&s->data_ops[0])); >> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >> + memory_region_size(&s->cmd_ops[0])); >> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >> + memory_region_size(&s->data_ops[1])); >> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >> + memory_region_size(&s->cmd_ops[1])); >> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >> + >> mr = g_new(MemoryRegion, 1); >> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >>
On Sun, 23 Apr 2023, Bernhard Beschow wrote: > Am 22. April 2023 21:10:14 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 22 Apr 2023, Bernhard Beschow wrote: >>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >>> standard-compliant PCI IDE device. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> include/hw/ide/pci.h | 2 -- >>> hw/ide/pci.c | 4 ++-- >>> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >>> 3 files changed, 20 insertions(+), 36 deletions(-) >>> >>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>> index 5025df5b82..dbb4b13161 100644 >>> --- a/include/hw/ide/pci.h >>> +++ b/include/hw/ide/pci.h >>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >>> extern MemoryRegionOps bmdma_addr_ioport_ops; >>> void pci_ide_create_devs(PCIDevice *dev); >>> >>> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >>> -extern const MemoryRegionOps pci_ide_data_le_ops; >>> #endif >>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>> index b2fcc00a64..97ccc75aa6 100644 >>> --- a/hw/ide/pci.c >>> +++ b/hw/ide/pci.c >>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >>> ide_ctrl_write(bus, addr + 2, data); >>> } >>> >>> -const MemoryRegionOps pci_ide_cmd_le_ops = { >>> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >>> .read = pci_ide_status_read, >>> .write = pci_ide_ctrl_write, >>> .endianness = DEVICE_LITTLE_ENDIAN, >>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >>> } >>> } >>> >>> -const MemoryRegionOps pci_ide_data_le_ops = { >>> +static const MemoryRegionOps pci_ide_data_le_ops = { >>> .read = pci_ide_data_read, >>> .write = pci_ide_data_write, >>> .endianness = DEVICE_LITTLE_ENDIAN, >>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>> index 0af897a9ef..9cf920369f 100644 >>> --- a/hw/ide/sii3112.c >>> +++ b/hw/ide/sii3112.c >>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >>> val |= (uint32_t)d->i.bmdma[1].status << 16; >>> break; >>> - case 0x80 ... 0x87: >>> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >>> - break; >>> - case 0x8a: >>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >>> - break; >>> case 0xa0: >>> val = d->regs[0].confstat; >>> break; >>> - case 0xc0 ... 0xc7: >>> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >>> - break; >>> - case 0xca: >>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >>> - break; >>> case 0xe0: >>> val = d->regs[1].confstat; >>> break; >>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>> case 0x0c ... 0x0f: >>> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >>> break; >>> - case 0x80 ... 0x87: >>> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >>> - break; >>> - case 0x8a: >>> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >>> - break; >>> - case 0xc0 ... 0xc7: >>> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >>> - break; >>> - case 0xca: >>> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >>> - break; >>> case 0x100: >>> d->regs[0].scontrol = val & 0xfff; >>> if (val & 1) { >>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>> pci_config_set_interrupt_pin(dev->config, 1); >>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >>> >>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >>> + >>> /* BAR5 is in PCI memory space */ >>> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >>> "sii3112.bar5", 0x200); >>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>> >>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >> >> This patch breaks the above comment > > Indeed. It's now the other way around. OK, then adjust comments as well, also the other one about BAR5 at the top which may not be true any more if you remove stuff from BAR5 and alias the other BARs instead. The idea here was to follow the data sheet which documents the memory space BAR5 and other io BARs are just io space aliases of parts of the memory mapped registers. Those are to support easily porting older drivers but other drivers may only map BAR5 where all the regs are available. >> but I think you should not mess with BAR0-4 at all and leave to to >> aliased into BAR5. These have the same registers mirrored and some >> guests access them via the memory mapped BAR5 while others prefer the >> io mapped BAR0-4 so removing these from BAR5 would break some guests. > > BARs 0-3 are the PCI-native BARs and BAR4 is the BMDMA BAR which are > mapped by via and cmd646 already since they support these modes. SIL3112 > supports these modes as well but had custom implementations so far while > ignoring the attributes of the parent class. Now that the parent class Which attributes? Do those make sense for a SATA controller or does the sii3112 have those in BAR5? I'll wait for an updated version to review this further as that may clear up some things. > already initializes these attributes we can just reuse them here which > in addition makes it very obvious that SIL3112 supports these modes. By the way it's called SiI3112 for Silicon Image but the upper case I is often misread as a lower case l as these look similar. Regards, BALATON Zoltan > I'll split this patch and the next one to (hopefully) make more visible what happens. > >> If you want to remove something from BAR5 and map subregions implementing those instead then I think only BAR5 needs to be chnaged or I'm not getting what is happening here so a more detailed commit message would be needed. > > Agreed. I'll put wording similar to above into the commit message. > >> >> Was this tested? A minimal test might be booting AROS and MorphOS on sam460ex. > > I tested with MorphOS on sam460ex. The second ppc test case in the cover letter was actually supposed to show this. > > Best regards, > Bernhard > >> >> Regards, >> BALATON Zoltan >> >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >>> + memory_region_size(&s->data_ops[0])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >>> + memory_region_size(&s->cmd_ops[0])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >>> + memory_region_size(&s->data_ops[1])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >>> + memory_region_size(&s->cmd_ops[1])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >>> + >>> mr = g_new(MemoryRegion, 1); >>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> > >
On 22/04/2023 16:07, Bernhard Beschow wrote: > Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a > standard-compliant PCI IDE device. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 2 -- > hw/ide/pci.c | 4 ++-- > hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- > 3 files changed, 20 insertions(+), 36 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 5025df5b82..dbb4b13161 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); > extern MemoryRegionOps bmdma_addr_ioport_ops; > void pci_ide_create_devs(PCIDevice *dev); > > -extern const MemoryRegionOps pci_ide_cmd_le_ops; > -extern const MemoryRegionOps pci_ide_data_le_ops; > #endif > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index b2fcc00a64..97ccc75aa6 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, > ide_ctrl_write(bus, addr + 2, data); > } > > -const MemoryRegionOps pci_ide_cmd_le_ops = { > +static const MemoryRegionOps pci_ide_cmd_le_ops = { > .read = pci_ide_status_read, > .write = pci_ide_ctrl_write, > .endianness = DEVICE_LITTLE_ENDIAN, > @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, > } > } > > -const MemoryRegionOps pci_ide_data_le_ops = { > +static const MemoryRegionOps pci_ide_data_le_ops = { > .read = pci_ide_data_read, > .write = pci_ide_data_write, > .endianness = DEVICE_LITTLE_ENDIAN, > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 0af897a9ef..9cf920369f 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, > val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); > val |= (uint32_t)d->i.bmdma[1].status << 16; > break; > - case 0x80 ... 0x87: > - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); > - break; > - case 0x8a: > - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); > - break; > case 0xa0: > val = d->regs[0].confstat; > break; > - case 0xc0 ... 0xc7: > - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); > - break; > - case 0xca: > - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); > - break; > case 0xe0: > val = d->regs[1].confstat; > break; > @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > case 0x0c ... 0x0f: > bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); > break; > - case 0x80 ... 0x87: > - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); > - break; > - case 0x8a: > - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); > - break; > - case 0xc0 ... 0xc7: > - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); > - break; > - case 0xca: > - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); > - break; > case 0x100: > d->regs[0].scontrol = val & 0xfff; > if (val & 1) { > @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) > pci_config_set_interrupt_pin(dev->config, 1); > pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); > > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); > + > /* BAR5 is in PCI memory space */ > memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, > "sii3112.bar5", 0x200); > @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) > > /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, > + memory_region_size(&s->data_ops[0])); > + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, > + memory_region_size(&s->cmd_ops[0])); > + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, > + memory_region_size(&s->data_ops[1])); > + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); > mr = g_new(MemoryRegion, 1); > - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); > + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, > + memory_region_size(&s->cmd_ops[1])); > + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); > + > mr = g_new(MemoryRegion, 1); > memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); So if I read this right, this is now switching the aliases over on BAR5 to allow re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's correct then I think the commit message needs a bit more detail, otherwise: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 22/04/2023 16:07, Bernhard Beschow wrote: > >> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >> standard-compliant PCI IDE device. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 2 -- >> hw/ide/pci.c | 4 ++-- >> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >> 3 files changed, 20 insertions(+), 36 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index 5025df5b82..dbb4b13161 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >> extern MemoryRegionOps bmdma_addr_ioport_ops; >> void pci_ide_create_devs(PCIDevice *dev); >> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >> -extern const MemoryRegionOps pci_ide_data_le_ops; >> #endif >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index b2fcc00a64..97ccc75aa6 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >> ide_ctrl_write(bus, addr + 2, data); >> } >> -const MemoryRegionOps pci_ide_cmd_le_ops = { >> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >> .read = pci_ide_status_read, >> .write = pci_ide_ctrl_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >> } >> } >> -const MemoryRegionOps pci_ide_data_le_ops = { >> +static const MemoryRegionOps pci_ide_data_le_ops = { >> .read = pci_ide_data_read, >> .write = pci_ide_data_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 0af897a9ef..9cf920369f 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >> val |= (uint32_t)d->i.bmdma[1].status << 16; >> break; >> - case 0x80 ... 0x87: >> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >> - break; >> - case 0x8a: >> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >> - break; >> case 0xa0: >> val = d->regs[0].confstat; >> break; >> - case 0xc0 ... 0xc7: >> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >> - break; >> - case 0xca: >> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >> - break; >> case 0xe0: >> val = d->regs[1].confstat; >> break; >> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> case 0x0c ... 0x0f: >> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >> break; >> - case 0x80 ... 0x87: >> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >> - break; >> - case 0x8a: >> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >> - break; >> - case 0xc0 ... 0xc7: >> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >> - break; >> - case 0xca: >> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >> - break; >> case 0x100: >> d->regs[0].scontrol = val & 0xfff; >> if (val & 1) { >> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >> pci_config_set_interrupt_pin(dev->config, 1); >> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >> + >> /* BAR5 is in PCI memory space */ >> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >> "sii3112.bar5", 0x200); >> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >> + memory_region_size(&s->data_ops[0])); >> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >> + memory_region_size(&s->cmd_ops[0])); >> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >> + memory_region_size(&s->data_ops[1])); >> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >> mr = g_new(MemoryRegion, 1); >> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >> + memory_region_size(&s->cmd_ops[1])); >> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >> + >> mr = g_new(MemoryRegion, 1); >> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); > >So if I read this right, this is now switching the aliases over on BAR5 to allow re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's correct then I think the commit message needs a bit more detail, otherwise: That's correct. Besides improving the commit message I'll additonally split this patch into two to show what's going on. Furthermore, I'd init the memory regions in sii3112's init method rather than in realize(). This will be more consistent with the other PCI IDE device models and with the other memory regions. Best regards, Bernhard > >Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > >ATB, > >Mark.
On Wed, 26 Apr 2023, Bernhard Beschow wrote: > Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >> On 22/04/2023 16:07, Bernhard Beschow wrote: >> >>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >>> standard-compliant PCI IDE device. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> include/hw/ide/pci.h | 2 -- >>> hw/ide/pci.c | 4 ++-- >>> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >>> 3 files changed, 20 insertions(+), 36 deletions(-) >>> >>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>> index 5025df5b82..dbb4b13161 100644 >>> --- a/include/hw/ide/pci.h >>> +++ b/include/hw/ide/pci.h >>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >>> extern MemoryRegionOps bmdma_addr_ioport_ops; >>> void pci_ide_create_devs(PCIDevice *dev); >>> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >>> -extern const MemoryRegionOps pci_ide_data_le_ops; >>> #endif >>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>> index b2fcc00a64..97ccc75aa6 100644 >>> --- a/hw/ide/pci.c >>> +++ b/hw/ide/pci.c >>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >>> ide_ctrl_write(bus, addr + 2, data); >>> } >>> -const MemoryRegionOps pci_ide_cmd_le_ops = { >>> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >>> .read = pci_ide_status_read, >>> .write = pci_ide_ctrl_write, >>> .endianness = DEVICE_LITTLE_ENDIAN, >>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >>> } >>> } >>> -const MemoryRegionOps pci_ide_data_le_ops = { >>> +static const MemoryRegionOps pci_ide_data_le_ops = { >>> .read = pci_ide_data_read, >>> .write = pci_ide_data_write, >>> .endianness = DEVICE_LITTLE_ENDIAN, >>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>> index 0af897a9ef..9cf920369f 100644 >>> --- a/hw/ide/sii3112.c >>> +++ b/hw/ide/sii3112.c >>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >>> val |= (uint32_t)d->i.bmdma[1].status << 16; >>> break; >>> - case 0x80 ... 0x87: >>> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >>> - break; >>> - case 0x8a: >>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >>> - break; >>> case 0xa0: >>> val = d->regs[0].confstat; >>> break; >>> - case 0xc0 ... 0xc7: >>> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >>> - break; >>> - case 0xca: >>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >>> - break; >>> case 0xe0: >>> val = d->regs[1].confstat; >>> break; >>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>> case 0x0c ... 0x0f: >>> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >>> break; >>> - case 0x80 ... 0x87: >>> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >>> - break; >>> - case 0x8a: >>> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >>> - break; >>> - case 0xc0 ... 0xc7: >>> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >>> - break; >>> - case 0xca: >>> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >>> - break; >>> case 0x100: >>> d->regs[0].scontrol = val & 0xfff; >>> if (val & 1) { >>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>> pci_config_set_interrupt_pin(dev->config, 1); >>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >>> + >>> /* BAR5 is in PCI memory space */ >>> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >>> "sii3112.bar5", 0x200); >>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >>> + memory_region_size(&s->data_ops[0])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >>> + memory_region_size(&s->cmd_ops[0])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >>> + memory_region_size(&s->data_ops[1])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >>> mr = g_new(MemoryRegion, 1); >>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >>> + memory_region_size(&s->cmd_ops[1])); >>> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >>> + >>> mr = g_new(MemoryRegion, 1); >>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >> >> So if I read this right, this is now switching the aliases over on BAR5 to allow re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's correct then I think the commit message needs a bit more detail, otherwise: > > That's correct. Besides improving the commit message I'll additonally split this patch into two to show what's going on. > > Furthermore, I'd init the memory regions in sii3112's init method rather > than in realize(). This will be more consistent with the other PCI IDE > device models and with the other memory regions. Why is an init method meeded? If it's not needed why add it? Just keep it simple. My view on these methods is that usually only a realize method is needed which is the normal constructor of the object, but sometimes we need some properties or something else to be available that can configure the object before realising which is when an init method is needed. Sometimes QEMU creates an object then destroys it without realising (I think e.g. when using help to query device properties) so then memory regions would be created and destroyed pointlessly. So unless there's a good reason to have an init method I'd leave this device without one to keep it simple. That other devices have an init method does not explain why this one should have one. Maybe those devices need it for some properties or they are just old code and not properly QOM'ified. Regards, BALATON Zoltan
On 27/04/2023 00:24, BALATON Zoltan wrote: > On Wed, 26 Apr 2023, Bernhard Beschow wrote: >> Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland >> <mark.cave-ayland@ilande.co.uk>: >>> On 22/04/2023 16:07, Bernhard Beschow wrote: >>> >>>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >>>> standard-compliant PCI IDE device. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> include/hw/ide/pci.h | 2 -- >>>> hw/ide/pci.c | 4 ++-- >>>> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >>>> 3 files changed, 20 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>>> index 5025df5b82..dbb4b13161 100644 >>>> --- a/include/hw/ide/pci.h >>>> +++ b/include/hw/ide/pci.h >>>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >>>> extern MemoryRegionOps bmdma_addr_ioport_ops; >>>> void pci_ide_create_devs(PCIDevice *dev); >>>> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >>>> -extern const MemoryRegionOps pci_ide_data_le_ops; >>>> #endif >>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>>> index b2fcc00a64..97ccc75aa6 100644 >>>> --- a/hw/ide/pci.c >>>> +++ b/hw/ide/pci.c >>>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >>>> ide_ctrl_write(bus, addr + 2, data); >>>> } >>>> -const MemoryRegionOps pci_ide_cmd_le_ops = { >>>> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >>>> .read = pci_ide_status_read, >>>> .write = pci_ide_ctrl_write, >>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >>>> } >>>> } >>>> -const MemoryRegionOps pci_ide_data_le_ops = { >>>> +static const MemoryRegionOps pci_ide_data_le_ops = { >>>> .read = pci_ide_data_read, >>>> .write = pci_ide_data_write, >>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>>> index 0af897a9ef..9cf920369f 100644 >>>> --- a/hw/ide/sii3112.c >>>> +++ b/hw/ide/sii3112.c >>>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >>>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >>>> val |= (uint32_t)d->i.bmdma[1].status << 16; >>>> break; >>>> - case 0x80 ... 0x87: >>>> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >>>> - break; >>>> - case 0x8a: >>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >>>> - break; >>>> case 0xa0: >>>> val = d->regs[0].confstat; >>>> break; >>>> - case 0xc0 ... 0xc7: >>>> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >>>> - break; >>>> - case 0xca: >>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >>>> - break; >>>> case 0xe0: >>>> val = d->regs[1].confstat; >>>> break; >>>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>>> case 0x0c ... 0x0f: >>>> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >>>> break; >>>> - case 0x80 ... 0x87: >>>> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >>>> - break; >>>> - case 0x8a: >>>> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >>>> - break; >>>> - case 0xc0 ... 0xc7: >>>> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >>>> - break; >>>> - case 0xca: >>>> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >>>> - break; >>>> case 0x100: >>>> d->regs[0].scontrol = val & 0xfff; >>>> if (val & 1) { >>>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>>> pci_config_set_interrupt_pin(dev->config, 1); >>>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >>>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >>>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >>>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >>>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >>>> + >>>> /* BAR5 is in PCI memory space */ >>>> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >>>> "sii3112.bar5", 0x200); >>>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >>>> mr = g_new(MemoryRegion, 1); >>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >>>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >>>> + memory_region_size(&s->data_ops[0])); >>>> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >>>> mr = g_new(MemoryRegion, 1); >>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >>>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >>>> + memory_region_size(&s->cmd_ops[0])); >>>> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >>>> mr = g_new(MemoryRegion, 1); >>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >>>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >>>> + memory_region_size(&s->data_ops[1])); >>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >>>> mr = g_new(MemoryRegion, 1); >>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >>>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >>>> + memory_region_size(&s->cmd_ops[1])); >>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >>>> + >>>> mr = g_new(MemoryRegion, 1); >>>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >>>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >>> >>> So if I read this right, this is now switching the aliases over on BAR5 to allow >>> re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's correct then I think >>> the commit message needs a bit more detail, otherwise: >> >> That's correct. Besides improving the commit message I'll additonally split this >> patch into two to show what's going on. >> >> Furthermore, I'd init the memory regions in sii3112's init method rather than in >> realize(). This will be more consistent with the other PCI IDE device models and >> with the other memory regions. > > Why is an init method meeded? If it's not needed why add it? Just keep it simple. My > view on these methods is that usually only a realize method is needed which is the > normal constructor of the object, but sometimes we need some properties or something > else to be available that can configure the object before realising which is when an > init method is needed. Sometimes QEMU creates an object then destroys it without > realising (I think e.g. when using help to query device properties) so then memory > regions would be created and destroyed pointlessly. So unless there's a good reason > to have an init method I'd leave this device without one to keep it simple. That > other devices have an init method does not explain why this one should have one. > Maybe those devices need it for some properties or they are just old code and not > properly QOM'ified. From memory one of the QOM developer guides recommends that all initialisation should be placed in .instance_init, except for objects that depend on properties which are set after .instance_init but before .realize(). Certainly there is some flexibility around this for legacy code, however quite a few reviewers have picked up on previous series I have posted when I have forgotten to move something from .realize() to .instance_init() when allowed after refactoring. ATB, Mark.
On Thu, 27 Apr 2023, Mark Cave-Ayland wrote: > On 27/04/2023 00:24, BALATON Zoltan wrote: >> On Wed, 26 Apr 2023, Bernhard Beschow wrote: >>> Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland >>> <mark.cave-ayland@ilande.co.uk>: >>>> On 22/04/2023 16:07, Bernhard Beschow wrote: >>>> >>>>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI >>>>> as a >>>>> standard-compliant PCI IDE device. >>>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>> --- >>>>> include/hw/ide/pci.h | 2 -- >>>>> hw/ide/pci.c | 4 ++-- >>>>> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >>>>> 3 files changed, 20 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>>>> index 5025df5b82..dbb4b13161 100644 >>>>> --- a/include/hw/ide/pci.h >>>>> +++ b/include/hw/ide/pci.h >>>>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >>>>> extern MemoryRegionOps bmdma_addr_ioport_ops; >>>>> void pci_ide_create_devs(PCIDevice *dev); >>>>> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >>>>> -extern const MemoryRegionOps pci_ide_data_le_ops; >>>>> #endif >>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>>>> index b2fcc00a64..97ccc75aa6 100644 >>>>> --- a/hw/ide/pci.c >>>>> +++ b/hw/ide/pci.c >>>>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr >>>>> addr, >>>>> ide_ctrl_write(bus, addr + 2, data); >>>>> } >>>>> -const MemoryRegionOps pci_ide_cmd_le_ops = { >>>>> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >>>>> .read = pci_ide_status_read, >>>>> .write = pci_ide_ctrl_write, >>>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr >>>>> addr, >>>>> } >>>>> } >>>>> -const MemoryRegionOps pci_ide_data_le_ops = { >>>>> +static const MemoryRegionOps pci_ide_data_le_ops = { >>>>> .read = pci_ide_data_read, >>>>> .write = pci_ide_data_write, >>>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>>>> index 0af897a9ef..9cf920369f 100644 >>>>> --- a/hw/ide/sii3112.c >>>>> +++ b/hw/ide/sii3112.c >>>>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr >>>>> addr, >>>>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >>>>> val |= (uint32_t)d->i.bmdma[1].status << 16; >>>>> break; >>>>> - case 0x80 ... 0x87: >>>>> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, >>>>> size); >>>>> - break; >>>>> - case 0x8a: >>>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >>>>> - break; >>>>> case 0xa0: >>>>> val = d->regs[0].confstat; >>>>> break; >>>>> - case 0xc0 ... 0xc7: >>>>> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, >>>>> size); >>>>> - break; >>>>> - case 0xca: >>>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >>>>> - break; >>>>> case 0xe0: >>>>> val = d->regs[1].confstat; >>>>> break; >>>>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr >>>>> addr, >>>>> case 0x0c ... 0x0f: >>>>> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, >>>>> size); >>>>> break; >>>>> - case 0x80 ... 0x87: >>>>> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, >>>>> size); >>>>> - break; >>>>> - case 0x8a: >>>>> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >>>>> - break; >>>>> - case 0xc0 ... 0xc7: >>>>> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, >>>>> size); >>>>> - break; >>>>> - case 0xca: >>>>> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >>>>> - break; >>>>> case 0x100: >>>>> d->regs[0].scontrol = val & 0xfff; >>>>> if (val & 1) { >>>>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, >>>>> Error **errp) >>>>> pci_config_set_interrupt_pin(dev->config, 1); >>>>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >>>>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >>>>> &s->data_ops[0]); >>>>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >>>>> &s->cmd_ops[0]); >>>>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >>>>> &s->data_ops[1]); >>>>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >>>>> &s->cmd_ops[1]); >>>>> + >>>>> /* BAR5 is in PCI memory space */ >>>>> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >>>>> "sii3112.bar5", 0x200); >>>>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, >>>>> Error **errp) >>>>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >>>>> mr = g_new(MemoryRegion, 1); >>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, >>>>> 0x80, 8); >>>>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >>>>> &s->data_ops[0], 0, >>>>> + memory_region_size(&s->data_ops[0])); >>>>> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >>>>> mr = g_new(MemoryRegion, 1); >>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, >>>>> 0x88, 4); >>>>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >>>>> &s->cmd_ops[0], 0, >>>>> + memory_region_size(&s->cmd_ops[0])); >>>>> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >>>>> mr = g_new(MemoryRegion, 1); >>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, >>>>> 0xc0, 8); >>>>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >>>>> &s->data_ops[1], 0, >>>>> + memory_region_size(&s->data_ops[1])); >>>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >>>>> mr = g_new(MemoryRegion, 1); >>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, >>>>> 0xc8, 4); >>>>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", >>>>> &s->cmd_ops[1], 0, >>>>> + memory_region_size(&s->cmd_ops[1])); >>>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >>>>> + >>>>> mr = g_new(MemoryRegion, 1); >>>>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, >>>>> 0, 16); >>>>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>> >>>> So if I read this right, this is now switching the aliases over on BAR5 >>>> to allow re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's >>>> correct then I think the commit message needs a bit more detail, >>>> otherwise: >>> >>> That's correct. Besides improving the commit message I'll additonally >>> split this patch into two to show what's going on. >>> >>> Furthermore, I'd init the memory regions in sii3112's init method rather >>> than in realize(). This will be more consistent with the other PCI IDE >>> device models and with the other memory regions. >> >> Why is an init method meeded? If it's not needed why add it? Just keep it >> simple. My view on these methods is that usually only a realize method is >> needed which is the normal constructor of the object, but sometimes we need >> some properties or something else to be available that can configure the >> object before realising which is when an init method is needed. Sometimes >> QEMU creates an object then destroys it without realising (I think e.g. >> when using help to query device properties) so then memory regions would be >> created and destroyed pointlessly. So unless there's a good reason to have >> an init method I'd leave this device without one to keep it simple. That >> other devices have an init method does not explain why this one should have >> one. Maybe those devices need it for some properties or they are just old >> code and not properly QOM'ified. > > From memory one of the QOM developer guides recommends that all > initialisation should be placed in .instance_init, except for objects that > depend on properties which are set after .instance_init but before > .realize(). Any pointer to that doc? I've tried to find it but I couldn't. The docs/devel/qom.rst does not say anything about this, The older collection of docs here: https://habkost.net/posts/2016/11/incomplete-list-of-qemu-apis.html also does not have anything useful either, those mostly talk about properties instead. The header include/hw/qdev-core.h has some info but it only says that "Trivial field initializations should go into #TypeInfo.instance_init. Operations depending on @props static properties should go into @realize." So I don't see a clear guide on what should go where unless something is needed before the device is realized. > Certainly there is some flexibility around this for legacy code, however > quite a few reviewers have picked up on previous series I have posted when I > have forgotten to move something from .realize() to .instance_init() when > allowed after refactoring. I don't mind if things are done in init or realize as long as we have only one of these unless we really need both. Having object init split into two functions for no good reason just makes it mode confusing so I'd like to keep it at a single place for simlicity. Between init and realize the latter seems to be more appropriate for creating the things that are only needed when the object is really created and init is for those that may be needed when the object is not fully up yet, e.g. for introspection or working with connected objects. But for simple devices like sii3112 having more than a realize method probably does not make much sense. If you have examples of those reviews you refer to maybe that explains this more. I just want to avoid unneded complexity. Regards, BALATON Zoltan
On 27/04/2023 13:55, BALATON Zoltan wrote: > On Thu, 27 Apr 2023, Mark Cave-Ayland wrote: >> On 27/04/2023 00:24, BALATON Zoltan wrote: >>> On Wed, 26 Apr 2023, Bernhard Beschow wrote: >>>> Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland >>>> <mark.cave-ayland@ilande.co.uk>: >>>>> On 22/04/2023 16:07, Bernhard Beschow wrote: >>>>> >>>>>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a >>>>>> standard-compliant PCI IDE device. >>>>>> >>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>>> --- >>>>>> include/hw/ide/pci.h | 2 -- >>>>>> hw/ide/pci.c | 4 ++-- >>>>>> hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- >>>>>> 3 files changed, 20 insertions(+), 36 deletions(-) >>>>>> >>>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>>>>> index 5025df5b82..dbb4b13161 100644 >>>>>> --- a/include/hw/ide/pci.h >>>>>> +++ b/include/hw/ide/pci.h >>>>>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); >>>>>> extern MemoryRegionOps bmdma_addr_ioport_ops; >>>>>> void pci_ide_create_devs(PCIDevice *dev); >>>>>> -extern const MemoryRegionOps pci_ide_cmd_le_ops; >>>>>> -extern const MemoryRegionOps pci_ide_data_le_ops; >>>>>> #endif >>>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>>>>> index b2fcc00a64..97ccc75aa6 100644 >>>>>> --- a/hw/ide/pci.c >>>>>> +++ b/hw/ide/pci.c >>>>>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, >>>>>> ide_ctrl_write(bus, addr + 2, data); >>>>>> } >>>>>> -const MemoryRegionOps pci_ide_cmd_le_ops = { >>>>>> +static const MemoryRegionOps pci_ide_cmd_le_ops = { >>>>>> .read = pci_ide_status_read, >>>>>> .write = pci_ide_ctrl_write, >>>>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>>>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, >>>>>> } >>>>>> } >>>>>> -const MemoryRegionOps pci_ide_data_le_ops = { >>>>>> +static const MemoryRegionOps pci_ide_data_le_ops = { >>>>>> .read = pci_ide_data_read, >>>>>> .write = pci_ide_data_write, >>>>>> .endianness = DEVICE_LITTLE_ENDIAN, >>>>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>>>>> index 0af897a9ef..9cf920369f 100644 >>>>>> --- a/hw/ide/sii3112.c >>>>>> +++ b/hw/ide/sii3112.c >>>>>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, >>>>>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); >>>>>> val |= (uint32_t)d->i.bmdma[1].status << 16; >>>>>> break; >>>>>> - case 0x80 ... 0x87: >>>>>> - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); >>>>>> - break; >>>>>> - case 0x8a: >>>>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); >>>>>> - break; >>>>>> case 0xa0: >>>>>> val = d->regs[0].confstat; >>>>>> break; >>>>>> - case 0xc0 ... 0xc7: >>>>>> - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); >>>>>> - break; >>>>>> - case 0xca: >>>>>> - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); >>>>>> - break; >>>>>> case 0xe0: >>>>>> val = d->regs[1].confstat; >>>>>> break; >>>>>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>>>>> case 0x0c ... 0x0f: >>>>>> bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); >>>>>> break; >>>>>> - case 0x80 ... 0x87: >>>>>> - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); >>>>>> - break; >>>>>> - case 0x8a: >>>>>> - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); >>>>>> - break; >>>>>> - case 0xc0 ... 0xc7: >>>>>> - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); >>>>>> - break; >>>>>> - case 0xca: >>>>>> - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); >>>>>> - break; >>>>>> case 0x100: >>>>>> d->regs[0].scontrol = val & 0xfff; >>>>>> if (val & 1) { >>>>>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) >>>>>> pci_config_set_interrupt_pin(dev->config, 1); >>>>>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); >>>>>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); >>>>>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); >>>>>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); >>>>>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); >>>>>> + >>>>>> /* BAR5 is in PCI memory space */ >>>>>> memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, >>>>>> "sii3112.bar5", 0x200); >>>>>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error >>>>>> **errp) >>>>>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ >>>>>> mr = g_new(MemoryRegion, 1); >>>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); >>>>>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, >>>>>> + memory_region_size(&s->data_ops[0])); >>>>>> + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); >>>>>> mr = g_new(MemoryRegion, 1); >>>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); >>>>>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, >>>>>> + memory_region_size(&s->cmd_ops[0])); >>>>>> + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); >>>>>> mr = g_new(MemoryRegion, 1); >>>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); >>>>>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, >>>>>> + memory_region_size(&s->data_ops[1])); >>>>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); >>>>>> mr = g_new(MemoryRegion, 1); >>>>>> - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); >>>>>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>>> + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, >>>>>> + memory_region_size(&s->cmd_ops[1])); >>>>>> + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); >>>>>> + >>>>>> mr = g_new(MemoryRegion, 1); >>>>>> memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); >>>>>> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr); >>>>> >>>>> So if I read this right, this is now switching the aliases over on BAR5 to allow >>>>> re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's correct then I >>>>> think the commit message needs a bit more detail, otherwise: >>>> >>>> That's correct. Besides improving the commit message I'll additonally split this >>>> patch into two to show what's going on. >>>> >>>> Furthermore, I'd init the memory regions in sii3112's init method rather than in >>>> realize(). This will be more consistent with the other PCI IDE device models and >>>> with the other memory regions. >>> >>> Why is an init method meeded? If it's not needed why add it? Just keep it simple. >>> My view on these methods is that usually only a realize method is needed which is >>> the normal constructor of the object, but sometimes we need some properties or >>> something else to be available that can configure the object before realising >>> which is when an init method is needed. Sometimes QEMU creates an object then >>> destroys it without realising (I think e.g. when using help to query device >>> properties) so then memory regions would be created and destroyed pointlessly. So >>> unless there's a good reason to have an init method I'd leave this device without >>> one to keep it simple. That other devices have an init method does not explain why >>> this one should have one. Maybe those devices need it for some properties or they >>> are just old code and not properly QOM'ified. >> >> From memory one of the QOM developer guides recommends that all initialisation >> should be placed in .instance_init, except for objects that depend on properties >> which are set after .instance_init but before .realize(). > > Any pointer to that doc? I've tried to find it but I couldn't. The docs/devel/qom.rst > does not say anything about this, The older collection of docs here: > https://habkost.net/posts/2016/11/incomplete-list-of-qemu-apis.html > also does not have anything useful either, those mostly talk about properties > instead. The header include/hw/qdev-core.h has some info but it only says that > "Trivial field initializations should go into #TypeInfo.instance_init. Operations > depending on @props static properties should go into @realize." So I don't see a > clear guide on what should go where unless something is needed before the device is > realized. I found a reference to this in Anthony's original documentation at https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg02271.html (see the section "Using Instance Initialization"). >> Certainly there is some flexibility around this for legacy code, however quite a >> few reviewers have picked up on previous series I have posted when I have forgotten >> to move something from .realize() to .instance_init() when allowed after refactoring. > > I don't mind if things are done in init or realize as long as we have only one of > these unless we really need both. Having object init split into two functions for no > good reason just makes it mode confusing so I'd like to keep it at a single place for > simlicity. Between init and realize the latter seems to be more appropriate for > creating the things that are only needed when the object is really created and init > is for those that may be needed when the object is not fully up yet, e.g. for > introspection or working with connected objects. But for simple devices like sii3112 > having more than a realize method probably does not make much sense. If you have > examples of those reviews you refer to maybe that explains this more. I just want to > avoid unneded complexity. Note that the concept of realisation only exists for devices i.e. things derived from TYPE_DEVICE as opposed to things derived from TYPE_OBJECT. ATB, Mark.
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index 5025df5b82..dbb4b13161 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); extern MemoryRegionOps bmdma_addr_ioport_ops; void pci_ide_create_devs(PCIDevice *dev); -extern const MemoryRegionOps pci_ide_cmd_le_ops; -extern const MemoryRegionOps pci_ide_data_le_ops; #endif diff --git a/hw/ide/pci.c b/hw/ide/pci.c index b2fcc00a64..97ccc75aa6 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr, ide_ctrl_write(bus, addr + 2, data); } -const MemoryRegionOps pci_ide_cmd_le_ops = { +static const MemoryRegionOps pci_ide_cmd_le_ops = { .read = pci_ide_status_read, .write = pci_ide_ctrl_write, .endianness = DEVICE_LITTLE_ENDIAN, @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr, } } -const MemoryRegionOps pci_ide_data_le_ops = { +static const MemoryRegionOps pci_ide_data_le_ops = { .read = pci_ide_data_read, .write = pci_ide_data_write, .endianness = DEVICE_LITTLE_ENDIAN, diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c index 0af897a9ef..9cf920369f 100644 --- a/hw/ide/sii3112.c +++ b/hw/ide/sii3112.c @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0); val |= (uint32_t)d->i.bmdma[1].status << 16; break; - case 0x80 ... 0x87: - val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size); - break; - case 0x8a: - val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size); - break; case 0xa0: val = d->regs[0].confstat; break; - case 0xc0 ... 0xc7: - val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size); - break; - case 0xca: - val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size); - break; case 0xe0: val = d->regs[1].confstat; break; @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, case 0x0c ... 0x0f: bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size); break; - case 0x80 ... 0x87: - pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size); - break; - case 0x8a: - pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size); - break; - case 0xc0 ... 0xc7: - pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size); - break; - case 0xca: - pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size); - break; case 0x100: d->regs[0].scontrol = val & 0xfff; if (val & 1) { @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) pci_config_set_interrupt_pin(dev->config, 1); pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8); + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[0]); + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]); + /* BAR5 is in PCI memory space */ memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d, "sii3112.bar5", 0x200); @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp) /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */ mr = g_new(MemoryRegion, 1); - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8); - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr); + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0, + memory_region_size(&s->data_ops[0])); + memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1); mr = g_new(MemoryRegion, 1); - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4); - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr); + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0, + memory_region_size(&s->cmd_ops[0])); + memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1); mr = g_new(MemoryRegion, 1); - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8); - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr); + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0, + memory_region_size(&s->data_ops[1])); + memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1); mr = g_new(MemoryRegion, 1); - memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4); - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr); + memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0, + memory_region_size(&s->cmd_ops[1])); + memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1); + mr = g_new(MemoryRegion, 1); memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a standard-compliant PCI IDE device. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/ide/pci.h | 2 -- hw/ide/pci.c | 4 ++-- hw/ide/sii3112.c | 50 ++++++++++++++++---------------------------- 3 files changed, 20 insertions(+), 36 deletions(-)