qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Tomoyuki HIROSE
Subject: Re: [RFC PATCH 0/5] support unaligned access to xHCI Capability
Date: Fri, 29 Nov 2024 12:33:35 +0900
User-agent: Mozilla Thunderbird

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]