[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
Re: [Qemu-devel] [PATCH v2 05/20] memory-device: convert get_region_size() to get_memory_region()
Mon, 17 Sep 2018 12:52:02 +0200
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
> 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
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