Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

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.