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

[Richard Henderson]

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)?

Why use a floor-ish computation here and pow2ceil later in aliased_mr_realize? 
 Why use either floor or ceil as opposed to asserting that the size is in fact 
a power of 2?  Why must the region be a power of 2, as opposed to e.g. a 
multiple of page_size?  Or just the most basic requirement that region_size be 
a multiple of span_size?


r~