Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[David Hildenbrand]

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 <address@hidden> 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