Re: [RFC PATCH 2/5] system/memory: support unaligned access

[Peter Xu <peterx@redhat.com>]

Hi, Tomoyuki,

On Wed, Jan 08, 2025 at 11:58:10AM +0900, Tomoyuki HIROSE wrote:
> Happy new year, Peter.
> I had another job and was late in replying to your email, sorry.

Happy new year.  That's fine. :)

[...]

> > So.. it turns out we shouldn't drop impl.unaligned?  Because above two
> > seems to be the real user of such.  What we may want to do is:
> > 
> >    - Change above two use cases, adding impl.unaligned=true.
> > 
> >      This step should hopefully have zero effect in reality on the two
> >      devices.  One thing to mention is both of them do not look like to have
> >      an upper bound of max_access_size (either 8 which is the maximum, or
> >      not specified).
> 
> This might be a good way. In this way, we need to add 'impl.unaligned
> = true' to the xHCI Capability Register's MR. We also need to fix the

We need to keep xHCI's impl.unaligned to FALSE?  IIUC only if it's FALSE
would it start to use your new code in this series to automatically convert
the unaligned access request into one or multiple aligned accesses (without
changing xHCI's MR ops implementation, IOW resolve this in memory core).

I just had another look at your last patch:

https://lore.kernel.org/qemu-devel/20241108032952.56692-6-tomoyuki.hirose@igel.co.jp/

index d85adaca0d..f35cbe526f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3165,9 +3165,11 @@ static const MemoryRegionOps xhci_cap_ops = {
     .read = xhci_cap_read,
     .write = xhci_cap_write,
     .valid.min_access_size = 1,
-    .valid.max_access_size = 4,
+    .valid.max_access_size = 8,
+    .valid.unaligned = true,
     .impl.min_access_size = 4,
     .impl.max_access_size = 4,
+    .impl.unaligned = false,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };

I think that should keep being valid.  So "valid.unaligned = true" will
start enable unaligned accesses from the API level which will start to
follow the xHCI controller's spec, then ".impl.unaligned = false" tells the
memory core to _not_ pass unaligned accesses to MR ops, instead break them
down properly.

> MR implementation to be safe when unaligned accessing (current xHCI
> implementation does not handle unaligned accesses but the spec allows
> unaligned accesses).
> 
> In addition, maybe it would be better to document the constraint that
> the situation where 'valid.unaligned = true' and 'impl.unaligned =
> false' is not supported.

Do you perhaps mean this instead?

  valid.unaligned = FALSE && impl.unaligned == TRUE

If so, I agree.  I think we could even consider adding an assertion into
memory_region_init_io() to make sure it won't be set.

Thanks,

-- 
Peter Xu