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

[Tomoyuki HIROSE]

On 2025/01/09 1:50, Peter Xu wrote:
> 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).

Yes, we need to keep it to 'false' because xHCI's MR implementation
does not supported unaligned accesses.

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

I'm sorry if I've misunderstood, but are the following understandings
correct?:
- Need to merge my patch that converts an unaligned access to aligned
  accesses.
- Need to add 'impl.unaligned = true' in the following two places.
  - hw/xtensa/mx_pic.c
  - hw/ssi/npcm7xx_fiu.c
- Add an assertion that to check for invalid patterns, additionally.

Thanks,
Tomoyuki HIROSE