Re: [PATCH] xen: update machine_to_phys_order on resume

Re: [PATCH] xen: update machine_to_phys_order on resume

[Jan Beulich]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

>>> Ian Campbell  07/13/11 11:12 AM >>>
>It's not so much an objection to this patch but this issue seems to have
>been caused by Xen cset 20892:d311d1efc25e which looks to me like a
>subtle ABI breakage for guests. Perhaps we should introduce a feature
>flag to indicate that a guest can cope with the m2p changing size over
>migration like this?

Indeed - migration was completely beyond my consideration when
submitting this. A feature flag seems the right way to go to me.

Jan


[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 801 bytes --]

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

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

Re: [Xen-devel] [PATCH] xen: update machine_to_phys_order on resume

[Olaf Hering]

On Wed, Jul 13, Jan Beulich wrote:

> >>> Ian Campbell <Ian.Campbell@citrix.com> 07/13/11 11:12 AM >>>
> >It's not so much an objection to this patch but this issue seems to have
> >been caused by Xen cset 20892:d311d1efc25e which looks to me like a
> >subtle ABI breakage for guests. Perhaps we should introduce a feature
> >flag to indicate that a guest can cope with the m2p changing size over
> >migration like this?
> 
> Indeed - migration was completely beyond my consideration when
> submitting this. A feature flag seems the right way to go to me.

I will prepare a patch for a new feature flag.

Olaf

Re: [Xen-devel] [PATCH] xen: update machine_to_phys_order on resume

[Jan Beulich]

>>> On 14.07.11 at 12:26, Olaf Hering <olaf@aepfle.de> wrote:
> On Wed, Jul 13, Jan Beulich wrote:
> 
>> >>> Ian Campbell <Ian.Campbell@citrix.com> 07/13/11 11:12 AM >>>
>> >It's not so much an objection to this patch but this issue seems to have
>> >been caused by Xen cset 20892:d311d1efc25e which looks to me like a
>> >subtle ABI breakage for guests. Perhaps we should introduce a feature
>> >flag to indicate that a guest can cope with the m2p changing size over
>> >migration like this?
>> 
>> Indeed - migration was completely beyond my consideration when
>> submitting this. A feature flag seems the right way to go to me.
> 
> I will prepare a patch for a new feature flag.

Let me fold this into the feature handling change patch I'm close
to submit - without those changes I don't think a guest kernel would
have a way to actually announce its capability.

Jan

Re: [Xen-devel] [PATCH] xen: update machine_to_phys_order on resume

[Jan Beulich]

>>> On 14.07.11 at 12:26, Olaf Hering <olaf@aepfle.de> wrote:
> On Wed, Jul 13, Jan Beulich wrote:
> 
>> >>> Ian Campbell <Ian.Campbell@citrix.com> 07/13/11 11:12 AM >>>
>> >It's not so much an objection to this patch but this issue seems to have
>> >been caused by Xen cset 20892:d311d1efc25e which looks to me like a
>> >subtle ABI breakage for guests. Perhaps we should introduce a feature
>> >flag to indicate that a guest can cope with the m2p changing size over
>> >migration like this?
>> 
>> Indeed - migration was completely beyond my consideration when
>> submitting this. A feature flag seems the right way to go to me.
> 
> I will prepare a patch for a new feature flag.

Let me fold this into the feature handling change patch I'm close
to submit - without those changes I don't think a guest kernel would
have a way to actually announce its capability.

Jan

Re: [PATCH] xen: update machine_to_phys_order on resume

[Jan Beulich]

>>> On 18.07.11 at 10:31, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> Pushing things down at build time is pretty easy. FWIW here's an
> incomplete patch to push the kernel declared features down into the
> hypervisor at build time extracted from some old PV in HVM container
> stuff (so not directly applicable). I can bring it up to date if the
> approach seems useful.

Yes, that looks like what we'd want from a conceptual perspective,
but...

> One thing which is somewhat missing is user control of non-mandatory
> features declared by a kernel, although normally that should be a
> decision made by the tools/hypervisor based upon available features etc.
> 
> diff -r a6c4d03b7d45 tools/libxc/xc_dom_core.c
> --- a/tools/libxc/xc_dom_core.c	Mon Feb 08 13:05:48 2010 +0000
> +++ b/tools/libxc/xc_dom_core.c	Thu Feb 11 12:48:47 2010 +0000
> @@ -609,6 +609,7 @@
>  
>  int xc_dom_parse_image(struct xc_dom_image *dom)
>  {
> +    DECLARE_DOMCTL;
>      int i;
>  
>      xc_dom_printf("%s: called\n", __FUNCTION__);
> @@ -629,8 +630,26 @@
>      /* check features */
>      for ( i = 0; i < XENFEAT_NR_SUBMAPS; i++ )
>      {
> -        dom->f_active[i] |= dom->f_requested[i]; /* cmd line */
> -        dom->f_active[i] |= dom->parms.f_required[i]; /* kernel   */
> +        domctl.cmd = XEN_DOMCTL_setfeatures;
> +        domctl.domain = dom->guest_domid;
> +
> +        domctl.u.setfeatures.submap_idx = i;
> +        domctl.u.setfeatures.submap = 0;
> +
> +        domctl.u.setfeatures.submap |= dom->f_requested[i]; /* cmd line */
> +        domctl.u.setfeatures.submap |= dom->parms.f_required[i]; /* kernel   */
> +
> +        xc_dom_printf("requesting features[%d] = %#x\n", domctl.u.setfeatures.submap_idx, domctl.u.setfeatures.submap);
> +        if (do_domctl(dom->guest_xc, &domctl))
> +        {
> +            xc_dom_panic(XC_INVALID_PARAM,
> +                         "%s: unable to set requested features\n", __FUNCTION__);
> +            goto err;
> +        }
> +
> +        xc_dom_printf("received   features[%d] = %#x\n", domctl.u.setfeatures.submap_idx, domctl.u.setfeatures.submap);
> +        dom->f_active[i] = domctl.u.setfeatures.submap;
> +
>          if ( (dom->f_active[i] & dom->parms.f_supported[i]) !=
>               dom->f_active[i] )
>          {
> @@ -639,6 +658,7 @@
>              goto err;
>          }
>      }
> +
>      return 0;
>  
>   err:
> diff -r a6c4d03b7d45 tools/libxl/xl.c
> --- a/tools/libxl/xl.c	Mon Feb 08 13:05:48 2010 +0000
> +++ b/tools/libxl/xl.c	Thu Feb 11 12:48:47 2010 +0000
> @@ -468,6 +468,8 @@
>          }
>          if (config_lookup_string (&config, "ramdisk", &buf) == CONFIG_TRUE)
>              b_info->u.pv.ramdisk = strdup(buf);
> +        if (config_lookup_string (&config, "features", &buf) == CONFIG_TRUE)
> +            b_info->u.pv.features = strdup(buf);
>      }
>  
>      if ((vbds = config_lookup (&config, "disk")) != NULL) {
> diff -r a6c4d03b7d45 xen/common/domctl.c
> --- a/xen/common/domctl.c	Mon Feb 08 13:05:48 2010 +0000
> +++ b/xen/common/domctl.c	Thu Feb 11 12:48:47 2010 +0000
> @@ -23,6 +23,7 @@
>  #include <xen/paging.h>
>  #include <asm/current.h>
>  #include <public/domctl.h>
> +#include <public/features.h>
>  #include <xsm/xsm.h>
>  
>  static DEFINE_SPINLOCK(domctl_lock);
> @@ -960,6 +962,54 @@
>      }
>      break;
>  
> +    case XEN_DOMCTL_setfeatures:
> +    {
> +        struct domain *d;
> +        ret = -ESRCH;
> +        if ( (d = rcu_lock_domain_by_id(op->domain)) != NULL )
> +        {
> +            printk("dom%d set features[%d] = %#x\n", d->domain_id, op->u.setfeatures.submap_idx, op->u.setfeatures.submap);
> +
> +            switch (op->u.setfeatures.submap_idx) {
> +            case 0:
> +                if ( !paging_mode_translate(d) )

... this condition looks inverted to me.

> +                {
> +                    op->u.setfeatures.submap &= ~(1U<<XENFEAT_writable_page_tables);
> +                    op->u.setfeatures.submap &= ~(1U<<XENFEAT_auto_translated_physmap);
> +                }
> +                if ( !is_pvhvm_domain(d) )
> +                {
> +                    op->u.setfeatures.submap &= ~(1U<<XENFEAT_supervisor_mode_kernel);
> +                }
> +
> +                op->u.setfeatures.submap &= ~(1U<<XENFEAT_writable_descriptor_tables);

Why do you turn this off unconditionally?

> +
> +                /* XXX other features */

That's perhaps also the place holder where the passed in information
would actually get stored?

Jan

> +
> +
> +                if ( op->u.setfeatures.submap &(1U<<XENFEAT_supervisor_mode_kernel) )
> +                    d->arch.pv_kernel_minimum_rpl = 0;
> +
> +                ret = 0;
> +                break;
> +
> +            default:
> +                printk("dom%d unknown feature submap %d\n", d->domain_id, op->u.setfeatures.submap_idx);
> +                op->u.setfeatures.submap = 0;
> +                ret = -EINVAL;
> +                break;
> +            }
> +
> +            rcu_unlock_domain(d);
> +            ret = 0;
> +
> +            if ( copy_to_guest(u_domctl, op, 1) )
> +                ret = -EFAULT;
> +
> +        }
> +    }
> +    break;
> +
>      default:
>          ret = arch_do_domctl(op, u_domctl);
>          break;
> diff -r a6c4d03b7d45 xen/include/public/domctl.h
> --- a/xen/include/public/domctl.h	Mon Feb 08 13:05:48 2010 +0000
> +++ b/xen/include/public/domctl.h	Thu Feb 11 12:48:47 2010 +0000
> @@ -169,6 +169,13 @@
>      XEN_GUEST_HANDLE_64(xen_pfn_t) array;
>  };
>  
> +/* XEN_DOMCTL_setfeatures */
> +struct xen_domctl_setfeatures {
> +    /* IN variables */
> +    unsigned int submap_idx;    /* which 32-bit submap to return */
> +    /* IN/OUT variables */
> +    uint32_t     submap;        /* 32-bit submap, updated with actual 
> result. */
> +};
>  
>  /*
>   * Control shadow pagetables operation
> @@ -842,6 +848,7 @@
>  #define XEN_DOMCTL_gettscinfo                    59
>  #define XEN_DOMCTL_settscinfo                    60
>  #define XEN_DOMCTL_getpageframeinfo3             61
> +#define XEN_DOMCTL_setfeatures	                 62
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -855,6 +862,7 @@
>          struct xen_domctl_getpageframeinfo  getpageframeinfo;
>          struct xen_domctl_getpageframeinfo2 getpageframeinfo2;
>          struct xen_domctl_getpageframeinfo3 getpageframeinfo3;
> +        struct xen_domctl_setfeatures		setfeatures;
>          struct xen_domctl_vcpuaffinity      vcpuaffinity;
>          struct xen_domctl_shadow_op         shadow_op;
>          struct xen_domctl_max_mem           max_mem;

Re: [PATCH] xen: update machine_to_phys_order on resume

[Keir Fraser]

On 18/07/2011 08:05, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 15.07.11 at 20:23, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 15/07/2011 18:30, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>> Actually, one more thought: What's the purpose of this hypercall if
>>> it is set in stone what values it ought to return? Isn't a guest using
>>> it (supposed to be) advertising that it can deal with the values being
>>> variable (and it was just overlooked so far that this doesn't only
>>> include varying values from boot to boot, but also migration)? Or in
>>> other words, if we found a need to relocate the M2P table or grow
>>> its static maximum size, it would be impossible to migrate guests
>>> from an old to a new hypervisor.
>> 
>> Fair point. There has to be a static fallback set of return values for old
>> guests.
> 
> Hmm, in my reading the two sentences sort of contradict each other.
> That is, I'm not certain what route we want to go here: Keep things
> the way they are after 23706:3dd399873c9e, and introduce a
> completely new discovery mechanism if we find it necessary to change
> the M2P table's location and/or size, including a mechanism for a guest
> to announce it's capable of dealing with that? If so, I think we ought
> to add a comment to the hypercall implementation documenting that
> its return values must not be changed (and why).

We can return different values from the existing hypercall if that is
negotiated with the guest, at run time or build time.

 -- Keir

> Jan
> 

Re: [PATCH] xen: update machine_to_phys_order on resume

[Keir Fraser]

On 15/07/2011 18:30, "Jan Beulich" <JBeulich@novell.com> wrote:

> Actually, one more thought: What's the purpose of this hypercall if
> it is set in stone what values it ought to return? Isn't a guest using
> it (supposed to be) advertising that it can deal with the values being
> variable (and it was just overlooked so far that this doesn't only
> include varying values from boot to boot, but also migration)? Or in
> other words, if we found a need to relocate the M2P table or grow
> its static maximum size, it would be impossible to migrate guests
> from an old to a new hypervisor.

Fair point. There has to be a static fallback set of return values for old
guests.

 -- Keir

Re: [PATCH] xen: update machine_to_phys_order on resume

[Jan Beulich]

[-- Attachment #1.1: Type: text/plain, Size: 1427 bytes --]

>>> "Jan Beulich"  07/15/11 6:07 PM >>>
>>>> On 13.07.11 at 11:12, Ian Campbell  wrote:
>> It's not so much an objection to this patch but this issue seems to have
>> been caused by Xen cset 20892:d311d1efc25e which looks to me like a
>> subtle ABI breakage for guests. Perhaps we should introduce a feature
>> flag to indicate that a guest can cope with the m2p changing size over
>> migration like this?
>
>That's actually not strait forward, as the hypervisor can't see the ELF
>note specified features of a DomU kernel. Passing this information
>down from the tools or from the guest kernel itself otoh doesn't
>necessarily seem worth it. Instead a guest that can deal with the
>upper bound of the M2P table changing can easily obtain the
>desired information through XENMEM_maximum_ram_page. So I
>think on the hypervisor side we're good with the patch I sent
>earlier today.

Actually, one more thought: What's the purpose of this hypercall if
it is set in stone what values it ought to return? Isn't a guest using
it (supposed to be) advertising that it can deal with the values being
variable (and it was just overlooked so far that this doesn't only
include varying values from boot to boot, but also migration)? Or in
other words, if we found a need to relocate the M2P table or grow
its static maximum size, it would be impossible to migrate guests
from an old to a new hypervisor.

Jan


[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 1852 bytes --]

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

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

[PATCH] xen: update machine_to_phys_order on resume

[Olaf Hering]

Migration of pv guests fails, the guest crashes on the target host once the
guest is unpaused after transit. It happens when the guest is started on a
small systen, then migrated from that small system to a large system.
If the guest is started on a large system, then migrated to a small system and
back to the large system, the migration will be successful.

The issue is that mfn_to_pfn() makes use of machine_to_phys_order, which
is only configured once early in the boot process. After migration to a
large host the mfns will exceed the order from the small system and a
wrong code path is taken.

Calling xen_setup_machphys_mapping() again in the resume path will avoid
the crash.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 arch/x86/xen/mmu.c     |    2 +-
 arch/x86/xen/suspend.c |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-3.0-rc7/arch/x86/xen/mmu.c
===================================================================
--- linux-3.0-rc7.orig/arch/x86/xen/mmu.c
+++ linux-3.0-rc7/arch/x86/xen/mmu.c
@@ -1623,7 +1623,7 @@ static void __init xen_map_identity_earl
 	set_page_prot(pmd, PAGE_KERNEL_RO);
 }
 
-void __init xen_setup_machphys_mapping(void)
+void xen_setup_machphys_mapping(void)
 {
 	struct xen_machphys_mapping mapping;
 	unsigned long machine_to_phys_nr_ents;
Index: linux-3.0-rc7/arch/x86/xen/suspend.c
===================================================================
--- linux-3.0-rc7.orig/arch/x86/xen/suspend.c
+++ linux-3.0-rc7/arch/x86/xen/suspend.c
@@ -43,6 +43,8 @@ void xen_arch_hvm_post_suspend(int suspe
 
 void xen_arch_post_suspend(int suspend_cancelled)
 {
+	xen_setup_machphys_mapping();
+
 	xen_build_mfn_list_list();
 
 	xen_setup_shared_info();