On 17/11/2020 19.49, Philippe Mathieu-Daudé wrote: > On 11/17/20 7:39 PM, Thomas Huth wrote: >> On 17/11/2020 19.30, Philippe Mathieu-Daudé wrote: >>> On 11/17/20 7:19 PM, Matthew Rosato wrote: >>>> On 11/17/20 12:59 PM, Philippe Mathieu-Daudé wrote: >>>>> On 11/17/20 6:13 PM, Cornelia Huck wrote: >>>>>> zPCI control blocks are big endian, we need to take care that we >>>>>> do proper accesses in order not to break tcg guests on little endian >>>>>> hosts. >>>>>> >>>>>> Fixes: 28dc86a07299 ("s390x/pci: use a PCI Group structure") >>>>>> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure") >>>>>> Fixes: 1e7552ff5c34 ("s390x/pci: get zPCI function info from host") >>>>>> Signed-off-by: Cornelia Huck <coh...@redhat.com> >>>>>> --- >>>>>> >>>>>> Works for me with virtio-pci devices for tcg on x86 and s390x, and >>>>>> for kvm. >>>>>> The vfio changes are not strictly needed; did not test them due to >>>>>> lack of >>>>>> hardware -- testing appreciated. >> >>>>>> As this fixes a regression, I want this in 5.2. >>>>>> >>>>>> --- >>>>>> hw/s390x/s390-pci-bus.c | 12 ++++++------ >>>>>> hw/s390x/s390-pci-inst.c | 4 ++-- >>>>>> hw/s390x/s390-pci-vfio.c | 8 ++++---- >>>>>> 3 files changed, 12 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >>>>>> index e0dc20ce4a56..17e64e0b1200 100644 >>>>>> --- a/hw/s390x/s390-pci-bus.c >>>>>> +++ b/hw/s390x/s390-pci-bus.c >>>>>> @@ -787,12 +787,12 @@ static void s390_pci_init_default_group(void) >>>>>> static void set_pbdev_info(S390PCIBusDevice *pbdev) >>>>>> { >>>>>> - pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR; >>>>>> - pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR; >>>>>> - pbdev->zpci_fn.pchid = 0; >>>>>> + stq_p(&pbdev->zpci_fn.sdma, ZPCI_SDMA_ADDR); >>>>> >>>>> "zPCI control blocks are big endian" so don't we >>>>> need the _be_ accessors? stq_be_p() etc... >>>>> >>>> >>>> I don't think this is necessary. This is only available for target >>>> s390x, which is always big endian... cpu-all.h should define stq_p as >>>> stq_be_p for example inside the #if defined(TARGET_WORDS_BIGENDIAN). >>> >>> But if you run on little-endian host, you need to byte-swap that, >>> isn't it? >> >> It's done by the macros. They depend on the target endianess. See cpu-all.h. > > I'm confused because the description is about target endianness, > but stq_p() is about host alignment.
stq_p() is apparently also about endianess. Why would it depend on TARGET_WORDS_BIGENDIAN otherwise? Thomas