Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Cornelia Huck]

On Thu, 22 Feb 2018 14:29:09 -0500
Matthew Rosato  wrote:

> On 02/22/2018 06:13 AM, Christian Borntraeger wrote:
> > 
> > 
> > On 02/21/2018 06:39 PM, Cornelia Huck wrote:  
> >> On Tue, 20 Feb 2018 16:05:54 +0100
> >> David Hildenbrand  wrote:
> >>  
> >>> On 20.02.2018 15:57, Cornelia Huck wrote:  
>  On Tue, 20 Feb 2018 13:16:37 +0100
>  David Hildenbrand  wrote:
>  
> > On 20.02.2018 13:05, Christian Borntraeger wrote:
> >>
> >>
> >> On 02/19/2018 06:42 PM, David Hildenbrand wrote:  
> >>> From an architecture point of view, nothing can be mapped into the 
> >>> address
> >>> space on s390x. All there is is memory. Therefore there is also not 
> >>> really
> >>> an interface to communicate such information to the guest. All we can 
> >>> do is
> >>> specify the maximum ram address and guests can probe in that range if
> >>> memory is available and usable (TPROT).  
> >>
> >> In fact there is an interface in SCLP that describes the memory sizes 
> >> (maximum in 
> >> read scp info) and the details (read_storage_element0_info).  I am 
> >> asking myself
> >> if we should re-introduce read_storage_element_info and use that to 
> >> avoid tprot  
> >
> > Yes, we could do that (basically V1 of this patch) but have to glue it
> > to the a compatibility machine then.
> 
>  Actually, this makes quite a bit of sense (introduce the interface for
>  everyone in 2.12 and turn it off in compat machines).
> >>>
> >>> Jup, either 2.12 or 2.13, no need to hurry.
> >>>  
> 
>  Does real hardware have configurations where you can get the memory
>  sizes, but not the attach/deattach support? (Hardware with the feature,
>  but no standby memory defined?)
> > 
> > We have different sclp facilities for attach/detach and information, so
> > we can implement that. 
> > 
> >   
> >>>
> >>> I would guess that "0" for standby memory is valid but only people with
> >>> access to documentation can answer that :)  
> >>
> >> So, should we go with this patch now and re-introduce the read
> >> functions if the above is indeed true?  
> > 
> > Yes, go with this patch. Right now Linux guests will not make use of that, 
> > so
> > we can re-add that if it turns out to be useful for future guests.
> > 
> > 
> > 
> > Matt, last chance to complain with reasons why we want to keep the current 
> > standby memory
> > solution in its current form. (Or please ack the patch if you agree)  
> 
> Nope, this makes sense given its incompatibility w/ the common layer.  I
> also agree with the prior comment that, should we revisit this feature
> in the future, it should probably be via an s390-specific interface.
> 
> Acked-by: Matthew Rosato 

Thanks, applied (with the discussed description tweak) to s390-next.

We should also mention this in the changelog once this hits master.
I'll try to remember to do that.

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Matthew Rosato]

On 02/22/2018 06:13 AM, Christian Borntraeger wrote:
> 
> 
> On 02/21/2018 06:39 PM, Cornelia Huck wrote:
>> On Tue, 20 Feb 2018 16:05:54 +0100
>> David Hildenbrand  wrote:
>>
>>> On 20.02.2018 15:57, Cornelia Huck wrote:
 On Tue, 20 Feb 2018 13:16:37 +0100
 David Hildenbrand  wrote:
   
> On 20.02.2018 13:05, Christian Borntraeger wrote:  
>>
>>
>> On 02/19/2018 06:42 PM, David Hildenbrand wrote:
>>> From an architecture point of view, nothing can be mapped into the 
>>> address
>>> space on s390x. All there is is memory. Therefore there is also not 
>>> really
>>> an interface to communicate such information to the guest. All we can 
>>> do is
>>> specify the maximum ram address and guests can probe in that range if
>>> memory is available and usable (TPROT).
>>
>> In fact there is an interface in SCLP that describes the memory sizes 
>> (maximum in 
>> read scp info) and the details (read_storage_element0_info).  I am 
>> asking myself
>> if we should re-introduce read_storage_element_info and use that to 
>> avoid tprot
>
> Yes, we could do that (basically V1 of this patch) but have to glue it
> to the a compatibility machine then.  

 Actually, this makes quite a bit of sense (introduce the interface for
 everyone in 2.12 and turn it off in compat machines).  
>>>
>>> Jup, either 2.12 or 2.13, no need to hurry.
>>>

 Does real hardware have configurations where you can get the memory
 sizes, but not the attach/deattach support? (Hardware with the feature,
 but no standby memory defined?)  
> 
> We have different sclp facilities for attach/detach and information, so
> we can implement that. 
> 
> 
>>>
>>> I would guess that "0" for standby memory is valid but only people with
>>> access to documentation can answer that :)
>>
>> So, should we go with this patch now and re-introduce the read
>> functions if the above is indeed true?
> 
> Yes, go with this patch. Right now Linux guests will not make use of that, so
> we can re-add that if it turns out to be useful for future guests.
> 
> 
> 
> Matt, last chance to complain with reasons why we want to keep the current 
> standby memory
> solution in its current form. (Or please ack the patch if you agree)

Nope, this makes sense given its incompatibility w/ the common layer.  I
also agree with the prior comment that, should we revisit this feature
in the future, it should probably be via an s390-specific interface.

Acked-by: Matthew Rosato 

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Christian Borntraeger]

On 02/21/2018 06:39 PM, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 16:05:54 +0100
> David Hildenbrand  wrote:
> 
>> On 20.02.2018 15:57, Cornelia Huck wrote:
>>> On Tue, 20 Feb 2018 13:16:37 +0100
>>> David Hildenbrand  wrote:
>>>   
 On 20.02.2018 13:05, Christian Borntraeger wrote:  
>
>
> On 02/19/2018 06:42 PM, David Hildenbrand wrote:
>> From an architecture point of view, nothing can be mapped into the 
>> address
>> space on s390x. All there is is memory. Therefore there is also not 
>> really
>> an interface to communicate such information to the guest. All we can do 
>> is
>> specify the maximum ram address and guests can probe in that range if
>> memory is available and usable (TPROT).
>
> In fact there is an interface in SCLP that describes the memory sizes 
> (maximum in 
> read scp info) and the details (read_storage_element0_info).  I am asking 
> myself
> if we should re-introduce read_storage_element_info and use that to avoid 
> tprot

 Yes, we could do that (basically V1 of this patch) but have to glue it
 to the a compatibility machine then.  
>>>
>>> Actually, this makes quite a bit of sense (introduce the interface for
>>> everyone in 2.12 and turn it off in compat machines).  
>>
>> Jup, either 2.12 or 2.13, no need to hurry.
>>
>>>
>>> Does real hardware have configurations where you can get the memory
>>> sizes, but not the attach/deattach support? (Hardware with the feature,
>>> but no standby memory defined?)  

We have different sclp facilities for attach/detach and information, so
we can implement that. 


>>
>> I would guess that "0" for standby memory is valid but only people with
>> access to documentation can answer that :)
> 
> So, should we go with this patch now and re-introduce the read
> functions if the above is indeed true?

Yes, go with this patch. Right now Linux guests will not make use of that, so
we can re-add that if it turns out to be useful for future guests.



Matt, last chance to complain with reasons why we want to keep the current 
standby memory
solution in its current form. (Or please ack the patch if you agree)

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Cornelia Huck]

On Tue, 20 Feb 2018 16:05:54 +0100
David Hildenbrand  wrote:

> On 20.02.2018 15:57, Cornelia Huck wrote:
> > On Tue, 20 Feb 2018 13:16:37 +0100
> > David Hildenbrand  wrote:
> >   
> >> On 20.02.2018 13:05, Christian Borntraeger wrote:  
> >>>
> >>>
> >>> On 02/19/2018 06:42 PM, David Hildenbrand wrote:
>  From an architecture point of view, nothing can be mapped into the 
>  address
>  space on s390x. All there is is memory. Therefore there is also not 
>  really
>  an interface to communicate such information to the guest. All we can do 
>  is
>  specify the maximum ram address and guests can probe in that range if
>  memory is available and usable (TPROT).
> >>>
> >>> In fact there is an interface in SCLP that describes the memory sizes 
> >>> (maximum in 
> >>> read scp info) and the details (read_storage_element0_info).  I am asking 
> >>> myself
> >>> if we should re-introduce read_storage_element_info and use that to avoid 
> >>> tprot
> >>
> >> Yes, we could do that (basically V1 of this patch) but have to glue it
> >> to the a compatibility machine then.  
> > 
> > Actually, this makes quite a bit of sense (introduce the interface for
> > everyone in 2.12 and turn it off in compat machines).  
> 
> Jup, either 2.12 or 2.13, no need to hurry.
> 
> > 
> > Does real hardware have configurations where you can get the memory
> > sizes, but not the attach/deattach support? (Hardware with the feature,
> > but no standby memory defined?)  
> 
> I would guess that "0" for standby memory is valid but only people with
> access to documentation can answer that :)

So, should we go with this patch now and re-introduce the read
functions if the above is indeed true?

> 
> >> Interesting, didn't know about that. Will rephrase then to
> >>
> >> "While the hypervisor can deny to online an increment, all increments
> >> have to be predefined and there is no way of telling the guest about a
> >> newly "hotplugged" increment."  
> > 
> > Rephrase which part? :)  
> 
> "And nobody can really hinder it from doing so."

OK.

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[David Hildenbrand]

On 20.02.2018 15:57, Cornelia Huck wrote:
> On Tue, 20 Feb 2018 13:16:37 +0100
> David Hildenbrand  wrote:
> 
>> On 20.02.2018 13:05, Christian Borntraeger wrote:
>>>
>>>
>>> On 02/19/2018 06:42 PM, David Hildenbrand wrote:  
 From an architecture point of view, nothing can be mapped into the address
 space on s390x. All there is is memory. Therefore there is also not really
 an interface to communicate such information to the guest. All we can do is
 specify the maximum ram address and guests can probe in that range if
 memory is available and usable (TPROT).  
>>>
>>> In fact there is an interface in SCLP that describes the memory sizes 
>>> (maximum in 
>>> read scp info) and the details (read_storage_element0_info).  I am asking 
>>> myself
>>> if we should re-introduce read_storage_element_info and use that to avoid 
>>> tprot  
>>
>> Yes, we could do that (basically V1 of this patch) but have to glue it
>> to the a compatibility machine then.
> 
> Actually, this makes quite a bit of sense (introduce the interface for
> everyone in 2.12 and turn it off in compat machines).

Jup, either 2.12 or 2.13, no need to hurry.

> 
> Does real hardware have configurations where you can get the memory
> sizes, but not the attach/deattach support? (Hardware with the feature,
> but no standby memory defined?)

I would guess that "0" for standby memory is valid but only people with
access to documentation can answer that :)

>> Interesting, didn't know about that. Will rephrase then to
>>
>> "While the hypervisor can deny to online an increment, all increments
>> have to be predefined and there is no way of telling the guest about a
>> newly "hotplugged" increment."
> 
> Rephrase which part? :)

"And nobody can really hinder it from doing so."

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Cornelia Huck]

On Tue, 20 Feb 2018 13:16:37 +0100
David Hildenbrand  wrote:

> On 20.02.2018 13:05, Christian Borntraeger wrote:
> > 
> > 
> > On 02/19/2018 06:42 PM, David Hildenbrand wrote:  
> >> From an architecture point of view, nothing can be mapped into the address
> >> space on s390x. All there is is memory. Therefore there is also not really
> >> an interface to communicate such information to the guest. All we can do is
> >> specify the maximum ram address and guests can probe in that range if
> >> memory is available and usable (TPROT).  
> > 
> > In fact there is an interface in SCLP that describes the memory sizes 
> > (maximum in 
> > read scp info) and the details (read_storage_element0_info).  I am asking 
> > myself
> > if we should re-introduce read_storage_element_info and use that to avoid 
> > tprot  
> 
> Yes, we could do that (basically V1 of this patch) but have to glue it
> to the a compatibility machine then.

Actually, this makes quite a bit of sense (introduce the interface for
everyone in 2.12 and turn it off in compat machines).

Does real hardware have configurations where you can get the memory
sizes, but not the attach/deattach support? (Hardware with the feature,
but no standby memory defined?)

> 
> > in the guests. Anyway that is future but maybe change your "TPROT" to sclp 
> > + tprot.  
> 
> Yes, makes sense.
> 
> > 
> >   
> >>
> >> Also memory hotplug is strange. The guest can decide at some point in
> >> time to add / remove memory in some range. And nobody can really hinder it
> >> from doing so.  
> > 
> > The host can reject to online an increment. For example on LPAR this is 
> > used as
> > a poor mans overcommitment. You can online prepared standy memory if there 
> > is
> > enough memory left.  
> 
> Interesting, didn't know about that. Will rephrase then to
> 
> "While the hypervisor can deny to online an increment, all increments
> have to be predefined and there is no way of telling the guest about a
> newly "hotplugged" increment."

Rephrase which part? :)

> 
> > 
> >  So if we specify right now e.g.  
> >> -m 2G,slots=2,maxmem=20G
> >> An ordinary fedora guest will happily online (hotplug) all memory,
> >> resulting in a guest consuming 20G. So it really behaves rather like
> >> -m 22G
> >> There is no way to hotplug memory from the outside like on other
> >> architectures. This is of course bad for upper management layers.  
> > 
> > This is the main issue with the current code.  
> 
> 

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[David Hildenbrand]

On 20.02.2018 13:05, Christian Borntraeger wrote:
> 
> 
> On 02/19/2018 06:42 PM, David Hildenbrand wrote:
>> From an architecture point of view, nothing can be mapped into the address
>> space on s390x. All there is is memory. Therefore there is also not really
>> an interface to communicate such information to the guest. All we can do is
>> specify the maximum ram address and guests can probe in that range if
>> memory is available and usable (TPROT).
> 
> In fact there is an interface in SCLP that describes the memory sizes 
> (maximum in 
> read scp info) and the details (read_storage_element0_info).  I am asking 
> myself
> if we should re-introduce read_storage_element_info and use that to avoid 
> tprot

Yes, we could do that (basically V1 of this patch) but have to glue it
to the a compatibility machine then.

> in the guests. Anyway that is future but maybe change your "TPROT" to sclp + 
> tprot.

Yes, makes sense.

> 
> 
>>
>> Also memory hotplug is strange. The guest can decide at some point in
>> time to add / remove memory in some range. And nobody can really hinder it
>> from doing so.
> 
> The host can reject to online an increment. For example on LPAR this is used 
> as
> a poor mans overcommitment. You can online prepared standy memory if there is
> enough memory left.

Interesting, didn't know about that. Will rephrase then to

"While the hypervisor can deny to online an increment, all increments
have to be predefined and there is no way of telling the guest about a
newly "hotplugged" increment."

> 
>  So if we specify right now e.g.
>> -m 2G,slots=2,maxmem=20G
>> An ordinary fedora guest will happily online (hotplug) all memory,
>> resulting in a guest consuming 20G. So it really behaves rather like
>> -m 22G
>> There is no way to hotplug memory from the outside like on other
>> architectures. This is of course bad for upper management layers.
> 
> This is the main issue with the current code.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Christian Borntraeger]

On 02/19/2018 06:42 PM, David Hildenbrand wrote:
> From an architecture point of view, nothing can be mapped into the address
> space on s390x. All there is is memory. Therefore there is also not really
> an interface to communicate such information to the guest. All we can do is
> specify the maximum ram address and guests can probe in that range if
> memory is available and usable (TPROT).

In fact there is an interface in SCLP that describes the memory sizes (maximum 
in 
read scp info) and the details (read_storage_element0_info).  I am asking myself
if we should re-introduce read_storage_element_info and use that to avoid tprot
in the guests. Anyway that is future but maybe change your "TPROT" to sclp + 
tprot.


> 
> Also memory hotplug is strange. The guest can decide at some point in
> time to add / remove memory in some range. And nobody can really hinder it
> from doing so.

The host can reject to online an increment. For example on LPAR this is used as
a poor mans overcommitment. You can online prepared standy memory if there is
enough memory left.

 So if we specify right now e.g.
> -m 2G,slots=2,maxmem=20G
> An ordinary fedora guest will happily online (hotplug) all memory,
> resulting in a guest consuming 20G. So it really behaves rather like
> -m 22G
> There is no way to hotplug memory from the outside like on other
> architectures. This is of course bad for upper management layers.

This is the main issue with the current code.

> 
> As the guest can create/delete memory regions while it is running, of
> course migration support is not available and tricky to implement.
> 
> With virtualization, it is different. We might want to map something
> into guest address space (e.g. fake DAX devices) and not detect it
> automatically as memory. So we really want to use the maxmem and slots
> parameter just like on all other architectures. Such devices will have
> to expose the applicable memory range themselves. To finally be able to
> provide memory hotplug to guests, we will need a new paravirtualized
> interface to do that (e.g. something into the direction of virtio-mem).
> 
> This implies, that maxmem cannot be used for s390x memory hotplug
> anymore and has to go. This simplifies the code quite a bit.
> 
> As migration support is not working, this change cannot really break
> migration as guests without slots and maxmem don't see the SCLP
> features. Also, the ram size calcualtion does not change.
> 
> Signed-off-by: David Hildenbrand 


code looks sane.

Reviewed-by: Christian Borntraeger 


> ---
>  hw/s390x/sclp.c | 310 
> +---
>  include/hw/s390x/sclp.h |  25 
>  target/s390x/cpu.h  |   1 -
>  3 files changed, 5 insertions(+), 331 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 276972b59f..047d577313 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -15,9 +15,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
> -#include "exec/memory.h"
>  #include "sysemu/sysemu.h"
> -#include "exec/address-spaces.h"
>  #include "hw/boards.h"
>  #include "hw/s390x/sclp.h"
>  #include "hw/s390x/event-facility.h"
> @@ -57,10 +55,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
>  ReadInfo *read_info = (ReadInfo *) sccb;
>  MachineState *machine = MACHINE(qdev_get_machine());
> -sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
>  int cpu_count;
>  int rnsize, rnmax;
> -int slots = MIN(machine->ram_slots, s390_get_memslot_count());
>  IplParameterBlock *ipib = s390_ipl_get_iplb();
> 
>  /* CPU information */
> @@ -80,36 +76,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>  SCLP_HAS_IOA_RECONFIG);
> 
> -/* Memory Hotplug is only supported for the ccw machine type */
> -if (mhd) {
> -mhd->standby_subregion_size = MEM_SECTION_SIZE;
> -/* Deduct the memory slot already used for core */
> -if (slots > 0) {
> -while ((mhd->standby_subregion_size * (slots - 1)
> -< mhd->standby_mem_size)) {
> -mhd->standby_subregion_size = mhd->standby_subregion_size << 
> 1;
> -}
> -}
> -/*
> - * Initialize mapping of guest standby memory sections indicating 
> which
> - * are and are not online. Assume all standby memory begins offline.
> - */
> -if (mhd->standby_state_map == 0) {
> -if (mhd->standby_mem_size % mhd->standby_subregion_size) {
> -mhd->standby_state_map = g_malloc0((mhd->standby_mem_size /
> - mhd->standby_subregion_size + 
> 1) *
> - (mhd->standby_subregion_size /
> - 

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[David Hildenbrand]

On 20.02.2018 12:18, Christian Borntraeger wrote:
> Given that we have trouble modeling this properly I somewhat like the idea.
> We should have never folded this into the common code memory hotplug 
> infrastructure as it is different. Instead we really should have provided
> a different interface and we should have build it with migration support. 
> 
> If we need it again in the future, we can certainly add it back via an 
> s390 specific interface.
> 

Thinking out load (stuff we could to in the future):

We could model ordinary memory (-m XG) using standby memory. But instead
of creating and deleting memory regions, work on a single huge one (just
as now). (using compat machines to control behavior)

What would be the optimization? When the guest tells us to unassign
memory, we can simply madv(free) that memory.

That means if the guest unplugs(offlines) memory, we can actually free
the backing storage. This way it acts somewhat like a balloon - but also
gets rid of struct pages in the guest.

Of course, this would still not be real memory hot(un)plug, as there is
no way to control it from the outside. Just a possible optimization.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Christian Borntraeger]

On 02/20/2018 12:18 PM, Christian Borntraeger wrote:
> Given that we have trouble modeling this properly I somewhat like the idea.
> We should have never folded this into the common code memory hotplug 
> infrastructure as it is different. Instead we really should have provided
> a different interface and we should have build it with migration support. 
> 
> If we need it again in the future, we can certainly add it back via an 
> s390 specific interface.
> 
> 
> On 02/19/2018 06:42 PM, David Hildenbrand wrote:
>> From an architecture point of view, nothing can be mapped into the address
>> space on s390x. All there is is memory. Therefore there is also not really
>> an interface to communicate such information to the guest. All we can do is
>> specify the maximum ram address and guests can probe in that range if
>> memory is available and usable (TPROT).
>>
>> Also memory hotplug is strange. The guest can decide at some point in
>> time to add / remove memory in some range. And nobody can really hinder it
>> from doing so. So if we specify right now e.g.
>> -m 2G,slots=2,maxmem=20G
>> An ordinary fedora guest will happily online (hotplug) all memory,
>> resulting in a guest consuming 20G. So it really behaves rather like
>> -m 22G
>> There is no way to hotplug memory from the outside like on other
>> architectures. This is of course bad for upper management layers.
>>
>> As the guest can create/delete memory regions while it is running, of
>> course migration support is not available and tricky to implement.
>>
>> With virtualization, it is different. We might want to map something
>> into guest address space (e.g. fake DAX devices) and not detect it
>> automatically as memory. So we really want to use the maxmem and slots
>> parameter just like on all other architectures. Such devices will have
>> to expose the applicable memory range themselves. To finally be able to
>> provide memory hotplug to guests, we will need a new paravirtualized
>> interface to do that (e.g. something into the direction of virtio-mem).
>>
>> This implies, that maxmem cannot be used for s390x memory hotplug
>> anymore and has to go. This simplifies the code quite a bit.
>>
>> As migration support is not working, this change cannot really break
>> migration as guests without slots and maxmem don't see the SCLP
>> features. Also, the ram size calcualtion does not change.
>>
>> Signed-off-by: David Hildenbrand 
> [...]
> 
> looks mostly sane. Give me some more time to review. 
> 
>> @@ -540,9 +302,6 @@ static void sclp_memory_init(SCLPDevice *sclp)
>>  {
>>  MachineState *machine = MACHINE(qdev_get_machine());
>>  ram_addr_t initial_mem = machine->ram_size;
>> -ram_addr_t max_mem = machine->maxram_size;
>> -ram_addr_t standby_mem = max_mem - initial_mem;
>> -ram_addr_t pad_mem = 0;
>>  int increment_size = 20;
>>
>>  /* The storage increment size is a multiple of 1M and is a power of 2.
>> @@ -552,34 +311,14 @@ static void sclp_memory_init(SCLPDevice *sclp)
>>  while ((initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>>  increment_size++;
>>  }
> 
> 
> In theory we could now be more fine grained (as we no longer expose the 
> increments)
> but this would break migration so better try now to be clever. (e.g. with
> compat machines)
>   try NOT of course.

Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[Christian Borntraeger]

Given that we have trouble modeling this properly I somewhat like the idea.
We should have never folded this into the common code memory hotplug 
infrastructure as it is different. Instead we really should have provided
a different interface and we should have build it with migration support. 

If we need it again in the future, we can certainly add it back via an 
s390 specific interface.


On 02/19/2018 06:42 PM, David Hildenbrand wrote:
> From an architecture point of view, nothing can be mapped into the address
> space on s390x. All there is is memory. Therefore there is also not really
> an interface to communicate such information to the guest. All we can do is
> specify the maximum ram address and guests can probe in that range if
> memory is available and usable (TPROT).
> 
> Also memory hotplug is strange. The guest can decide at some point in
> time to add / remove memory in some range. And nobody can really hinder it
> from doing so. So if we specify right now e.g.
> -m 2G,slots=2,maxmem=20G
> An ordinary fedora guest will happily online (hotplug) all memory,
> resulting in a guest consuming 20G. So it really behaves rather like
> -m 22G
> There is no way to hotplug memory from the outside like on other
> architectures. This is of course bad for upper management layers.
> 
> As the guest can create/delete memory regions while it is running, of
> course migration support is not available and tricky to implement.
> 
> With virtualization, it is different. We might want to map something
> into guest address space (e.g. fake DAX devices) and not detect it
> automatically as memory. So we really want to use the maxmem and slots
> parameter just like on all other architectures. Such devices will have
> to expose the applicable memory range themselves. To finally be able to
> provide memory hotplug to guests, we will need a new paravirtualized
> interface to do that (e.g. something into the direction of virtio-mem).
> 
> This implies, that maxmem cannot be used for s390x memory hotplug
> anymore and has to go. This simplifies the code quite a bit.
> 
> As migration support is not working, this change cannot really break
> migration as guests without slots and maxmem don't see the SCLP
> features. Also, the ram size calcualtion does not change.
> 
> Signed-off-by: David Hildenbrand 
[...]

looks mostly sane. Give me some more time to review. 

> @@ -540,9 +302,6 @@ static void sclp_memory_init(SCLPDevice *sclp)
>  {
>  MachineState *machine = MACHINE(qdev_get_machine());
>  ram_addr_t initial_mem = machine->ram_size;
> -ram_addr_t max_mem = machine->maxram_size;
> -ram_addr_t standby_mem = max_mem - initial_mem;
> -ram_addr_t pad_mem = 0;
>  int increment_size = 20;
> 
>  /* The storage increment size is a multiple of 1M and is a power of 2.
> @@ -552,34 +311,14 @@ static void sclp_memory_init(SCLPDevice *sclp)
>  while ((initial_mem >> increment_size) > MAX_STORAGE_INCREMENTS) {
>  increment_size++;
>  }


In theory we could now be more fine grained (as we no longer expose the 
increments)
but this would break migration so better try now to be clever. (e.g. with
compat machines)

[Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support

[David Hildenbrand]

>From an architecture point of view, nothing can be mapped into the address
space on s390x. All there is is memory. Therefore there is also not really
an interface to communicate such information to the guest. All we can do is
specify the maximum ram address and guests can probe in that range if
memory is available and usable (TPROT).

Also memory hotplug is strange. The guest can decide at some point in
time to add / remove memory in some range. And nobody can really hinder it
from doing so. So if we specify right now e.g.
-m 2G,slots=2,maxmem=20G
An ordinary fedora guest will happily online (hotplug) all memory,
resulting in a guest consuming 20G. So it really behaves rather like
-m 22G
There is no way to hotplug memory from the outside like on other
architectures. This is of course bad for upper management layers.

As the guest can create/delete memory regions while it is running, of
course migration support is not available and tricky to implement.

With virtualization, it is different. We might want to map something
into guest address space (e.g. fake DAX devices) and not detect it
automatically as memory. So we really want to use the maxmem and slots
parameter just like on all other architectures. Such devices will have
to expose the applicable memory range themselves. To finally be able to
provide memory hotplug to guests, we will need a new paravirtualized
interface to do that (e.g. something into the direction of virtio-mem).

This implies, that maxmem cannot be used for s390x memory hotplug
anymore and has to go. This simplifies the code quite a bit.

As migration support is not working, this change cannot really break
migration as guests without slots and maxmem don't see the SCLP
features. Also, the ram size calcualtion does not change.

Signed-off-by: David Hildenbrand 
---
 hw/s390x/sclp.c | 310 +---
 include/hw/s390x/sclp.h |  25 
 target/s390x/cpu.h  |   1 -
 3 files changed, 5 insertions(+), 331 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 276972b59f..047d577313 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -15,9 +15,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "cpu.h"
-#include "exec/memory.h"
 #include "sysemu/sysemu.h"
-#include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/event-facility.h"
@@ -57,10 +55,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
 ReadInfo *read_info = (ReadInfo *) sccb;
 MachineState *machine = MACHINE(qdev_get_machine());
-sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
 int cpu_count;
 int rnsize, rnmax;
-int slots = MIN(machine->ram_slots, s390_get_memslot_count());
 IplParameterBlock *ipib = s390_ipl_get_iplb();
 
 /* CPU information */
@@ -80,36 +76,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
 SCLP_HAS_IOA_RECONFIG);
 
-/* Memory Hotplug is only supported for the ccw machine type */
-if (mhd) {
-mhd->standby_subregion_size = MEM_SECTION_SIZE;
-/* Deduct the memory slot already used for core */
-if (slots > 0) {
-while ((mhd->standby_subregion_size * (slots - 1)
-< mhd->standby_mem_size)) {
-mhd->standby_subregion_size = mhd->standby_subregion_size << 1;
-}
-}
-/*
- * Initialize mapping of guest standby memory sections indicating which
- * are and are not online. Assume all standby memory begins offline.
- */
-if (mhd->standby_state_map == 0) {
-if (mhd->standby_mem_size % mhd->standby_subregion_size) {
-mhd->standby_state_map = g_malloc0((mhd->standby_mem_size /
- mhd->standby_subregion_size + 1) *
- (mhd->standby_subregion_size /
- MEM_SECTION_SIZE));
-} else {
-mhd->standby_state_map = g_malloc0(mhd->standby_mem_size /
-   MEM_SECTION_SIZE);
-}
-}
-mhd->padded_ram_size = ram_size + mhd->pad_size;
-mhd->rzm = 1 << mhd->increment_size;
-
-read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
-}
 read_info->mha_pow = s390_get_mha_pow();
 read_info->hmfai = cpu_to_be32(s390_get_hmfai());
 
@@ -121,7 +87,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->rnsize2 = cpu_to_be32(rnsize);
 }
 
-rnmax = machine->maxram_size >> sclp->increment_size;
+/* we don't support standby memory, maxram_size is never exposed */
+rnmax = machine->ram_size >> sclp->increment_size;
 if (rnmax < 0x1) {
 read_info->rnmax =