Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()

[David Hildenbrand]

Am 14.09.18 um 16:34 schrieb Igor Mammedov:
> On Thu, 13 Sep 2018 14:20:25 +0200
> Igor Mammedov <address@hidden> wrote:
> 
>> On Wed, 29 Aug 2018 17:36:09 +0200
>> David Hildenbrand <address@hidden> wrote:
>>
>>> To factor out plugging and unplugging of memory device we need access to
>>> the memory region. So let's replace get_region_size() by
>>> get_memory_region().
> this part isn't clear and doesn't really answer "why" patch's been written
> and how it will really help.
> 

Sure, I can add more details.

>>>
>>> If any memory device will in the future have multiple memory regions
>>> that make up the total region size, it can simply use a wrapping memory
>>> region to embed the other ones.> It might be better to document expectation 
>>> as part of
get_memory_region() doc comment.

Yes, I will extend that, too.

> 
> 
> I'm not really getting reasoning behind this patch.
>  * Why get_region_size() doesn't work for you?
>    benefits I see is that's one less get_foo_size() callback,
>    so it becomes less confusing

get_region_size() size is no longer necessary when factoring
pre_plug/plug_unplug out.

Especially we need in addition of the size:

1. the alignment of the region (patch #9)
2. the region for memory_region_add_subregion() to plug it (patch #10)
3. the region for memory_region_del_subregion() to unplug it (patch #11)

> 
>  * I get we might need access to memory region at MemoryDeviceClass level,
>    but why are you keeping PCDIMMDeviceClass::get_memory_region()?
>    I'd expect the later being generalized (moved) to MemoryDeviceClass
>    so we would end up with one level indirection that we have now.
> 
Yes, this is the main reason behind it. I guess getting rid of
PCDIMMDeviceClass::get_memory_region() makes it clear that this is the
next step of factoring out.

I could turn this patch into said "factor get_memory_region() out" patch
and keep get_region_size() for now.

Thanks!

-- 

Thanks,

David / dhildenb