Re: Re: [PATCH]: Allow tools to map arbitrarily large machphys_mfn_list on 32bit dom0

["Jan Beulich" <JBeulich@novell.com>]

>>> On 14.03.11 at 17:33, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> On Mon, 2011-03-14 at 16:22 +0000, Jan Beulich wrote:
>> >>> On 14.03.11 at 17:03, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
>> > On Mon, 2011-03-14 at 15:55 +0000, Jan Beulich wrote:
>> >> >>> On 14.03.11 at 16:19, Gianni Tedesco <gianni.tedesco@citrix.com> wrote:
>> >> > 
>> >> > This permits suspend/resume to work with 32bit dom0/tools. AFAICT the
>> >> > limit to MACH2PHYS_COMPAT_NR_ENTRIES is redundant since that refers to a
>> >> > limit in 32bit guest compat mappings under 64bit hypervisors, not
>> >> > userspace where there may be gigabytes of useful virtual space available
>> >> > for this.
>> >> > 
>> >> > Suggested-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
>> >> > Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
>> >> > 
>> >> > diff -r 8b5cbccbc654 xen/arch/x86/x86_64/compat/mm.c
>> >> > --- a/xen/arch/x86/x86_64/compat/mm.c	Mon Mar 14 14:59:27 2011 +0000
>> >> > +++ b/xen/arch/x86/x86_64/compat/mm.c	Mon Mar 14 15:17:59 2011 +0000
>> >> > @@ -161,9 +161,7 @@ int compat_arch_memory_op(int op, XEN_GU
>> >> >          if ( copy_from_guest(&xmml, arg, 1) )
>> >> >              return -EFAULT;
>> >> >  
>> >> > -        limit = (unsigned long)(compat_machine_to_phys_mapping +
>> >> > -            min_t(unsigned long, max_page,
>> >> > -                  MACH2PHYS_COMPAT_NR_ENTRIES(current->domain)));
>> >> > +        limit = (unsigned long)(compat_machine_to_phys_mapping + 
>> > max_page);
>> >> 
>> >> While doing this shouldn't hurt (except slightly for performance of
>> >> the hypercall), I don't see why it's useful: For slots past
>> >> MACH2PHYS_COMPAT_NR_ENTRIES(current->domain) you
>> >> wouldn't read non-null page table entries anyway (up to
>> >> RDWR_COMPAT_MPT_VIRT_END), so I don't see why the tools
>> >> couldn't equally well do with what we have currently (after all
>> >> they get told how many slots were filled).
>> > 
>> > In order to be able to migrate any guest the tools in domain 0 need to
>> > see the entire of host M2P, not just the subset which the kernel sees
>> > mapped into its hypervisor hole (which is what
>> > MACH2PHYS_COMPAT_NR_ENTRIES represents).
>> > 
>> > The hypercall reads from the global compat M2P mapping, not the guest
>> > kernel mapping of it, so it should read valid entries all the way up to
>> > RDWR_COMPAT_MPT_VIRT_END, AFAICT.
>> 
>> But RDWR_COMPAT_MPT_VIRT_END still doesn't necessarily
>> cover all of the memory the machine may have (after all the
>> range is way smaller than RDWR_MPT_VIRT_{START,END}.
> 
> It's 1GB which is enough to cover 1TB of host memory, which AFAIK is all
> we support these days. It certainly buys us time compared with currently
> failing at 160GB.

1Tb of *contiguous* host memory. And that's certainly not the limit
Xen has been run on, and Xen itself is set up to handle 5Tb. Which
I'm seeing to get exceeded on experimental(?) systems...

And while I agree that failing at 1Tb is better than failing at 160Gb,
I favor fixing this once and completely over doing a little bit of
papering over the problem now just to require debugging the same
issue again later.

>> If that's the goal, then the patch as presented isn't suitable,
>> as there's not event a compat table set up for all of the
>> memory.
> 
> paging_init seems to do the right thing and setup the compat M2P up to a
> maximum of RDWR_COMPAT_MPT_VIRT_END.

With 1Gb being the theoretical limit of what a 32-bit guest can
see and access, that's all a guest could ever sensibly ask for (a
[hypothetical] domain could ask for having a larger than the
default hole with more of the table mapped in).

Jan