This patch set aims to support unaligned access to xHCI Capability
Registers.

To achieve this, we introduce the emulation of an unaligned access
through multiple aligned accesses. This patch set also adds a test
device and several tests using this device to verify that the
emulation functions correctly.

Using these changes, unaligned access to xHCI Capability Registers is
now supported.

During development, I required a lot of 'MemoryRegionOps' structs with
its own read/write functions for tests. In the QEMU project, a large
number of similar functions or structs are often written in '.inc'
files. I followed this approach for the test functions but would
appreciate feedback on whether this is appropriate.

Tomoyuki HIROSE (5):
  hw/nvme/ctrl: specify the 'valid' field in MemoryRegionOps
  system/memory: support unaligned access
  hw/misc: add test device for memory access
  tests/qtest: add test for memory region access
  hw/usb/hcd-xhci: allow unaligned access to Capability Registers

 hw/misc/Kconfig                         |    4 +
 hw/misc/memaccess-testdev.c             |  197 +++
 hw/misc/meson.build                     |    1 +
 hw/nvme/ctrl.c                          |    5 +
 hw/usb/hcd-xhci.c                       |    4 +-
 include/hw/misc/memaccess-testdev.h     |   42 +
 include/hw/misc/memaccess-testdev.h.inc | 1864 +++++++++++++++++++++++
 system/memory.c                         |  147 +-
 system/physmem.c                        |    8 -
 tests/qtest/memaccess-test.c            |  598 ++++++++
 tests/qtest/meson.build                 |    9 +
 11 files changed, 2842 insertions(+), 37 deletions(-)
 create mode 100644 hw/misc/memaccess-testdev.c
 create mode 100644 include/hw/misc/memaccess-testdev.h
 create mode 100644 include/hw/misc/memaccess-testdev.h.inc
 create mode 100644 tests/qtest/memaccess-test.c

-- 
2.43.0

I would be happy to receive your comments.
ping.

On 2024/11/08 12:29, Tomoyuki HIROSE wrote:
> This patch set aims to support unaligned access to xHCI Capability
> Registers.
>
> To achieve this, we introduce the emulation of an unaligned access
> through multiple aligned accesses. This patch set also adds a test
> device and several tests using this device to verify that the
> emulation functions correctly.
>
> Using these changes, unaligned access to xHCI Capability Registers is
> now supported.
>
> During development, I required a lot of 'MemoryRegionOps' structs with
> its own read/write functions for tests. In the QEMU project, a large
> number of similar functions or structs are often written in '.inc'
> files. I followed this approach for the test functions but would
> appreciate feedback on whether this is appropriate.
>
> Tomoyuki HIROSE (5):
>    hw/nvme/ctrl: specify the 'valid' field in MemoryRegionOps
>    system/memory: support unaligned access
>    hw/misc: add test device for memory access
>    tests/qtest: add test for memory region access
>    hw/usb/hcd-xhci: allow unaligned access to Capability Registers
>
>   hw/misc/Kconfig                         |    4 +
>   hw/misc/memaccess-testdev.c             |  197 +++
>   hw/misc/meson.build                     |    1 +
>   hw/nvme/ctrl.c                          |    5 +
>   hw/usb/hcd-xhci.c                       |    4 +-
>   include/hw/misc/memaccess-testdev.h     |   42 +
>   include/hw/misc/memaccess-testdev.h.inc | 1864 +++++++++++++++++++++++
>   system/memory.c                         |  147 +-
>   system/physmem.c                        |    8 -
>   tests/qtest/memaccess-test.c            |  598 ++++++++
>   tests/qtest/meson.build                 |    9 +
>   11 files changed, 2842 insertions(+), 37 deletions(-)
>   create mode 100644 hw/misc/memaccess-testdev.c
>   create mode 100644 include/hw/misc/memaccess-testdev.h
>   create mode 100644 include/hw/misc/memaccess-testdev.h.inc
>   create mode 100644 tests/qtest/memaccess-test.c
>

On Wed, 27 Nov 2024 at 04:34, Tomoyuki HIROSE
<tomoyuki.hirose@igel.co.jp> wrote:
>
> I would be happy to receive your comments.
> ping.

Hi; this one is on my to-review list (along, sadly, with 23 other
series); I had a quick look a while back and it seemed good
(the testing support you've added looks great), but I need
to sit down and review the implementation more carefully.

The one concern I did have was the big long list of macro
invocations in the memaccess-testdev device. I wonder if it
would be more readable and more compact to fill in MemoryRegionOps
structs at runtime using loops in C code, rather than trying to do
it all at compile time with macros ?

thanks
-- PMM

Hi, thank you for your comment.

On 2024/11/27 20:23, Peter Maydell wrote:
> On Wed, 27 Nov 2024 at 04:34, Tomoyuki HIROSE
> <tomoyuki.hirose@igel.co.jp> wrote:
>> I would be happy to receive your comments.
>> ping.
> Hi; this one is on my to-review list (along, sadly, with 23 other
> series); I had a quick look a while back and it seemed good
> (the testing support you've added looks great), but I need
> to sit down and review the implementation more carefully.
>
> The one concern I did have was the big long list of macro
> invocations in the memaccess-testdev device. I wonder if it
> would be more readable and more compact to fill in MemoryRegionOps
> structs at runtime using loops in C code, rather than trying to do
> it all at compile time with macros ?

I also want to do as you say. But I don't know how to generate
MemoryRegionOps structs at runtime. We need to set read/write function
to each structs, but I don't know a simple method how to generate a
function at runtime. Sorry for my lack C knowledge. Do you know about
any method how to generate a function at runtime in C ?

> thanks
> -- PMM
Thanks,
Hirose

On Thu, 28 Nov 2024 at 06:19, Tomoyuki HIROSE
<tomoyuki.hirose@igel.co.jp> wrote:
>
> Hi, thank you for your comment.
>
> On 2024/11/27 20:23, Peter Maydell wrote:
> > On Wed, 27 Nov 2024 at 04:34, Tomoyuki HIROSE
> > <tomoyuki.hirose@igel.co.jp> wrote:
> >> I would be happy to receive your comments.
> >> ping.
> > Hi; this one is on my to-review list (along, sadly, with 23 other
> > series); I had a quick look a while back and it seemed good
> > (the testing support you've added looks great), but I need
> > to sit down and review the implementation more carefully.
> >
> > The one concern I did have was the big long list of macro
> > invocations in the memaccess-testdev device. I wonder if it
> > would be more readable and more compact to fill in MemoryRegionOps
> > structs at runtime using loops in C code, rather than trying to do
> > it all at compile time with macros ?
>
> I also want to do as you say. But I don't know how to generate
> MemoryRegionOps structs at runtime. We need to set read/write function
> to each structs, but I don't know a simple method how to generate a
> function at runtime. Sorry for my lack C knowledge. Do you know about
> any method how to generate a function at runtime in C ?

Your code doesn't generate any functions in the macros, though --
the functions are always memaccess_testdev_{read,write}_{big,little},
which are defined outside any macro.

The macros are only creating structures. Those you can populate
at runtime using normal assignments:

   for (valid_max = 1; valid_max < 16; valid_max <<= 1) {
       [other loops on valid_min, impl_max, etc, go here]
       MemoryRegionOps *memops = whatever;
       memops->read = memaccess_testdev_read_little;
       memops->write = memaccess_testdev_write_little;
       memops->valid.max_access_size = valid_max;
       etc...
   }

It just happens that for almost all MemoryRegionOps in
QEMU the contents are known at compile time and so we
make them static const at file scope.

thanks
-- PMM

On 2024/11/28 20:15, Peter Maydell wrote:

> On Thu, 28 Nov 2024 at 06:19, Tomoyuki HIROSE
> <tomoyuki.hirose@igel.co.jp> wrote:
>> Hi, thank you for your comment.
>>
>> On 2024/11/27 20:23, Peter Maydell wrote:
>>> On Wed, 27 Nov 2024 at 04:34, Tomoyuki HIROSE
>>> <tomoyuki.hirose@igel.co.jp> wrote:
>>>> I would be happy to receive your comments.
>>>> ping.
>>> Hi; this one is on my to-review list (along, sadly, with 23 other
>>> series); I had a quick look a while back and it seemed good
>>> (the testing support you've added looks great), but I need
>>> to sit down and review the implementation more carefully.
>>>
>>> The one concern I did have was the big long list of macro
>>> invocations in the memaccess-testdev device. I wonder if it
>>> would be more readable and more compact to fill in MemoryRegionOps
>>> structs at runtime using loops in C code, rather than trying to do
>>> it all at compile time with macros ?
>> I also want to do as you say. But I don't know how to generate
>> MemoryRegionOps structs at runtime. We need to set read/write function
>> to each structs, but I don't know a simple method how to generate a
>> function at runtime. Sorry for my lack C knowledge. Do you know about
>> any method how to generate a function at runtime in C ?
> Your code doesn't generate any functions in the macros, though --
> the functions are always memaccess_testdev_{read,write}_{big,little},
> which are defined outside any macro.
>
> The macros are only creating structures. Those you can populate
> at runtime using normal assignments:
>
>     for (valid_max = 1; valid_max < 16; valid_max <<= 1) {
>         [other loops on valid_min, impl_max, etc, go here]
>         MemoryRegionOps *memops = whatever;
>         memops->read = memaccess_testdev_read_little;
>         memops->write = memaccess_testdev_write_little;
>         memops->valid.max_access_size = valid_max;
>         etc...
>     }
>
> It just happens that for almost all MemoryRegionOps in
> QEMU the contents are known at compile time and so we
> make them static const at file scope.

OK, thanks! I got understand. I thought MemoryRegionOps had to be
'static const' .
I will try to improve code so that it does not require the use of
memaccess-testdev.h.inc .

thanks,
Tomoyuki HIROSE

On Fri, 29 Nov 2024 at 03:33, Tomoyuki HIROSE
<tomoyuki.hirose@igel.co.jp> wrote:
> OK, thanks! I got understand. I thought MemoryRegionOps had to be
> 'static const' .
> I will try to improve code so that it does not require the use of
> memaccess-testdev.h.inc .

Great. The other thing I thought of this weekend is that
we should document the behaviour in docs/devel/memory.rst.
We could have a new section there that describes how the
core memory code synthesizes accesses that are permitted
by the .valid settings but not handled by the .impl
settings. That way device model authors can know what
happens without having to read the source code.

thanks
-- PMM

On 2024/12/02 23:17, Peter Maydell wrote:
> On Fri, 29 Nov 2024 at 03:33, Tomoyuki HIROSE
> <tomoyuki.hirose@igel.co.jp> wrote:
>> OK, thanks! I got understand. I thought MemoryRegionOps had to be
>> 'static const' .
>> I will try to improve code so that it does not require the use of
>> memaccess-testdev.h.inc .
> Great. The other thing I thought of this weekend is that
> we should document the behaviour in docs/devel/memory.rst.
> We could have a new section there that describes how the
> core memory code synthesizes accesses that are permitted
> by the .valid settings but not handled by the .impl
> settings. That way device model authors can know what
> happens without having to read the source code.
OK, I will also write the doc as I can.
> thanks
> -- PMM
thanks,
Tomoyuki HIROSE