re-examining the PCI spec (3.2.2.3.2) and QEMU's PCI host bridge implementations, I agree .min_access_size = 4 is actually more correct I'll update the patch(v4) to: -Set .min_access_size = 4 in MemoryRegionOps -Remove the now-unnecessary bswap16 handling
On Mon, Mar 31, 2025 at 5:04 PM Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > 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. >
