On 26.06.2018 17:03, Igor Mammedov wrote:
> On Tue, 19 Jun 2018 19:06:38 +0200
> David Hildenbrand <da...@redhat.com> 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).
>
> 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
