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.
If there is no alignment problem, doesn't using stq_p() make code
harder to review?
