Re: [PATCH v7 4/4] s390: do not call memory_region_allocate_system_memory() multiple times

[Igor Mammedov]

On Thu, 26 Sep 2019 07:52:35 +0800
Peter Xu <address@hidden> wrote:

> On Wed, Sep 25, 2019 at 01:51:05PM +0200, Igor Mammedov wrote:
> > On Wed, 25 Sep 2019 11:27:00 +0800
> > Peter Xu <address@hidden> wrote:
> >   
> > > On Tue, Sep 24, 2019 at 10:47:51AM -0400, Igor Mammedov wrote:  
> > > > s390 was trying to solve limited KVM memslot size issue by abusing
> > > > memory_region_allocate_system_memory(), which breaks API contract
> > > > where the function might be called only once.
> > > > 
> > > > Beside an invalid use of API, the approach also introduced migration
> > > > issue, since RAM chunks for each KVM_SLOT_MAX_BYTES are transferred in
> > > > migration stream as separate RAMBlocks.
> > > > 
> > > > After discussion [1], it was agreed to break migration from older
> > > > QEMU for guest with RAM >8Tb (as it was relatively new (since 2.12)
> > > > and considered to be not actually used downstream).
> > > > Migration should keep working for guests with less than 8TB and for
> > > > more than 8TB with QEMU 4.2 and newer binary.
> > > > In case user tries to migrate more than 8TB guest, between incompatible
> > > > QEMU versions, migration should fail gracefully due to non-exiting
> > > > RAMBlock ID or RAMBlock size mismatch.
> > > > 
> > > > Taking in account above and that now KVM code is able to split too
> > > > big MemorySection into several memslots, partially revert commit
> > > >  (bb223055b s390-ccw-virtio: allow for systems larger that 7.999TB)
> > > > and use kvm_set_max_memslot_size() to set KVMSlot size to
> > > > KVM_SLOT_MAX_BYTES.
> > > > 
> > > > 1) [PATCH RFC v2 4/4] s390: do not call  
> > > > memory_region_allocate_system_memory() multiple times
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>    
> > > 
> > > Acked-by: Peter Xu <address@hidden>
> > > 
> > > IMHO it would be good to at least mention bb223055b9 in the commit
> > > message even if not with a "Fixed:" tag.  May be amended during commit
> > > if anyone prefers.  
> > 
> > /me confused, bb223055b9 is mentioned in commit message  
> 
> I'm sorry, I overlooked that.
> 
> >    
> > > Also, this only applies the split limitation to s390.  Would that be a
> > > good thing to some other archs as well?  
> > 
> > Don't we have the similar bitmap size issue in KVM for other archs?  
> 
> Yes I thought we had.  So I feel like it would be good to also allow
> other archs to support >8TB mem as well.  Thanks,
Another question, Is there another archs with that much RAM that are
available/used in real life (if not I'd wait for demand to arise first)?

If we are to generalize it to other targets, then instead of using
arbitrary memslot max size per target, we could just hardcode or get
from KVM, max supported size of bitmap and use that to calculate
kvm_max_slot_size depending on target page size.

Then there wouldn't be need for having machine specific code
to care about it and pick/set arbitrary values.

Another aspect to think about if we are to enable it for
other targets is memslot accounting. It doesn't affect s390
but other targets that support memory hotplug now assume 1:1
relation between memoryregion:memslot, which currently holds
true but would need to amended in case split is enabled there.