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 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> 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> > 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. > >
