On 29/3/25 12:30, Rakesh J wrote:
Thanks for feedback on [PATCH v1]!
I've posted v2 incorporating the suggestions:ve posted v2 incorporating
your suggestions
Paolo: You pointed out the size issue with .min_access_size = 1
and .max_access_size = 4, where bswap32 was wrong for 2-byte accesses.
I’ve fixed this with size-appropriate swaps (bswap16 for 2-byte, bswap32
for 4-byte). On the extra swap idea, I stuck with a single swap since it
aligns PCI LE with guest BE expectations without overcomplicating it—let
me know if I misunderstood.
I’ve sent [PATCH v2] incorporating changes:
1.Removed gt64120_update_pci_cfgdata_mapping() and moved initialization code
to gt64120_realize() for a simpler MByteSwap check.
2.Removed unused pci_host_data_be_ops and a misleading comment in dino.h
3.Size-specific swaps (bswap16 and bswap32)
I included bswap16 for 2-byte accesses in v2—should this be restricted
to 4-byte only (bswap32) per the spec, or does GT-64120 expect 2-byte
config swaps too? It’s a minor tweak, so I left it in v2 for now—happy
to adjust in a v3 if needed.
The new patch is available at:
https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html
<https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html>
Could you take a look at [PATCH v2] and let me know your thoughts,
especially on the 2-byte swap question? Thanks!
On Sat, Mar 29, 2025 at 4:05 PM Philippe Mathieu-Daudé
<phi...@linaro.org <mailto:phi...@linaro.org>> wrote:
On 28/3/25 18:34, Paolo Bonzini wrote:
> On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan
<bala...@eik.bme.hu <mailto:bala...@eik.bme.hu>> wrote:
>>> It should be fine. You should take into account:
>>>
>>> - the endianness produced by pci_data_read/pci_data_write
(always little
>>> endian)
>>>
>>> - the endianness expected by the guest (big endian under the
conditions in
>>> the patch)
>>>
>>> - the endianness expected by memory.c (always little endian, as
specified in
>>> gt64120_pci_data_ops)
>>>
>>> Because there is either zero or one mismatch, bswap32 is fine.
>>
>> This may worth a comment but I'm still not convinced this works
on big
>> endian host because I think pci_data_read returns val in host
endianness
>> and if you want big endian then you only need to bswap on LE
host not on
>> BE host. Was this tested on BE host and confirmed it works?
>
> Looking again at the code, there is definitely one problem: since
you have
>
> + .min_access_size = 1,
> + .max_access_size = 4,
>
> the bswap can also be bswap16 if size == 2 (and bswap32 only if
size == 4).
Per 'PCI LOCAL BUS SPECIFICATION, REV. 3.0':
'''
3.2.2.3.2. Software Generation of Configuration Transactions
Anytime a host bridge sees a full DWORD I/O write from the host
to CONFIG_ADDRESS, the bridge must latch the data into its
CONFIG_ADDRESS register. On full DWORD I/O reads to CONFIG_ADDRESS,
the bridge must return the data in CONFIG_ADDRESS. Any other types
of accesses to this address (non-DWORD) have no effect on
CONFIG_ADDRESS
and are executed as normal I/O transactions on the PCI bus. Therefore,
the only I/O Space consumed by this register is a DWORD at the given
address. I/O devices that share the same address but use BYTE or WORD
registers are not affected because their transactions will pass through
the host bridge unchanged.
'''
IIUC we don't need the bswap16.
What I'm questioning is why we need .min_access_size = 1
and not .min_access_size = 4.
