Re: [Qemu-devel] [PATCH v3 7/7] s390x/pci: search for subregion inside the BARs

[Pierre Morel]

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 <address@hidden>
> > Reviewed-by: Yi Min Zhao <address@hidden>
> > ---
> >   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

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany