| Message ID | 1510075479-17224-7-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 7 Nov 2017 18:24:38 +0100 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > Let's move the memory region write from pcistg into a dedicated > function. > This allows us to prepare a later patch searching for subregions > inside of the memory region. OK, so here is the memory region write. Do we have any sleeping endianness bugs in there for when we wire up tcg? I'm not sure how this plays with the bswaps (see patch 1). But maybe I've just gotten lost somewhere. > > 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 | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 50135a0..97f62b5 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) > } > } > > +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; > + } > + > + return memory_region_dispatch_write(mr, offset, data, len, > + MEMTXATTRS_UNSPECIFIED); > +} > + > int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) > { > CPUS390XState *env = &cpu->env; > uint64_t offset, data; > S390PCIBusDevice *pbdev; > - MemoryRegion *mr; > MemTxResult result; > uint8_t len; > uint32_t fh; > @@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) > return 0; > } > > - 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; > - } > - > - result = memory_region_dispatch_write(mr, offset, data, len, > - MEMTXATTRS_UNSPECIFIED); > + result = zpci_write_bar(pbdev, pcias, offset, data, len); > if (result != MEMTX_OK) { > program_interrupt(env, PGM_OPERAND, 4); > return 0;
在 2017/11/10 上午3:23, Cornelia Huck 写道: > On Tue, 7 Nov 2017 18:24:38 +0100 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> Let's move the memory region write from pcistg into a dedicated >> function. >> This allows us to prepare a later patch searching for subregions >> inside of the memory region. > OK, so here is the memory region write. Do we have any sleeping > endianness bugs in there for when we wire up tcg? I'm not sure how this > plays with the bswaps (see patch 1). > > But maybe I've just gotten lost somewhere. I think there's no error. For PCI bars' MRs, we got the little-endian data that is exactly fit to the byte ordering of pcilg instruction. For PCI config space, the data has been swapped according to the cpu byte ordering. So we use zpci_swap_endian() to swap the data back to the little-endian ordering. > >> 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 | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 50135a0..97f62b5 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) >> } >> } >> >> +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; >> + } >> + >> + return memory_region_dispatch_write(mr, offset, data, len, >> + MEMTXATTRS_UNSPECIFIED); >> +} >> + >> int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) >> { >> CPUS390XState *env = &cpu->env; >> uint64_t offset, data; >> S390PCIBusDevice *pbdev; >> - MemoryRegion *mr; >> MemTxResult result; >> uint8_t len; >> uint32_t fh; >> @@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) >> return 0; >> } >> >> - 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; >> - } >> - >> - result = memory_region_dispatch_write(mr, offset, data, len, >> - MEMTXATTRS_UNSPECIFIED); >> + result = zpci_write_bar(pbdev, pcias, offset, data, len); >> if (result != MEMTX_OK) { >> program_interrupt(env, PGM_OPERAND, 4); >> return 0; >
On Fri, 10 Nov 2017 17:40:12 +0800 Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > 在 2017/11/10 上午3:23, Cornelia Huck 写道: > > On Tue, 7 Nov 2017 18:24:38 +0100 > > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > > > >> Let's move the memory region write from pcistg into a dedicated > >> function. > >> This allows us to prepare a later patch searching for subregions > >> inside of the memory region. > > OK, so here is the memory region write. Do we have any sleeping > > endianness bugs in there for when we wire up tcg? I'm not sure how this > > plays with the bswaps (see patch 1). > > > > But maybe I've just gotten lost somewhere. > I think there's no error. For PCI bars' MRs, we got the little-endian data > that is exactly fit to the byte ordering of pcilg instruction. For PCI > config > space, the data has been swapped according to the cpu byte ordering. Host or target cpu? > So we use zpci_swap_endian() to swap the data back to the little-endian > ordering. That swap is unconditional. If we were running on a little-endian host, it would be wrong, wouldn't it? > > > >> 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 | 27 +++++++++++++++++---------- > >> 1 file changed, 17 insertions(+), 10 deletions(-)
On 10/11/2017 10:51, Cornelia Huck wrote: > On Fri, 10 Nov 2017 17:40:12 +0800 > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > >> 在 2017/11/10 上午3:23, Cornelia Huck 写道: >>> On Tue, 7 Nov 2017 18:24:38 +0100 >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: >>> >>>> Let's move the memory region write from pcistg into a dedicated >>>> function. >>>> This allows us to prepare a later patch searching for subregions >>>> inside of the memory region. >>> OK, so here is the memory region write. Do we have any sleeping >>> endianness bugs in there for when we wire up tcg? I'm not sure how this >>> plays with the bswaps (see patch 1). >>> >>> But maybe I've just gotten lost somewhere. >> I think there's no error. For PCI bars' MRs, we got the little-endian data >> that is exactly fit to the byte ordering of pcilg instruction. For PCI >> config >> space, the data has been swapped according to the cpu byte ordering. > > Host or target cpu? > >> So we use zpci_swap_endian() to swap the data back to the little-endian >> ordering. I do not see where we use the zpci_swap_endian() function in this patch or in the zpci_write_bar() function. > > That swap is unconditional. If we were running on a little-endian host, > it would be wrong, wouldn't it? I think there is no problem here, we do not use this function but we use the memory_region_dispatch_write() to access a subregion of the PCI device which is defined as DEVICE_LITTLE_ENDIAN AFAIU The memory access process the endianness correctly. > >>> >>>> 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 | 27 +++++++++++++++++---------- >>>> 1 file changed, 17 insertions(+), 10 deletions(-) >
On 10/11/2017 10:51, Cornelia Huck wrote: > On Fri, 10 Nov 2017 17:40:12 +0800 > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > >> 在 2017/11/10 上午3:23, Cornelia Huck 写道: >>> On Tue, 7 Nov 2017 18:24:38 +0100 >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: >>> >>>> Let's move the memory region write from pcistg into a dedicated >>>> function. >>>> This allows us to prepare a later patch searching for subregions >>>> inside of the memory region. >>> OK, so here is the memory region write. Do we have any sleeping >>> endianness bugs in there for when we wire up tcg? I'm not sure how this >>> plays with the bswaps (see patch 1). >>> >>> But maybe I've just gotten lost somewhere. >> I think there's no error. For PCI bars' MRs, we got the little-endian data >> that is exactly fit to the byte ordering of pcilg instruction. For PCI >> config >> space, the data has been swapped according to the cpu byte ordering. > > Host or target cpu? > >> So we use zpci_swap_endian() to swap the data back to the little-endian >> ordering. > I do not see where we use the zpci_swap_endian() function in this patch or in the zpci_write_bar() function. > That swap is unconditional. If we were running on a little-endian host, > it would be wrong, wouldn't it? I think there is no problem here, we do not use the swap function but we use the memory_region_dispatch_write() to access a subregion of the PCI device which is defined as DEVICE_LITTLE_ENDIAN AFAIU The memory access process the endianness correctly. > >>> >>>> 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 | 27 +++++++++++++++++---------- >>>> 1 file changed, 17 insertions(+), 10 deletions(-) >
On Mon, 13 Nov 2017 10:39:50 +0100 Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > On 10/11/2017 10:51, Cornelia Huck wrote: > > On Fri, 10 Nov 2017 17:40:12 +0800 > > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: > > > >> 在 2017/11/10 上午3:23, Cornelia Huck 写道: > >>> On Tue, 7 Nov 2017 18:24:38 +0100 > >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >>> > >>>> Let's move the memory region write from pcistg into a dedicated > >>>> function. > >>>> This allows us to prepare a later patch searching for subregions > >>>> inside of the memory region. > >>> OK, so here is the memory region write. Do we have any sleeping > >>> endianness bugs in there for when we wire up tcg? I'm not sure how this > >>> plays with the bswaps (see patch 1). > >>> > >>> But maybe I've just gotten lost somewhere. > >> I think there's no error. For PCI bars' MRs, we got the little-endian data > >> that is exactly fit to the byte ordering of pcilg instruction. For PCI > >> config > >> space, the data has been swapped according to the cpu byte ordering. > > > > Host or target cpu? > > > >> So we use zpci_swap_endian() to swap the data back to the little-endian > >> ordering. > > > > > I do not see where we use the zpci_swap_endian() function in this patch > or in the zpci_write_bar() function. So, is that swap function only ever used to convert BE register contents to LE? > > > > > That swap is unconditional. If we were running on a little-endian host, > > it would be wrong, wouldn't it? > > I think there is no problem here, we do not use the swap function but we > use the memory_region_dispatch_write() to access a subregion of the PCI > device which is defined as DEVICE_LITTLE_ENDIAN > > AFAIU The memory access process the endianness correctly. OK, if this is all going through generic memory mechanisms, we should be fine. > > > > > >>> > >>>> 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 | 27 +++++++++++++++++---------- > >>>> 1 file changed, 17 insertions(+), 10 deletions(-) > > > >
On 13/11/2017 12:54, Cornelia Huck wrote: > On Mon, 13 Nov 2017 10:39:50 +0100 > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: > >> On 10/11/2017 10:51, Cornelia Huck wrote: >>> On Fri, 10 Nov 2017 17:40:12 +0800 >>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote: >>> >>>> 在 2017/11/10 上午3:23, Cornelia Huck 写道: >>>>> On Tue, 7 Nov 2017 18:24:38 +0100 >>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote: >>>>> >>>>>> Let's move the memory region write from pcistg into a dedicated >>>>>> function. >>>>>> This allows us to prepare a later patch searching for subregions >>>>>> inside of the memory region. >>>>> OK, so here is the memory region write. Do we have any sleeping >>>>> endianness bugs in there for when we wire up tcg? I'm not sure how this >>>>> plays with the bswaps (see patch 1). >>>>> >>>>> But maybe I've just gotten lost somewhere. >>>> I think there's no error. For PCI bars' MRs, we got the little-endian data >>>> that is exactly fit to the byte ordering of pcilg instruction. For PCI >>>> config >>>> space, the data has been swapped according to the cpu byte ordering. >>> >>> Host or target cpu? >>> >>>> So we use zpci_swap_endian() to swap the data back to the little-endian >>>> ordering. >>> >> >> >> I do not see where we use the zpci_swap_endian() function in this patch >> or in the zpci_write_bar() function. > > So, is that swap function only ever used to convert BE register > contents to LE? Yes that is what I understood. The value in the register is read from memory somehow but it may be an immediate value setup by another instruction, I think the TCG should have already make sure it is big endian. On the other side the PCI memory is always Little endian AFAIK. So the swap function is always from BIG to little. But as I said, we do not use the zpci_swap_endian in this function, so I write this answer again in the thread where the definition of the function is to continue the discussion on the register to PCI translation there. > >> >> >> >>> That swap is unconditional. If we were running on a little-endian host, >>> it would be wrong, wouldn't it? >> >> I think there is no problem here, we do not use the swap function but we >> use the memory_region_dispatch_write() to access a subregion of the PCI >> device which is defined as DEVICE_LITTLE_ENDIAN >> >> AFAIU The memory access process the endianness correctly. > > OK, if this is all going through generic memory mechanisms, we should > be fine. > >> >> >>> >>>>> >>>>>> 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 | 27 +++++++++++++++++---------- >>>>>> 1 file changed, 17 insertions(+), 10 deletions(-) >>> >> >> >
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 50135a0..97f62b5 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) } } +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; + } + + return memory_region_dispatch_write(mr, offset, data, len, + MEMTXATTRS_UNSPECIFIED); +} + int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) { CPUS390XState *env = &cpu->env; uint64_t offset, data; S390PCIBusDevice *pbdev; - MemoryRegion *mr; MemTxResult result; uint8_t len; uint32_t fh; @@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) return 0; } - 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; - } - - result = memory_region_dispatch_write(mr, offset, data, len, - MEMTXATTRS_UNSPECIFIED); + result = zpci_write_bar(pbdev, pcias, offset, data, len); if (result != MEMTX_OK) { program_interrupt(env, PGM_OPERAND, 4); return 0;