Re: [RFC PATCH v2 1/7] hw/misc: Add device to help managing aliased memory regions

[Philippe Mathieu-Daudé]

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<peterx@redhat.com>
>> Cc: Paolo Bonzini<pbonzini@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.