On Tue, Nov 11, 2014 at 01:16:04PM +0100, Alexander Graf wrote:
>
>
> On 11.11.14 13:10, Frank Blaschka wrote:
> > On Mon, Nov 10, 2014 at 04:56:21PM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 10.11.14 15:20, Frank Blaschka wrote:
> >>> From: Frank Blaschka <frank.blasc...@de.ibm.com>
> >>>
> >>> This patch implements the s390 pci instructions in qemu. It allows
> >>> to access and drive pci devices attached to the s390 pci bus.
> >>> Because of platform constrains devices using IO BARs are not
> >>> supported. Also a device has to support MSI/MSI-X to run on s390.
> >>>
> >>> Signed-off-by: Frank Blaschka <frank.blasc...@de.ibm.com>
> >>> ---
> >>> target-s390x/Makefile.objs | 2 +-
> >>> target-s390x/kvm.c | 52 ++++
> >>> target-s390x/pci_ic.c | 753
> >>> +++++++++++++++++++++++++++++++++++++++++++++
> >>> target-s390x/pci_ic.h | 335 ++++++++++++++++++++
> >>> 4 files changed, 1141 insertions(+), 1 deletion(-)
> >>> create mode 100644 target-s390x/pci_ic.c
> >>> create mode 100644 target-s390x/pci_ic.h
> >>>
>
> [...]
>
> >>> +int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
> >>> +{
> >>> + CPUS390XState *env = &cpu->env;
> >>> + S390PCIBusDevice *pbdev;
> >>> + uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
> >>> + uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> >>> + PciLgStg *rp;
> >>> + uint64_t offset;
> >>> + uint64_t data;
> >>> + uint8_t len;
> >>> +
> >>> + cpu_synchronize_state(CPU(cpu));
> >>> +
> >>> + if (env->psw.mask & PSW_MASK_PSTATE) {
> >>> + program_interrupt(env, PGM_PRIVILEGED, 4);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + if (r2 & 0x1) {
> >>> + program_interrupt(env, PGM_SPECIFICATION, 4);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + rp = (PciLgStg *)&env->regs[r2];
> >>> + offset = env->regs[r2 + 1];
> >>> +
> >>> + pbdev = s390_pci_find_dev_by_fh(rp->fh);
> >>> + if (!pbdev) {
> >>> + DPRINTF("pcilg no pci dev\n");
> >>> + setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + len = rp->len & 0xF;
> >>> + if (rp->pcias < 6) {
> >>> + if ((8 - (offset & 0x7)) < len) {
> >>> + program_interrupt(env, PGM_OPERAND, 4);
> >>> + return 0;
> >>> + }
> >>> + MemoryRegion *mr = pbdev->pdev->io_regions[rp->pcias].memory;
> >>> + io_mem_read(mr, offset, &data, len);
> >>> + } else if (rp->pcias == 15) {
> >>> + if ((4 - (offset & 0x3)) < len) {
> >>> + program_interrupt(env, PGM_OPERAND, 4);
> >>> + return 0;
> >>> + }
> >>> + data = pci_host_config_read_common(
> >>> + pbdev->pdev, offset, pci_config_size(pbdev->pdev),
> >>> len);
> >>> +
> >>> + switch (len) {
> >>> + case 1:
> >>> + break;
> >>> + case 2:
> >>> + data = cpu_to_le16(data);
> >>> + break;
> >>> + case 4:
> >>> + data = cpu_to_le32(data);
> >>> + break;
> >>> + case 8:
> >>> + data = cpu_to_le64(data);
> >>> + break;
> >>
> >> Why? Also, this is wrong. cpu_to_le64 convert between host endianness
> >> and LE. So if you're running this on an LE host, you won't swap the
> >> value and get a broken result.
> >>
> >> If you know that the value is always swapped, use bswapxx().
> >>
> >
> > Actually the code is right and required for a big endian host :-)
> > pcilg/pcistg provide access to the PCI config space which is defined
> > as PCI byte order (little endian). Since pci_host_config_read_common does
> > already a le to cpu conversion we have to convert back to PCI byte order.
> > Doing an unconditional swap would be a bug on a little endian host.
>
> Why would it be a bug? The value you end up writing is contents of a
> register and thus doesn't have endianness. So if QEMU was an LE process,
No, the s390 guest executing pcilg instruction expects to receive config space
data
in PCI byte order.
> the value of data would be identical as on a BE QEMU before your swab.
> After the swab, it would be bswap'ed on BE, but not LE. So LE hosts break.
>
Again on BE endian host we do the swap because of pci_host_config_read_common
does
read the value and do a byte swap for that value, but we need PCI byte order
not BE here.
On LE host pci_host_config_read_common does not do a byte swap so we do not
have to
convert back to PCI byte order.
>
> Alex
>
