Re: [RFC PATCH 0/5] support unaligned access to xHCI Capability

[Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp>]

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