[PATCH] Disallow setting maxmem to higher value than total physical memory size

[PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

Hi,
this is the patch to disallow changing the maxmem value to higher value 
than total physical memory size since without this patch I was able to 
set dom0 maxmem to higher (invalid) value which is not correct. The 
check for total memory size using the xc.physinfo()['total_mem'] has 
been implemented in this patch and also the check for negative or zero 
value in setMemoryMaximum() has been added. When user enters an invalid 
value (no matter whether negative/zero or higher than total physical 
memory size) an error is returned saying that the memory size is invalid 
since no domain (no matter whether dom0 or domU) can have memory higher 
than maxmem and this prevents maxmem value to be higher than total 
physical memory installed on dom0. Also, on dom0/domU start the domain 
maxmem is being checked against whether it doesn't exceed the total 
physical memory configuration and if it does the value is being reduced 
the the physical memory size to disallow possibility to set to some 
higher value.

You can check the patch by `xm list -l | grep maxmem` command which now 
returns the dom0 physical size on dom0 boot up and when you try to set 
maximum memory of both dom0 and domU you can't set this to higher value 
than dom0 total physical memory size. Since there's a sanity check on 
setting up new memory on domain you'll get "Error: memory_dynamic_max 
must be less than or equal to memory_static_max" when trying to set to 
higher value than domain's maxmem.

Michal

Signed-off-by: Michal Novotny <minovotn@redhat.com>

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: xen-disallow-setting-max-mem-higher-than-total-phys-mem.patch --]
[-- Type: text/x-patch, Size: 1230 bytes --]

diff --git a/tools/python/xen/xend/XendDomainInfo.py b/tools/python/xen/xend/XendDomainInfo.py
index 4360ce2..b38b418 100644
--- a/tools/python/xen/xend/XendDomainInfo.py
+++ b/tools/python/xen/xend/XendDomainInfo.py
@@ -152,6 +152,11 @@ def recreate(info, priv):
 
     assert not info['dying']
 
+    # Validate domain maxmem value not to be higher than dom0 physical memory
+    total_mem = int(xc.physinfo()['total_memory'])
+    if info['maxmem_kb'] > total_mem:
+        info['maxmem_kb'] = total_mem
+
     xeninfo = XendConfig.XendConfig(dominfo = info)
     xeninfo['is_control_domain'] = priv
     xeninfo['is_a_template'] = False
@@ -1490,6 +1495,13 @@ class XendDomainInfo:
         """Set the maximum memory limit of this domain
         @param limit: In MiB.
         """
+        # Get total memory and convert to MiB
+        total_mem = int(xc.physinfo()['total_memory'] / 1024)
+
+        if limit <= 0 or limit > total_mem:
+            raise XendError('Invalid memory size, only positive values '
+                            'up to %s MiB are valid' % total_mem)
+
         log.debug("Setting memory maximum of domain %s (%s) to %d MiB.",
                   self.info['name_label'], str(self.domid), limit)
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
> Hi,
> this is the patch to disallow changing the maxmem value to higher value 
> than total physical memory size since without this patch I was able to 
> set dom0 maxmem to higher (invalid) value which is not correct.

I think it is allowable for a domU though. Consider the scenario where
you have two hosts, one of which has more physical RAM than the other.
You may which to boot a domain on the smaller host, (i.e. booting
ballooned with a current_pages suitable for the small host) and then
migrate it to the large machine where you then want to be able to
balloon to a value larger than was even possible on the previous
machine.

If maxmem is not configured to the largest amount you consider you might
want to give the domain then this scenario fails but it should work.

What is the actual issue with setting a larger maxmem even for domain 0
that you are trying to resolve? Just that it will continue to attempt to
balloon up and never get there?

Ian.

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

On 09/01/2010 02:44 PM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>    
>> Hi,
>> this is the patch to disallow changing the maxmem value to higher value
>> than total physical memory size since without this patch I was able to
>> set dom0 maxmem to higher (invalid) value which is not correct.
>>      
> I think it is allowable for a domU though. Consider the scenario where
> you have two hosts, one of which has more physical RAM than the other.
>    

Yeah, that's right. This scenario has been taken into mind and in fact 
this patch shouldn't do any harm on domU but it was mainly made for dom0 
since dom0 default maxmem value is being set to 16 GiB on x86_64 machine 
which is not correct since it allows setting up up to 16 GiB RAM to dom0 
although we have available only 8 GiB for example. Issuing `xm mem-set 
10240` is therefore possible but it shouldn't be so it's trying to 
reserve 10240. The main issue is that xenstore was having maxmem value 
of 10240 instead of maximum value possible, i.e. value of 8192 in my 
case. Since xenstore itself was having the incorrect information it was 
implemented for xenstore to provide valid information too.
> You may which to boot a domain on the smaller host, (i.e. booting
> ballooned with a current_pages suitable for the small host) and then
> migrate it to the large machine where you then want to be able to
> balloon to a value larger than was even possible on the previous
> machine.
>
> If maxmem is not configured to the largest amount you consider you might
> want to give the domain then this scenario fails but it should work.
>    

Well, if maxmem is not configured for PV domain you mean? This is the 
check only for case that info['maxmem_kb'] > total_mem so if the value 
is not configured what value will be in this variable? If there would be 
a value of 0 (for example) it won't meet the condition at all.

> What is the actual issue with setting a larger maxmem even for domain 0
> that you are trying to resolve? Just that it will continue to attempt to
> balloon up and never get there?
>    

Like stated above, it's the incorrect value in xenstore so this xenstore 
value is not reliable without this patch since without this patch you 
can set the value of dom0/domU to, let's say, 10G, but this won't be 
correct since the value can be having just 8G of RAM. Therefore some 
scripts relaying on the xenstore information would get confused by 
maxmem settings since there will be a value of 10G but host would still 
be physically having just 8G so xend could not be trusted on that one so 
that's why I think this is worth fixing.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 14:01 +0100, Michal Novotny wrote:
> On 09/01/2010 02:44 PM, Ian Campbell wrote:
> > On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
> >    
> >> Hi,
> >> this is the patch to disallow changing the maxmem value to higher value
> >> than total physical memory size since without this patch I was able to
> >> set dom0 maxmem to higher (invalid) value which is not correct.
> >>      
> > I think it is allowable for a domU though. Consider the scenario where
> > you have two hosts, one of which has more physical RAM than the other.
> >    
> 
> Yeah, that's right. This scenario has been taken into mind and in fact 
> this patch shouldn't do any harm on domU but it was mainly made for dom0 
> since dom0 default maxmem value is being set to 16 GiB on x86_64 machine 
> which is not correct since it allows setting up up to 16 GiB RAM to dom0 
> although we have available only 8 GiB for example. Issuing `xm mem-set 
> 10240` is therefore possible but it shouldn't be so it's trying to 
> reserve 10240. The main issue is that xenstore was having maxmem value 
> of 10240 instead of maximum value possible, i.e. value of 8192 in my 
> case. Since xenstore itself was having the incorrect information it was 
> implemented for xenstore to provide valid information too.

I'm saying that I think your patch does cause have harm on a domU, I
don't see anything which limits its actions to just dom0. Can you
explain why a domU is not effected by this change.

As far as I can tell the patch will prevent the creation of a domU which
has a maxmem larger than the current host is capable of providing. The
maxmem setting is the maximum memory is the amount of memory which the
domain _could_ be given. This is different from the amount it currently
actually has which can be different due to ballooning etc.

A domain must be configured with this maxmem value at boot time because
it may need to dynamically size some of data structures (e.g. the frame
table) to allow it to balloon up at a later date.

Which xenstore node are you talking about?

> > You may which to boot a domain on the smaller host, (i.e. booting
> > ballooned with a current_pages suitable for the small host) and then
> > migrate it to the large machine where you then want to be able to
> > balloon to a value larger than was even possible on the previous
> > machine.
> >
> > If maxmem is not configured to the largest amount you consider you might
> > want to give the domain then this scenario fails but it should work.
> >    
> 
> Well, if maxmem is not configured for PV domain you mean?

Sorry, I meant "If maxmem is not able to be configured to a value larger
than the physical memory on the current host...".

Ian.

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

On 09/01/2010 03:37 PM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 14:01 +0100, Michal Novotny wrote:
>    
>> On 09/01/2010 02:44 PM, Ian Campbell wrote:
>>      
>>> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>>>
>>>        
>>>> Hi,
>>>> this is the patch to disallow changing the maxmem value to higher value
>>>> than total physical memory size since without this patch I was able to
>>>> set dom0 maxmem to higher (invalid) value which is not correct.
>>>>
>>>>          
>>> I think it is allowable for a domU though. Consider the scenario where
>>> you have two hosts, one of which has more physical RAM than the other.
>>>
>>>        
>> Yeah, that's right. This scenario has been taken into mind and in fact
>> this patch shouldn't do any harm on domU but it was mainly made for dom0
>> since dom0 default maxmem value is being set to 16 GiB on x86_64 machine
>> which is not correct since it allows setting up up to 16 GiB RAM to dom0
>> although we have available only 8 GiB for example. Issuing `xm mem-set
>> 10240` is therefore possible but it shouldn't be so it's trying to
>> reserve 10240. The main issue is that xenstore was having maxmem value
>> of 10240 instead of maximum value possible, i.e. value of 8192 in my
>> case. Since xenstore itself was having the incorrect information it was
>> implemented for xenstore to provide valid information too.
>>      
> I'm saying that I think your patch does cause have harm on a domU, I
> don't see anything which limits its actions to just dom0. Can you
> explain why a domU is not effected by this change.
>
> As far as I can tell the patch will prevent the creation of a domU which
> has a maxmem larger than the current host is capable of providing. The
> maxmem setting is the maximum memory is the amount of memory which the
> domain _could_ be given. This is different from the amount it currently
> actually has which can be different due to ballooning etc.
>
> A domain must be configured with this maxmem value at boot time because
> it may need to dynamically size some of data structures (e.g. the frame
> table) to allow it to balloon up at a later date.
>    
Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be 
causing anything bad in domU. Of course, I can limit this to dom0 but 
for domU you can be having e.g. this:
1)
dom0: total memory = 8192
domU: memory = 4096, maxmem = 8192 (xm mem-max domU 16384 fails)
2)
and when you migrate to host B:
dom0: total memory = 16384
domU: memory = 4096, maxmem = 8192
3)
so when migrating back to host A you'll have:
dom0: total memory = 8192
domU: memory = 4096, maxmem = 8192

But I don't think this behaviour is that bad since if you won't be 
having the patch applied you could be able to set max_mem to value of 
16G in step 1 and then in step 2 (8G host machine) you could be able to 
issue `xm mem-max domU 10240` which is not valid on host B (as in step 
2) so we could prevent this by setting up domain maxmem to be 8192 which 
is the maximum on host B.

>
> Which xenstore node are you talking about?
>
>
>    

Oh, I confused xenstore and `xm list -l` output there. Sorry for that. I 
also tried to trigger the long listing using xl (using xl list -l) but I 
was having no luck to do so. The maxmem value is not available in 
xenstore but for `xm list -l` the value is there, you can try running:

`xm list -l | grep maxmem`

and before my patch applied you could see (maxmem 16777215) there. With 
my patch applied the correct value will be in this SXPR node.

I tried following for xl utility:

# xl list 0
Name                                        ID   Mem VCPUs      State   
Time(s)
Domain-0                                     0  6958     4     
r-----      74.2
# xl list 0 -l
Usage: xl [-v] list [options] [Domain]


List information about all/some domains.

Options:

-l, --long              Output all VM details
-v, --verbose           Prints out UUIDs
# xl list -l 0
#

so obviously there's no option to get the long domain listing using the 
libxl but it's useful to parse domain information (e.g. using shell 
scripts) that you can get using e.g .`xm list 0 -l` command.

>>> You may which to boot a domain on the smaller host, (i.e. booting
>>> ballooned with a current_pages suitable for the small host) and then
>>> migrate it to the large machine where you then want to be able to
>>> balloon to a value larger than was even possible on the previous
>>> machine.
>>>
>>> If maxmem is not configured to the largest amount you consider you might
>>> want to give the domain then this scenario fails but it should work.
>>>
>>>        
>> Well, if maxmem is not configured for PV domain you mean?
>>      
> Sorry, I meant "If maxmem is not able to be configured to a value larger
> than the physical memory on the current host...".
>    

I see what you mean but I guess I already replied above. If we preserve 
the possibility to setup maxmem to higher value than dom0 physical 
memory is then `xm mem-max domU HigherValueThanPhysMem` will be possible 
to be issued which would lead to setup the invalid value in `xm list -l` 
output.

Or should I just ignore the possibility domU maxmem could be set to 
higher value than host machine could provide and should I limit my check 
to dom0 only?

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

On 09/01/2010 04:18 PM, Michal Novotny wrote:
> On 09/01/2010 03:37 PM, Ian Campbell wrote:
>> On Wed, 2010-09-01 at 14:01 +0100, Michal Novotny wrote:
>>> On 09/01/2010 02:44 PM, Ian Campbell wrote:
>>>> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>>>>
>>>>> Hi,
>>>>> this is the patch to disallow changing the maxmem value to higher 
>>>>> value
>>>>> than total physical memory size since without this patch I was 
>>>>> able to
>>>>> set dom0 maxmem to higher (invalid) value which is not correct.
>>>>>
>>>> I think it is allowable for a domU though. Consider the scenario where
>>>> you have two hosts, one of which has more physical RAM than the other.
>>>>
>>> Yeah, that's right. This scenario has been taken into mind and in fact
>>> this patch shouldn't do any harm on domU but it was mainly made for 
>>> dom0
>>> since dom0 default maxmem value is being set to 16 GiB on x86_64 
>>> machine
>>> which is not correct since it allows setting up up to 16 GiB RAM to 
>>> dom0
>>> although we have available only 8 GiB for example. Issuing `xm mem-set
>>> 10240` is therefore possible but it shouldn't be so it's trying to
>>> reserve 10240. The main issue is that xenstore was having maxmem value
>>> of 10240 instead of maximum value possible, i.e. value of 8192 in my
>>> case. Since xenstore itself was having the incorrect information it was
>>> implemented for xenstore to provide valid information too.
>> I'm saying that I think your patch does cause have harm on a domU, I
>> don't see anything which limits its actions to just dom0. Can you
>> explain why a domU is not effected by this change.
>>
>> As far as I can tell the patch will prevent the creation of a domU which
>> has a maxmem larger than the current host is capable of providing. The
>> maxmem setting is the maximum memory is the amount of memory which the
>> domain _could_ be given. This is different from the amount it currently
>> actually has which can be different due to ballooning etc.
>>
>> A domain must be configured with this maxmem value at boot time because
>> it may need to dynamically size some of data structures (e.g. the frame
>> table) to allow it to balloon up at a later date.
> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to 
> be causing anything bad in domU. Of course, I can limit this to dom0 
> but for domU you can be having e.g. this:
> 1)
> dom0: total memory = 8192
> domU: memory = 4096, maxmem = 8192 (xm mem-max domU 16384 fails)
> 2)
> and when you migrate to host B:
> dom0: total memory = 16384
> domU: memory = 4096, maxmem = 8192
> 3)
> so when migrating back to host A you'll have:
> dom0: total memory = 8192
> domU: memory = 4096, maxmem = 8192
>
> But I don't think this behaviour is that bad since if you won't be 
> having the patch applied you could be able to set max_mem to value of 
> 16G in step 1 and then in step 2 (8G host machine) you could be able 
> to issue `xm mem-max domU 10240` which is not valid on host B (as in 
> step 2) so we could prevent this by setting up domain maxmem to be 
> 8192 which is the maximum on host B.

Oh, sorry. I meant `xm mem-set domU 10240` there.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jan Beulich]

>>> On 01.09.10 at 16:18, Michal Novotny <minovotn@redhat.com> wrote:
> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be 

And how does this play together with physical memory hotplug?

Jan

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

On 09/01/2010 04:26 PM, Jan Beulich wrote:
>>>> On 01.09.10 at 16:18, Michal Novotny<minovotn@redhat.com>  wrote:
>>>>          
>> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be
>>      
> And how does this play together with physical memory hotplug?
>
> Jan
>
>    
Well, it's reading the physical memory size using the xc.physinfo() call 
so if this is handled correctly by hypervisor (since this  basically 
issues a hypercall) then it should be working fine.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jan Beulich]

>>> On 01.09.10 at 16:50, Michal Novotny <minovotn@redhat.com> wrote:
> On 09/01/2010 04:26 PM, Jan Beulich wrote:
>>>>> On 01.09.10 at 16:18, Michal Novotny<minovotn@redhat.com>  wrote:
>>>>>          
>>> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be
>>>      
>> And how does this play together with physical memory hotplug?
>>
>> Jan
>>
>>    
> Well, it's reading the physical memory size using the xc.physinfo() call 
> so if this is handled correctly by hypervisor (since this  basically 
> issues a hypercall) then it should be working fine.

Meaning the tools would auto-adjust maxmem_kb' for Dom0 when
new memory got added? I can't see where that happens.

Jan

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

On 09/01/2010 05:00 PM, Jan Beulich wrote:
>>>> On 01.09.10 at 16:50, Michal Novotny<minovotn@redhat.com>  wrote:
>>>>          
>> On 09/01/2010 04:26 PM, Jan Beulich wrote:
>>      
>>>>>> On 01.09.10 at 16:18, Michal Novotny<minovotn@redhat.com>   wrote:
>>>>>>
>>>>>>              
>>>> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be
>>>>
>>>>          
>>> And how does this play together with physical memory hotplug?
>>>
>>> Jan
>>>
>>>
>>>        
>> Well, it's reading the physical memory size using the xc.physinfo() call
>> so if this is handled correctly by hypervisor (since this  basically
>> issues a hypercall) then it should be working fine.
>>      
> Meaning the tools would auto-adjust maxmem_kb' for Dom0 when
> new memory got added? I can't see where that happens.
>
> Jan
>
>    
It's all based on xc.physinfo() call which is basically a hypercall. If 
there's a valid value coming from the hypervisor then it would be 
working fine.

You can see the call is there always on this line of my patch:

+ total_mem = int(xc.physinfo()['total_memory'])

If this is handled correctly by hypervisor then we're good in user-space 
as can be seen in tools/python/xen/lowlevel/xc/xc.c and 
tools/libxl/libxl.c where call to tools/libxc/xc_misc.c:xc_physinfo() 
resides.

Michal


-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jan Beulich]

>>> On 01.09.10 at 17:10, Michal Novotny <minovotn@redhat.com> wrote:
> On 09/01/2010 05:00 PM, Jan Beulich wrote:
>>>>> On 01.09.10 at 16:50, Michal Novotny<minovotn@redhat.com>  wrote:
>>>>>          
>>> On 09/01/2010 04:26 PM, Jan Beulich wrote:
>>>      
>>>>>>> On 01.09.10 at 16:18, Michal Novotny<minovotn@redhat.com>   wrote:
>>>>>>>
>>>>>>>              
>>>>> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be
>>>>>
>>>>>          
>>>> And how does this play together with physical memory hotplug?
>>>>
>>>> Jan
>>>>
>>>>
>>>>        
>>> Well, it's reading the physical memory size using the xc.physinfo() call
>>> so if this is handled correctly by hypervisor (since this  basically
>>> issues a hypercall) then it should be working fine.
>>>      
>> Meaning the tools would auto-adjust maxmem_kb' for Dom0 when
>> new memory got added? I can't see where that happens.
>>
>> Jan
>>
>>    
> It's all based on xc.physinfo() call which is basically a hypercall. If 
> there's a valid value coming from the hypervisor then it would be 
> working fine.
> 
> You can see the call is there always on this line of my patch:
> 
> + total_mem = int(xc.physinfo()['total_memory'])

But note that this line you add to recreate(), and I don't think this
will get executed a second time long after xend started.

> If this is handled correctly by hypervisor then we're good in user-space 
> as can be seen in tools/python/xen/lowlevel/xc/xc.c and 
> tools/libxl/libxl.c where call to tools/libxc/xc_misc.c:xc_physinfo() 
> resides.

Jan

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 15:18 +0100, Michal Novotny wrote:

> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be 
> causing anything bad in domU. Of course, I can limit this to dom0 but 
> for domU you can be having e.g. this:
> 1)
> dom0: total memory = 8192
> domU: memory = 4096, maxmem = 8192 (xm mem-max domU 16384 fails)

It is useful, legal and valid to set e.g. maxmem = 12288 here, leaving
memory = 4096. Your patch prevents that.

> 2)
> and when you migrate to host B:
> dom0: total memory = 16384
> domU: memory = 4096, maxmem = 8192

If maxmem = 12288 then it would be possible to balloon this guest up to
12288 on this system. With your patch it is no longer possible. Note
that maxmem cannot change once the domU is booted so it needs to have
been = 12288 at the time the guest was created on Host A.

> 
> Or should I just ignore the possibility domU maxmem could be set to 
> higher value than host machine could provide and should I limit my check 
> to dom0 only?

Yes!

I haven't considered the applicability of this patch to dom0
particularly deeply but it is wrong to enforce this constraint on domUs.

Ian.

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On 09/01/2010 05:20 PM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 15:18 +0100, Michal Novotny wrote:
>
>    
>> Oh, ok. It's not limited to dom0 nevertheless I don't see anything to be
>> causing anything bad in domU. Of course, I can limit this to dom0 but
>> for domU you can be having e.g. this:
>> 1)
>> dom0: total memory = 8192
>> domU: memory = 4096, maxmem = 8192 (xm mem-max domU 16384 fails)
>>      
> It is useful, legal and valid to set e.g. maxmem = 12288 here, leaving
> memory = 4096. Your patch prevents that.
>
>    

That's right. I'm just saying that there's a possibility that there will 
be wrong data output from `xm list -l` command.

>> 2)
>> and when you migrate to host B:
>> dom0: total memory = 16384
>> domU: memory = 4096, maxmem = 8192
>>      
> If maxmem = 12288 then it would be possible to balloon this guest up to
> 12288 on this system. With your patch it is no longer possible. Note
> that maxmem cannot change once the domU is booted so it needs to have
> been = 12288 at the time the guest was created on Host A.
>
>    

That's correct.

>> Or should I just ignore the possibility domU maxmem could be set to
>> higher value than host machine could provide and should I limit my check
>> to dom0 only?
>>      
> Yes!
>
> I haven't considered the applicability of this patch to dom0
> particularly deeply but it is wrong to enforce this constraint on domUs.
>
> Ian.
>
>    
Ok, I'm attaching the patch now for review. Now it's working fine for 
dom0 to limit just dom0.

What do you think about this?

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: xen-disallow-setting-max-mem-higher-than-total-phys-mem.patch --]
[-- Type: text/x-patch, Size: 1316 bytes --]

diff --git a/tools/python/xen/xend/XendDomainInfo.py b/tools/python/xen/xend/XendDomainInfo.py
index 4360ce2..aabc30f 100644
--- a/tools/python/xen/xend/XendDomainInfo.py
+++ b/tools/python/xen/xend/XendDomainInfo.py
@@ -152,6 +152,12 @@ def recreate(info, priv):
 
     assert not info['dying']
 
+    # Validate domain maxmem value not to be higher than dom0 physical memory
+    if priv:
+        total_mem = int(xc.physinfo()['total_memory'])
+        if info['maxmem_kb'] > total_mem:
+            info['maxmem_kb'] = total_mem
+
     xeninfo = XendConfig.XendConfig(dominfo = info)
     xeninfo['is_control_domain'] = priv
     xeninfo['is_a_template'] = False
@@ -1490,6 +1496,14 @@ class XendDomainInfo:
         """Set the maximum memory limit of this domain
         @param limit: In MiB.
         """
+        # Get total memory and convert to MiB
+        if self.info['is_control_domain']:
+            total_mem = int(xc.physinfo()['total_memory'] / 1024)
+
+            if limit <= 0 or limit > total_mem:
+                raise XendError('Invalid memory size, only positive values '
+                                'up to %s MiB are valid' % total_mem)
+
         log.debug("Setting memory maximum of domain %s (%s) to %d MiB.",
                   self.info['name_label'], str(self.domid), limit)
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jeremy Fitzhardinge]

 On 09/01/2010 05:44 AM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>> Hi,
>> this is the patch to disallow changing the maxmem value to higher value 
>> than total physical memory size since without this patch I was able to 
>> set dom0 maxmem to higher (invalid) value which is not correct.
> I think it is allowable for a domU though. Consider the scenario where
> you have two hosts, one of which has more physical RAM than the other.
> You may which to boot a domain on the smaller host, (i.e. booting
> ballooned with a current_pages suitable for the small host) and then
> migrate it to the large machine where you then want to be able to
> balloon to a value larger than was even possible on the previous
> machine.

But max-mem can change between hosts; on the small host it needn't have
a maxmem larger than the host's memory.  (The domain itself may have a
larger notion of maxmem internally, but that's separate.)

    J

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 17:53 +0100, Jeremy Fitzhardinge wrote:
> On 09/01/2010 05:44 AM, Ian Campbell wrote:
> > On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
> >> Hi,
> >> this is the patch to disallow changing the maxmem value to higher value 
> >> than total physical memory size since without this patch I was able to 
> >> set dom0 maxmem to higher (invalid) value which is not correct.
> > I think it is allowable for a domU though. Consider the scenario where
> > you have two hosts, one of which has more physical RAM than the other.
> > You may which to boot a domain on the smaller host, (i.e. booting
> > ballooned with a current_pages suitable for the small host) and then
> > migrate it to the large machine where you then want to be able to
> > balloon to a value larger than was even possible on the previous
> > machine.
> 
> But max-mem can change between hosts; on the small host it needn't have
> a maxmem larger than the host's memory.  (The domain itself may have a
> larger notion of maxmem internally, but that's separate.)

It's not separate, this guest configuration item precisely informs the
guest how large it can expect it's memory map to ever need to be (the
setting is also called the static-max in both xend and xapi).

For a PV guest the value is pushed down into the hypervisor by the
toolstack via XENMEM_set_memory_map and this controls the memory map
returned to the guest from XENMEM_get_memory_map, which in turn informs
the guest's choice of maxmem value (for PV guests which can boot
ballooned that is).

For an HVM guest maxmem sets (via HVM loader) the limit of the e820
presented to the guest.

Ian.

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jeremy Fitzhardinge]

 On 09/01/2010 10:56 AM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 17:53 +0100, Jeremy Fitzhardinge wrote:
>> On 09/01/2010 05:44 AM, Ian Campbell wrote:
>>> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>>>> Hi,
>>>> this is the patch to disallow changing the maxmem value to higher value 
>>>> than total physical memory size since without this patch I was able to 
>>>> set dom0 maxmem to higher (invalid) value which is not correct.
>>> I think it is allowable for a domU though. Consider the scenario where
>>> you have two hosts, one of which has more physical RAM than the other.
>>> You may which to boot a domain on the smaller host, (i.e. booting
>>> ballooned with a current_pages suitable for the small host) and then
>>> migrate it to the large machine where you then want to be able to
>>> balloon to a value larger than was even possible on the previous
>>> machine.
>> But max-mem can change between hosts; on the small host it needn't have
>> a maxmem larger than the host's memory.  (The domain itself may have a
>> larger notion of maxmem internally, but that's separate.)
> It's not separate, this guest configuration item precisely informs the
> guest how large it can expect it's memory map to ever need to be (the
> setting is also called the static-max in both xend and xapi).

No, that is separate.  There are three values:

   1. the domain's initial memory allocation
   2. the max size the domain will ever grow to
   3. the max size Xen will allow the domain to grow to right now

At domain build time, the domain needs to know 1 and 2, but 3 is
irrelevant (so long it is larger than 1). 

2 can be arbitrarily large.  The domain may not need to know about 2 at
all if it can dynamically add memory at runtime via, say, memory
hotplug.  It only matters if it needs to statically allocate space in
its memory map for future balloonings.

There's no need for 3 to ever be larger than the host's physical memory,
and it can be changed at will (if the host memory size changes due to
hotplug memory, save/restore or migrate).

AFAICT, "static-max" is completely useless, at least for PV Linux
domains, because the kernel needs to get that value when its
constructing its basic memory mappings, way before it has any chance to
talk to xenstore.  When else is it useful?

> For a PV guest the value is pushed down into the hypervisor by the
> toolstack via XENMEM_set_memory_map and this controls the memory map
> returned to the guest from XENMEM_get_memory_map, which in turn informs
> the guest's choice of maxmem value (for PV guests which can boot
> ballooned that is).

OK, I need to investigate that.  But the maxmem size for the domain
should be something derived from the domain's config file, and have no
direct relationship to Xen's current maxmem for that domain (though the
toolstack may choose to make them the same at build time).

> For an HVM guest maxmem sets (via HVM loader) the limit of the e820
> presented to the guest.

Presumably that's a value constructed by the domain builder which -
again - needn't have any relationship to the Xen maxmem.

    J

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 19:15 +0100, Jeremy Fitzhardinge wrote:
> On 09/01/2010 10:56 AM, Ian Campbell wrote:
> > On Wed, 2010-09-01 at 17:53 +0100, Jeremy Fitzhardinge wrote:
> >> On 09/01/2010 05:44 AM, Ian Campbell wrote:
> >>> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
> >>>> Hi,
> >>>> this is the patch to disallow changing the maxmem value to higher value 
> >>>> than total physical memory size since without this patch I was able to 
> >>>> set dom0 maxmem to higher (invalid) value which is not correct.
> >>> I think it is allowable for a domU though. Consider the scenario where
> >>> you have two hosts, one of which has more physical RAM than the other.
> >>> You may which to boot a domain on the smaller host, (i.e. booting
> >>> ballooned with a current_pages suitable for the small host) and then
> >>> migrate it to the large machine where you then want to be able to
> >>> balloon to a value larger than was even possible on the previous
> >>> machine.
> >> But max-mem can change between hosts; on the small host it needn't have
> >> a maxmem larger than the host's memory.  (The domain itself may have a
> >> larger notion of maxmem internally, but that's separate.)
> > It's not separate, this guest configuration item precisely informs the
> > guest how large it can expect it's memory map to ever need to be (the
> > setting is also called the static-max in both xend and xapi).
> 
> No, that is separate.  There are three values:
> 
>    1. the domain's initial memory allocation
>    2. the max size the domain will ever grow to
>    3. the max size Xen will allow the domain to grow to right now
> 
> At domain build time, the domain needs to know 1 and 2, but 3 is
> irrelevant (so long it is larger than 1). 
> 
> 2 can be arbitrarily large.  The domain may not need to know about 2 at
> all if it can dynamically add memory at runtime via, say, memory
> hotplug.  It only matters if it needs to statically allocate space in
> its memory map for future balloonings.

Yes, so it only matters for all PV Linux guests which currently support
ballooning up from their initial size...

> There's no need for 3 to ever be larger than the host's physical memory,
> and it can be changed at will (if the host memory size changes due to
> hotplug memory, save/restore or migrate).
> 
> AFAICT, "static-max" is completely useless, at least for PV Linux
> domains, because the kernel needs to get that value when its
> constructing its basic memory mappings, way before it has any chance to
> talk to xenstore.

It's not only in xenstore, it is also in the result of the
XENMEM_get_memory_map hypercall.

Old-style PV Linux domains really do use static-max in this way. It may
well be that there are potentially better ways for guests to implement
this in the future but we have a deployed base of guests which work this
way.

> > For a PV guest the value is pushed down into the hypervisor by the
> > toolstack via XENMEM_set_memory_map and this controls the memory map
> > returned to the guest from XENMEM_get_memory_map, which in turn informs
> > the guest's choice of maxmem value (for PV guests which can boot
> > ballooned that is).
> 
> OK, I need to investigate that.  But the maxmem size for the domain
> should be something derived from the domain's config file,

I thought we were talking about clamping the value from the domain's
config file, weren't we? At least that's what I thought the proposed
patch would end up doing, which is why I was objecting.

It may be that xend uses the same variable names in various structures
(and/or otherwise conflates the two concepts of maxmem) and we are
talking at cross purposes here but from what I can tell
xc_domain_set_memmap_limit() is called from
X86_Linux_ImageHandler::buildDomain with the result of
XendDomainInfo::getMemoryMaximum which I think is the same value as is
stored by XendDomainInfo::setMemoryMaximum (one of the functions being
patched here). This field ultimately corresponds to the maxmem field in
the domain configuration file, although when it is translated from cfg
file to XendDomainInfo it appears to be set without using the setter
method so maybe the patch only has an impact in some obscure set of
circumstances. With xend being the twisty maze that it is it is hard to
be sure what's going on.

I don't think it has been shown that the two maxmems are completely
disconnected in xend's mind or what the impact this change is on the
maxmem which is exposed to guests.

It's also not entirely clear what actual problem this patch is solving
(apart from allusions to something being wrong in "xm list -v") but it
seems to me that if it is clamping an invalid value for domain 0 into
something value then surely that value came from _somewhere_ and that is
the place which really needs fixing.

Ian.

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Jeremy Fitzhardinge]

 On 09/01/2010 11:53 AM, Ian Campbell wrote:
> On Wed, 2010-09-01 at 19:15 +0100, Jeremy Fitzhardinge wrote:
>> On 09/01/2010 10:56 AM, Ian Campbell wrote:
>>> On Wed, 2010-09-01 at 17:53 +0100, Jeremy Fitzhardinge wrote:
>>>> On 09/01/2010 05:44 AM, Ian Campbell wrote:
>>>>> On Wed, 2010-09-01 at 13:31 +0100, Michal Novotny wrote:
>>>>>> Hi,
>>>>>> this is the patch to disallow changing the maxmem value to higher value 
>>>>>> than total physical memory size since without this patch I was able to 
>>>>>> set dom0 maxmem to higher (invalid) value which is not correct.
>>>>> I think it is allowable for a domU though. Consider the scenario where
>>>>> you have two hosts, one of which has more physical RAM than the other.
>>>>> You may which to boot a domain on the smaller host, (i.e. booting
>>>>> ballooned with a current_pages suitable for the small host) and then
>>>>> migrate it to the large machine where you then want to be able to
>>>>> balloon to a value larger than was even possible on the previous
>>>>> machine.
>>>> But max-mem can change between hosts; on the small host it needn't have
>>>> a maxmem larger than the host's memory.  (The domain itself may have a
>>>> larger notion of maxmem internally, but that's separate.)
>>> It's not separate, this guest configuration item precisely informs the
>>> guest how large it can expect it's memory map to ever need to be (the
>>> setting is also called the static-max in both xend and xapi).
>> No, that is separate.  There are three values:
>>
>>    1. the domain's initial memory allocation
>>    2. the max size the domain will ever grow to
>>    3. the max size Xen will allow the domain to grow to right now
>>
>> At domain build time, the domain needs to know 1 and 2, but 3 is
>> irrelevant (so long it is larger than 1). 
>>
>> 2 can be arbitrarily large.  The domain may not need to know about 2 at
>> all if it can dynamically add memory at runtime via, say, memory
>> hotplug.  It only matters if it needs to statically allocate space in
>> its memory map for future balloonings.
> Yes, so it only matters for all PV Linux guests which currently support
> ballooning up from their initial size...

Yes,  but it doesn't relate to 3, is my point.  It only matters at the
moment of domain creation.

>> There's no need for 3 to ever be larger than the host's physical memory,
>> and it can be changed at will (if the host memory size changes due to
>> hotplug memory, save/restore or migrate).
>>
>> AFAICT, "static-max" is completely useless, at least for PV Linux
>> domains, because the kernel needs to get that value when its
>> constructing its basic memory mappings, way before it has any chance to
>> talk to xenstore.
> It's not only in xenstore, it is also in the result of the
> XENMEM_get_memory_map hypercall.

Is that separate from 3?

> Old-style PV Linux domains really do use static-max in this way. It may
> well be that there are potentially better ways for guests to implement
> this in the future but we have a deployed base of guests which work this
> way.

I specifically meant the Xenstore "static-max" value is useless. 
Getting it via hypercall is fine.

>>> For a PV guest the value is pushed down into the hypervisor by the
>>> toolstack via XENMEM_set_memory_map and this controls the memory map
>>> returned to the guest from XENMEM_get_memory_map, which in turn informs
>>> the guest's choice of maxmem value (for PV guests which can boot
>>> ballooned that is).
>> OK, I need to investigate that.  But the maxmem size for the domain
>> should be something derived from the domain's config file,
> I thought we were talking about clamping the value from the domain's
> config file, weren't we? At least that's what I thought the proposed
> patch would end up doing, which is why I was objecting.

Oh, if that's the case, then sure, don't clamp that.

    J

Re: [PATCH] Disallow setting maxmem to higher value than total physical memory size

[Ian Campbell]

On Wed, 2010-09-01 at 22:10 +0100, Jeremy Fitzhardinge wrote:
> 
> > It's not only in xenstore, it is also in the result of the
> > XENMEM_get_memory_map hypercall.
> 
> Is that separate from 3?

As far as the h/v is concerned yes. Although they know it's an e820 the
set/get_memory_map hypercalls basically store an opaque blob so far as
the h/v is concerned. xend doesn't seem so sure they are unrelated...

> > Old-style PV Linux domains really do use static-max in this way. It may
> > well be that there are potentially better ways for guests to implement
> > this in the future but we have a deployed base of guests which work this
> > way.
> 
> I specifically meant the Xenstore "static-max" value is useless. 
> Getting it via hypercall is fine.

I'm not sure but I think this is used by HVM balloon drivers when PoD is
active. I don't know why this is the case though, other than
XENMEM_get_memory_map being a PV only hypercall.

Ian.