On 19.06.2018 19:06, David Hildenbrand wrote:
> On 19.06.2018 17:59, Igor Mammedov wrote:
>> On Mon, 18 Jun 2018 16:47:58 +0200
>> David Hildenbrand <da...@redhat.com> wrote:
>>
>>> We want to handle memory device address assignment without passing
>>> compatibility parameters ("*align").
>>>
>>> As x86 and Power use different strategies to determine an alignment and
>>> we need clean support for compat handling, let's introduce an enum on
>>> the machine class level. This is the machine configuration on how to
>>> align memory devices in guest physical memory.
>>>
>>> The three introduced types represent what is being done on x86 and Power
>>> right now.
>>
>> commit message doesn't deliver purpose of the path,
>
> "We want to handle memory device address assignment without passing
> compatibility parameters ("*align")."
>
> So in order to do patch nr 4 without this, I would basically have to
> move the align parameter to pc_dimm_pre_plug, along with the code for
> "detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
> this because ...
>
>> So I'm no conviced it's necessary.
>> we probably discussed it in previous revisions but could you reiterate
>> it here WHY do you need this and 3/4
>>
>
> .. I want to get rid of the align parameter in the long run. Alignment
> is some memory device specific property that can be easily detected
> using a detection configuration (this patch). This approach looks much
> cleaner to me. This way we can use the same alignment strategy for all
> memory devices.
>
> In follow up series I want to factor out address assignment completely
> into memory_device_pre_plug(). And I also don't want to have an align
> parameter at that function. I want to avoid moving the same code around
> two times (pc.c).
It is probably best if you tell my your opinion on how address
assignment/alignment handling of pc-dimm/memory-devices is to be handled
after my rework.
My idea for the end result:
pc_dimm_pre_plug(machine, dev, errp) {
// detect and verify slot ...
memory_device_pre_plug(machine, dev, errp);
}
virtio_mem_pre_plug(machine, dev, errp) {
memory_device_pre_plug(machine, dev, errp);
}
memory_device_pre_plug(machine, md, errp) {
align = memory_device_get_align(machine, md); //handle compat
addr = md->get_addr();
addr = memory_device_get_free_addr(... addr, align ...)
md->set_addr(addr);
}
If you want *align to remain part of the function call, then
pc_dimm_pre_plug and virtio_mem_pre_plug will have to detect the
alignment themselves (e.g. using the memory region) and either
a) pass it into memory_device_pre_plug()
b) handle what memory_device_pre_plug() would do (get_free_addr ... what
we have in pc_dimm_plug() right now)
--
Thanks,
David / dhildenb
