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

Reply via email to