[Top][All Lists]

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

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

From: David Hildenbrand
Subject: Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()
Date: Mon, 17 Sep 2018 12:52:02 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

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.




David / dhildenb

reply via email to

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