| Message ID | 1510075479-17224-8-git-send-email-pmorel@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/pci: Improve zPCI to cover more cases | expand |
On Tue, 7 Nov 2017 18:24:39 +0100 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > When dispatching memory access to PCI BAR region, we must > look for eventual subregion, used by the PCI device to map s/eventual subregion/possible subregions/ ? > different memory areas inside the same PCI BAR. > > Since the data offset we received is calculated starting at the > region start address we need to adjust the offset for the subregion. > > The data offset inside the subregion is calculated by substracting > the subregion's starting address from the data offset in the region. > > The function trap_msix() was used, despite not really useful since we > removed the index from the msix message. With this patch we can > definitively suppress this function. Suggest rewording this (see below). > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > --- > hw/s390x/s390-pci-inst.c | 44 +++++++++++++++++++++++++------------------- > 1 file changed, 25 insertions(+), 19 deletions(-) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 97f62b5..b59ceef 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -344,12 +344,31 @@ static int zpci_endian_swap(uint64_t *ptr, uint8_t len) > return 0; > } > > +static MemoryRegion *s390_get_subregion(MemoryRegion *mr, uint64_t offset, > + uint8_t len) > +{ > + MemoryRegion *other; > + uint64_t subregion_size; > + > + QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { > + subregion_size = int128_get64(other->size); > + if ((offset >= other->addr) && > + (offset + len) <= (other->addr + subregion_size)) { > + mr = other; > + break; > + } > + } > + return mr; > +} > + So, this works for the msix stuff because it is a subregion? This means you can remove trap_msix() because it is handled in a generic way now, doesn't it? > static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, uint8_t pcias, > uint64_t offset, uint64_t *data, uint8_t len) > { > MemoryRegion *mr; > > mr = pbdev->pdev->io_regions[pcias].memory; > + mr = s390_get_subregion(mr, offset, len); > + offset -= mr->addr; > return memory_region_dispatch_read(mr, offset, data, len, > MEMTXATTRS_UNSPECIFIED); > } > @@ -443,30 +462,14 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) > return 0; > } > > -static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) > -{ > - if (pbdev->msix.available && pbdev->msix.table_bar == pcias && > - offset >= pbdev->msix.table_offset && > - offset < (pbdev->msix.table_offset + > - pbdev->msix.entries * PCI_MSIX_ENTRY_SIZE)) { > - return 1; > - } else { > - return 0; > - } > -} > - > static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias, > uint64_t offset, uint64_t data, uint8_t len) > { > MemoryRegion *mr; > > - if (trap_msix(pbdev, offset, pcias)) { > - offset = offset - pbdev->msix.table_offset; > - mr = &pbdev->pdev->msix_table_mmio; > - } else { > - mr = pbdev->pdev->io_regions[pcias].memory; > - } > - > + mr = pbdev->pdev->io_regions[pcias].memory; > + mr = s390_get_subregion(mr, offset, len); > + offset -= mr->addr; > return memory_region_dispatch_write(mr, offset, data, len, > MEMTXATTRS_UNSPECIFIED); > } > @@ -728,6 +731,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > } > > mr = pbdev->pdev->io_regions[pcias].memory; > + mr = s390_get_subregion(mr, offset, len); > + offset -= mr->addr; > + > if (!memory_region_access_valid(mr, offset, len, true)) { > program_interrupt(env, PGM_OPERAND, 6); > return 0;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 97f62b5..b59ceef 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -344,12 +344,31 @@ static int zpci_endian_swap(uint64_t *ptr, uint8_t len) return 0; } +static MemoryRegion *s390_get_subregion(MemoryRegion *mr, uint64_t offset, + uint8_t len) +{ + MemoryRegion *other; + uint64_t subregion_size; + + QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { + subregion_size = int128_get64(other->size); + if ((offset >= other->addr) && + (offset + len) <= (other->addr + subregion_size)) { + mr = other; + break; + } + } + return mr; +} + static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, uint8_t pcias, uint64_t offset, uint64_t *data, uint8_t len) { MemoryRegion *mr; mr = pbdev->pdev->io_regions[pcias].memory; + mr = s390_get_subregion(mr, offset, len); + offset -= mr->addr; return memory_region_dispatch_read(mr, offset, data, len, MEMTXATTRS_UNSPECIFIED); } @@ -443,30 +462,14 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) return 0; } -static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) -{ - if (pbdev->msix.available && pbdev->msix.table_bar == pcias && - offset >= pbdev->msix.table_offset && - offset < (pbdev->msix.table_offset + - pbdev->msix.entries * PCI_MSIX_ENTRY_SIZE)) { - return 1; - } else { - return 0; - } -} - static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias, uint64_t offset, uint64_t data, uint8_t len) { MemoryRegion *mr; - if (trap_msix(pbdev, offset, pcias)) { - offset = offset - pbdev->msix.table_offset; - mr = &pbdev->pdev->msix_table_mmio; - } else { - mr = pbdev->pdev->io_regions[pcias].memory; - } - + mr = pbdev->pdev->io_regions[pcias].memory; + mr = s390_get_subregion(mr, offset, len); + offset -= mr->addr; return memory_region_dispatch_write(mr, offset, data, len, MEMTXATTRS_UNSPECIFIED); } @@ -728,6 +731,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, } mr = pbdev->pdev->io_regions[pcias].memory; + mr = s390_get_subregion(mr, offset, len); + offset -= mr->addr; + if (!memory_region_access_valid(mr, offset, len, true)) { program_interrupt(env, PGM_OPERAND, 6); return 0;