Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support

["kwangwoo.lee@sk.com" <kwangwoo.lee@sk.com>]

Hi Igor,

Thank you for your guide to split the hotplug patch.
Currently I'm looking at the Linux kernel codes related with huge page size.

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Tuesday, August 02, 2016 9:18 PM
> To: Peter Maydell
> Cc: Xiao Guangrong; Eduardo Habkost; Michael S. Tsirkin; QEMU Developers; 정우석(CHUNG WOO SUK) MS SW;
> qemu-arm; Shannon Zhao; Shannon Zhao; Paolo Bonzini; 김현철(KIM HYUNCHUL) MS SW; 이광우(LEE KWANGWOO) MS
> SW; Richard Henderson
> Subject: Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support
> 
> On Tue, 2 Aug 2016 08:59:46 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 1 August 2016 at 10:14, Igor Mammedov <imammedo@redhat.com> wrote:
> > > On Mon, 1 Aug 2016 09:13:34 +0100
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > >> On 1 August 2016 at 08:46, Igor Mammedov <imammedo@redhat.com> wrote:
> > >> > Base alignment comes from max supported hugepage size,
> > >>
> > >> Max hugepage size for any host? (if so, should be defined
> > >> in a common header somewhere)
> > >> Max hugepage size for ARM hosts? (if so, why is TCG
> > >> different from KVM?, and should still be in a common
> > >> header somewhere)
> > > It's the same for TCG but it probably doesn't matter much there,
> > > main usage is to provide better performance with KVM.
> > >
> > > So I'd say it's host depended (for x86 it's 1Gb),
> > > probably other values for ARM and PPC
> >
> > We probably don't want to make the memory layout depend
> > on the host architecture, though :-(
> Important here is not change it dynamically so it won't
> break migration.
> Otherwise it could be a value that makes a sense for
> the use-case where performance matters most, i.e. KVM
> which makes us to derive value from max supported page
> size for a KVM host (meaning guest is the same arch)
> 
> In case of x86 value is constant hardcoded in target
> specific code which applies to both KVM and TCG.
> 
> Perhaps there is a better way to handle it which I just
> don't see.
> 
> >
> > >>
> > >> > while
> > >> > size alignment should depend on backend's page size
> > >>
> > >> Which page size do you have in mind here? TARGET_PAGE_SIZE
> > >> is often not the right answer, since it doesn't
> > >> correspond either to the actual page size being used
> > >> by the host kernel or to the actual page size used
> > >> by the guest kernel...
> > > alignment comes from here: memory_region_get_alignment()
> > >
> > > exec:c
> > >    MAX(page_size, QEMU_VMALLOC_ALIGN)
> > > so it's either backend's page size or a min chunk QEMU
> > > allocates memory to make KVM/valgrind/whatnot happy.
> >
> > Since that's always larger than TARGET_PAGE_SIZE
> > why are we checking for an alignment here that's
> > not actually sufficient to make things work?
> You mean following hunk:
> 
> +        if (QEMU_ALIGN_UP(machine->maxram_size,
> +                     TARGET_PAGE_SIZE) != machine->maxram_size) {
> 
> It's a bit late to fix for x86 without breaking CLI,
> side effect would be inability to hotplug upto maxmem if maxmem
> is misaligned wrt used backend page size
> 
> ARCH_VIRT_HOTPLUG_MEM_ALIGN should be used instead of TARGET_PAGE_SIZE
> 
> PS:
> I haven't reviewed series yet, but I'd split this patch in 3
> to make review easier
>   1st - introduce machine level hotplug hooks
>   2nd - add MemoryHotplugState to VirtMachineState and initialize it
>   3rd - add virt_dimm_plug() handler

OK. I'll try to split it.

> >
> > thanks
> > -- PMM
> >

Best Regards,
Kwangwoo Lee