| Message ID | 1511388334-16347-8-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22.11.2017 23:05, Pierre Morel wrote: > When dispatching memory access to PCI BAR region, we must > look for possible subregions, used by the PCI device to map > 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 access to the MSIX region is now handled in a generic way, > we do not need the specific trap_msix() function anymore. > > 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 8d35f8f..fae3ef7 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -345,12 +345,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; Just my personal taste, but I'd rather call it "submr" or something similar. > + 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)) { (Too much parentheses for my taste ;-)) > + 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; I'm not that familiar with the MemoryRegion nesting ... So may I ask: Is that "offset -= mr->addr" also right if mr does *not* point to a sub-region, but to the main memory region? (I.e. is mr->addr == 0 in that case?) > return memory_region_dispatch_read(mr, offset, data, len, > MEMTXATTRS_UNSPECIFIED); > } > @@ -442,30 +461,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); > } > @@ -726,6 +729,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > } > > mr = pbdev->pdev->io_regions[pcias].memory; Since this above line is done right before all calls to s390_get_subregion(), you could maybe move it into that function, too. (and rather call the function sth. like pci_mem_get_subregion() instead?) > + 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; > Thomas
On 23/11/2017 10:54, Thomas Huth wrote: > On 22.11.2017 23:05, Pierre Morel wrote: >> When dispatching memory access to PCI BAR region, we must >> look for possible subregions, used by the PCI device to map >> 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 access to the MSIX region is now handled in a generic way, >> we do not need the specific trap_msix() function anymore. >> >> 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 8d35f8f..fae3ef7 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -345,12 +345,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; > > Just my personal taste, but I'd rather call it "submr" or something similar. OK, you are right it is more speaking. > >> + 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)) { > > (Too much parentheses for my taste ;-)) I find it more readable so. So if no other opinion I would let it so. > >> + 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; > > I'm not that familiar with the MemoryRegion nesting ... So may I ask: Is > that "offset -= mr->addr" also right if mr does *not* point to a > sub-region, but to the main memory region? (I.e. is mr->addr == 0 in > that case?) Yes. This routine is common to VIRTIO and VFIO and we have a single region in VFIO. > >> return memory_region_dispatch_read(mr, offset, data, len, >> MEMTXATTRS_UNSPECIFIED); >> } >> @@ -442,30 +461,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); >> } >> @@ -726,6 +729,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, >> } >> >> mr = pbdev->pdev->io_regions[pcias].memory; > > Since this above line is done right before all calls to > s390_get_subregion(), you could maybe move it into that function, too. > (and rather call the function sth. like pci_mem_get_subregion() instead?) Yes, I know, I could also have a pointer to offset in the call and put the offset adjustment in the call. I save 4 lines but have an extra argument and I find that the result is less readable. So it no other opinions I would prefer to let it like this. > >> + 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; >> > > Thomas > Thanks for the review, Pierre
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 8d35f8f..fae3ef7 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -345,12 +345,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); } @@ -442,30 +461,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); } @@ -726,6 +729,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;