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

[David Hildenbrand <david@redhat.com>]

On 26.06.2018 17:03, Igor Mammedov wrote:
> On Tue, 19 Jun 2018 19:06:38 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 19.06.2018 17:59, Igor Mammedov wrote:
>>> On Mon, 18 Jun 2018 16:47:58 +0200
>>> David Hildenbrand <david@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).
> 
> Lets look at what we have currently:
> 
>   1.1 PC: RAM backend target page size alignment (bug fixed by 92a37a04d
>       as non aligned addr is not valid at all)
>             align = TARGET_PAGE_SIZE
> 
>       but immediately following up commits a2b257d62 / 0c0de1b68
>       overwrites it unconditionally to QEMU_VMALLOC_ALIGN for 2.2 and later
> 
>       so
>       ..v2.1
>             address/size = not multiple of TARGET_PAGE_SIZE in QEMU-2.1 is broken
>                            (a2b257d62) and we don't care
> 
>             align = TARGET_PAGE_SIZE since QEMU-2.2 binary even for older machine types
>       and later
>             align = QEMU_VMALLOC_ALIGN
> 
>   1.2 SPAPR: RAM backend. memhotplug came after 1.1 so it has
>          v2.5
>             align = QEMU_VMALLOC_ALIGN
> 
>   2.1 PC: file backend
>           v2.1
>             align = TARGET_PAGE_SIZE
>           v2.2 .. 2.11
>             align = qemu_fd_getpagesize(fd)
>           v2.12 adds one more invariant
>             align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
>             
>   2.2 SPAPR: file backend
>           v2.5..2.11
>             align = qemu_fd_getpagesize(fd)
>           v2.12
>             align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
> 
> also there is s390 kvm invariant for file backend see: file_ram_alloc()
> 
>            block->mr->align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
>            if (vkm)
>               block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
> 
> 
> to sum up they all have memory region based alignment except of v2.1 PC machine
> which is compat trick that I don't expect to be used anywhere else.
> So taking in account above and fact that it's backend property,
> I'm against of pushing it up to generic machine level, I'd try to keep
> compat hack local to PC machine along with enforce_aligned_dimm.

Problematic case is win32 qemu_anon_ram_alloc(), which does not fixup
the alignment. As far as I can see, the alignment will stay 0. So it
could happen that we have a 0 alignment, but I'll send a fix for that (I
don't think we have to worry about compat in windows here (changing
alignment from 0 to getpagesize())).

> 
> Moreover 3/4 patch where you are making memory-device.c build per target is no go,
> we are trying to minimize number of such files and not to add any without
> a good reason.

I agree, this is to be avoided.

> 
> Pushing align detection into common helper would be sufficient, 
> following could do the job with explicit comment inside that *align
> is compat hack for pc machine.
> 
>    memory_device_pre_plug(.... int *align)
>      use non null for pc-2.1 compat hack and NULL in all other cases
> 

Okay, in the first shot I'll do

pcdimm_pre_plug(... uint64_t *enforced_align ...)


-- 

Thanks,

David / dhildenb