On 4/22/21 3:33 AM, Richard Henderson wrote:
> On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
>> Not really RFC, simply that I'v to add the technical description,
>> but I'd like to know if there could be a possibility to not accept
>> this device (because I missed something) before keeping working on
>> it. So far it is only used in hobbyist boards.
>>
>> Cc: Peter Xu<pet...@redhat.com>
>> Cc: Paolo Bonzini<pbonz...@redhat.com>
>> ---
>> include/hw/misc/aliased_region.h | 87 ++++++++++++++++++++
>> hw/misc/aliased_region.c | 132 +++++++++++++++++++++++++++++++
>> MAINTAINERS | 6 ++
>> hw/misc/Kconfig | 3 +
>> hw/misc/meson.build | 1 +
>> 5 files changed, 229 insertions(+)
>> create mode 100644 include/hw/misc/aliased_region.h
>> create mode 100644 hw/misc/aliased_region.c
>
> Looks reasonable to me.
>
>
>> + subregion_bits = 64 - clz64(s->span_size - 1);
>> + s->mem.count = s->region_size >> subregion_bits;
>> + assert(s->mem.count > 1);
>> + subregion_size = 1ULL << subregion_bits;
>
> So... subregion_size = pow2floor(s->span_size)?
No... should be subregion_size = pow2ceil(s->span_size).
> Why use a floor-ish computation here and pow2ceil later in
> aliased_mr_realize?
I missed it :)
> Why use either floor or ceil as opposed to
> asserting that the size is in fact a power of 2?
Unfortunately not all memory regions are power of 2 :(
See for example the armv7m_systick device (size 0xe0).
> Why must the region be
> a power of 2, as opposed to e.g. a multiple of page_size?
I/O regions don't have to be multiple of page_size... See
AVR devices for example:
(qemu) info mtree
address-space: memory
0000000000000000-ffffffffffffffff (prio 0, i/o): system
0000000000000000-0000000000007fff (prio 0, rom): flash
0000000000800000-00000000008000ff (prio -1234, i/o): I/O
0000000000800023-0000000000800025 (prio -1000, i/o): atmega-gpio-b
0000000000800026-0000000000800028 (prio -1000, i/o): atmega-gpio-c
0000000000800029-000000000080002b (prio -1000, i/o): atmega-gpio-d
0000000000800035-0000000000800035 (prio -1000, i/o): avr-timer8-intflag
0000000000800036-0000000000800036 (prio 0, i/o): avr-timer16-intflag
0000000000800037-0000000000800037 (prio -1000, i/o): avr-timer8-intflag
000000000080003f-0000000000800041 (prio -1000, i/o): avr-eeprom
0000000000800044-0000000000800048 (prio -1000, i/o): avr-timer8
000000000080004c-000000000080004e (prio -1000, i/o): avr-spi
0000000000800060-0000000000800060 (prio -1000, i/o): avr-watchdog
0000000000800064-0000000000800064 (prio 0, i/o): avr-power
000000000080006e-000000000080006e (prio -1000, i/o): avr-timer8-intmask
000000000080006f-000000000080006f (prio 0, i/o): avr-timer16-intmask
0000000000800070-0000000000800070 (prio -1000, i/o): avr-timer8-intmask
0000000000800074-0000000000800075 (prio -1000, i/o): avr-ext-mem-ctrl
0000000000800078-000000000080007f (prio -1000, i/o): avr-adc
0000000000800080-000000000080008d (prio 0, i/o): avr-timer16
00000000008000b0-00000000008000b4 (prio -1000, i/o): avr-timer8
00000000008000b8-00000000008000bd (prio -1000, i/o): avr-twi
00000000008000c0-00000000008000c6 (prio 0, i/o): avr-usart
0000000000800100-00000000008008ff (prio 0, ram): sram
> Or just the
> most basic requirement that region_size be a multiple of span_size?
OK.
Thanks for the review! Now I need to fill the documentation part :/
Phil.
