[PATCH] xencomm, xenmem and hypercall continuation

[PATCH] xencomm, xenmem and hypercall continuation

[Isaku Yamahata]

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

fix xenmem hypercall for non-trivial xencomm arch(i.e. ia64, and powerpc)
On ia64 and powerpc, guest_handle_add_offset() effect persists over
hypercall continuation because its consumed offset is recorced in
guest domains memory space.
On the other hand, x86 guest_handle_add_offset() effect is volatile
over hypercall continuation.
So xenmem hypercall(more specifically increase_reservation,
decrease_reservaton, populate_memory and exchange) is broken on
ia64 and powerpc.
#ifdef/ifndef CONFIG_X86 is used to solve this issue without breaking
the existing ABI. #ifdef is ugly, but it would be difficult to solve
this issue without #ifdef and to preserve the existing ABI simaltaneously.

I checked other users of both guest_handle_add_offset() and hypercall
continuation. Fortunately other users records restart points
by hypercall argument so that this isn't an issue.


-- 
yamahata

[-- Attachment #2: 12315_f3e97d467b6f_xencomm_and_xenmem_hypercall.patch --]
[-- Type: text/plain, Size: 3754 bytes --]

# HG changeset patch
# User yamahata@valinux.co.jp
# Date 1163136126 -32400
# Node ID f3e97d467b6f7e36c95d4deb3ed3bc955710f8e7
# Parent  5470cadfeb22e33e7abb86193224984140732361
fix xenmem hypercall for non-trivial xencomm arch(i.e. ia64, and powerpc)
On ia64 and powerpc, guest_handle_add_offset() effect persists over
hypercall continuation because its consumed offset is recorced in
guest domains memory space.
On the other hand, x86 guest_handle_add_offset() effect is volatile
over hypercall continuation.
So xenmem hypercall(more specifically increase_reservation,
decrease_reservaton, populate_memory and exchange) is broken on
ia64 and powerpc.
#ifdef/ifndef CONFIG_X86 is used to solve this issue without breaking
the existing ABI. #ifdef is ugly, but it would be difficult to solve
this issue without #ifdef and to preserve the existing ABI simaltaneously.

I checked other users of both guest_handle_add_offset() and hypercall
continuation. Fortunately other users records restart points
by hypercall argument so that this isn't an issue.
PATCHNAME: xencomm_and_xenmem_hypercall

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff -r 5470cadfeb22 -r f3e97d467b6f xen/common/memory.c
--- a/xen/common/memory.c	Fri Nov 10 14:22:05 2006 +0900
+++ b/xen/common/memory.c	Fri Nov 10 14:22:06 2006 +0900
@@ -341,23 +341,29 @@ memory_exchange(XEN_GUEST_HANDLE(xen_mem
         memflags = MEMF_dma;
     }
 
+#ifdef CONFIG_X86
     guest_handle_add_offset(exch.in.extent_start, exch.nr_exchanged);
+#endif
     exch.in.nr_extents -= exch.nr_exchanged;
 
     if ( exch.in.extent_order <= exch.out.extent_order )
     {
         in_chunk_order  = exch.out.extent_order - exch.in.extent_order;
         out_chunk_order = 0;
+#ifdef CONFIG_X86
         guest_handle_add_offset(
             exch.out.extent_start, exch.nr_exchanged >> in_chunk_order);
+#endif
         exch.out.nr_extents -= exch.nr_exchanged >> in_chunk_order;
     }
     else
     {
         in_chunk_order  = 0;
         out_chunk_order = exch.in.extent_order - exch.out.extent_order;
+#ifdef CONFIG_X86
         guest_handle_add_offset(
             exch.out.extent_start, exch.nr_exchanged << out_chunk_order);
+#endif
         exch.out.nr_extents -= exch.nr_exchanged << out_chunk_order;
     }
 
@@ -379,6 +385,15 @@ memory_exchange(XEN_GUEST_HANDLE(xen_mem
     {
         if ( hypercall_preempt_check() )
         {
+#ifndef CONFIG_X86
+            guest_handle_add_offset(exch.in.extent_start, i);
+            if ( exch.in.extent_order <= exch.out.extent_order )
+                guest_handle_add_offset(
+                    exch.out.extent_start, i >> in_chunk_order);
+            else
+                guest_handle_add_offset(
+                    exch.out.extent_start, i << out_chunk_order);
+#endif
             exch.nr_exchanged += i << in_chunk_order;
             if ( copy_field_to_guest(arg, &exch, nr_exchanged) )
                 return -EFAULT;
@@ -543,8 +558,10 @@ long do_memory_op(unsigned long cmd, XEN
         if ( unlikely(start_extent > reservation.nr_extents) )
             return start_extent;
 
+#ifdef CONFIG_X86
         if ( !guest_handle_is_null(reservation.extent_start) )
             guest_handle_add_offset(reservation.extent_start, start_extent);
+#endif
         reservation.nr_extents -= start_extent;
 
         if ( (reservation.address_bits != 0) &&
@@ -596,6 +613,10 @@ long do_memory_op(unsigned long cmd, XEN
         if ( unlikely(reservation.domid != DOMID_SELF) )
             put_domain(d);
 
+#ifndef CONFIG_X86
+        if (rc > 0 && !guest_handle_is_null(reservation.extent_start))
+            guest_handle_add_offset(reservation.extent_start, rc);
+#endif
         rc += start_extent;
 
         if ( preempted )

[-- 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] xencomm, xenmem and hypercall continuation

[Keir Fraser]

On 10/11/06 5:49 am, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:

> fix xenmem hypercall for non-trivial xencomm arch(i.e. ia64, and powerpc)
> On ia64 and powerpc, guest_handle_add_offset() effect persists over
> hypercall continuation because its consumed offset is recorced in
> guest domains memory space.
> On the other hand, x86 guest_handle_add_offset() effect is volatile
> over hypercall continuation.
> So xenmem hypercall(more specifically increase_reservation,
> decrease_reservaton, populate_memory and exchange) is broken on
> ia64 and powerpc.
> #ifdef/ifndef CONFIG_X86 is used to solve this issue without breaking
> the existing ABI. #ifdef is ugly, but it would be difficult to solve
> this issue without #ifdef and to preserve the existing ABI simaltaneously.

There must be a cleaner solution. We're not going to ifdef all over the
place.

Does xencomm have to persist the offset increments in guest memory (does the
guest depend on this)? Could it undo these effects across continuations so
that guest_handle_add_offset works properly?

 -- Keir

Re: [PATCH] xencomm, xenmem and hypercall continuation

[Isaku Yamahata]

On Fri, Nov 10, 2006 at 08:03:51AM +0000, Keir Fraser wrote:
> On 10/11/06 5:49 am, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:
> 
> > fix xenmem hypercall for non-trivial xencomm arch(i.e. ia64, and powerpc)
> > On ia64 and powerpc, guest_handle_add_offset() effect persists over
> > hypercall continuation because its consumed offset is recorced in
> > guest domains memory space.
> > On the other hand, x86 guest_handle_add_offset() effect is volatile
> > over hypercall continuation.
> > So xenmem hypercall(more specifically increase_reservation,
> > decrease_reservaton, populate_memory and exchange) is broken on
> > ia64 and powerpc.
> > #ifdef/ifndef CONFIG_X86 is used to solve this issue without breaking
> > the existing ABI. #ifdef is ugly, but it would be difficult to solve
> > this issue without #ifdef and to preserve the existing ABI simaltaneously.
> 
> There must be a cleaner solution. We're not going to ifdef all over the
> place.
>
> Does xencomm have to persist the offset increments in guest memory (does the
> guest depend on this)? 

The guest domain doesn't depend on this.
(at least it doesn't on ia64.
Probably on powerpc neither. please correct me if I'm wrong)
The other callers of guest_handle_add_offset() depend on this.
do_multicall() @ xen/common/multicall.c and
guest_console_write() @ xen/drivers/char/console.c.


> Could it undo these effects across continuations so
> that guest_handle_add_offset works properly?

- introduce guest_handle_putback_offset_for_continuation()
  (or something like that) and
  call it right before hypercall_create_continuation() in memory.c

  on x86 define it as nop.
  on ia64 and powerpc, define it as something to prepare 
  hypercall continuation.
  leave do_multicall() and guest_console_write() as is.

- introduce guest_handle_add_offset_after_continuatiton() and
  replace guest_handle_add_offset() in memory.c with it.
  leave do_multicall() and guest_console_write() as is.

- any other better idea?

-- 
yamahata

Re: [Xen-devel] [PATCH] xencomm, xenmem and hypercall continuation

[Keir Fraser]

On 10/11/06 08:45, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:

> - introduce guest_handle_add_offset_after_continuatiton() and
>   replace guest_handle_add_offset() in memory.c with it.
>   leave do_multicall() and guest_console_write() as is.

This is the best option I think. But I'm loathe to make it part of the
guest_handle API. We should avoid getting into this mess in the first place
for future hypercalls, so this will be a memory-specific function.

We should stick it at the top of memory.c, with a comment and make it a
no-op dependent on something like ARCH_HAS_XENCOMM (or just __ia64__ ||
__powerpc__ would be fine, actually, since it's just one place).

 -- Keir

Re: [Xen-devel] [PATCH] xencomm, xenmem and hypercall continuation

[Isaku Yamahata]

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

On Fri, Nov 10, 2006 at 09:57:44AM +0000, Keir Fraser wrote:
> On 10/11/06 08:45, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:
> 
> > - introduce guest_handle_add_offset_after_continuatiton() and
> >   replace guest_handle_add_offset() in memory.c with it.
> >   leave do_multicall() and guest_console_write() as is.
> 
> This is the best option I think. But I'm loathe to make it part of the
> guest_handle API. We should avoid getting into this mess in the first place
> for future hypercalls, so this will be a memory-specific function.
> 
> We should stick it at the top of memory.c, with a comment and make it a
> no-op dependent on something like ARCH_HAS_XENCOMM (or just __ia64__ ||
> __powerpc__ would be fine, actually, since it's just one place).

Here is the patch. Powerpc guys may have their own idea, though.
Unfortunately 4 #ifndef's are needed.

-- 
yamahata

[-- Attachment #2: 12347_9c2edde14184_xencomm_and_xenmem_hypercall.patch --]
[-- Type: text/plain, Size: 5581 bytes --]

# HG changeset patch
# User yamahata@valinux.co.jp
# Date 1163155082 -32400
# Node ID 9c2edde14184d7b5600b09f7571e6752ed097ae9
# Parent  b4e7365d451de6ffa84f24cfc29d59fea9aa50be
fix xenmem hypercall for non-trivial xencomm arch(i.e. ia64, and powerpc)
On ia64 and powerpc, guest_handle_add_offset() effect persists over
hypercall continuation because its consumed offset is recorced in
guest domains memory space.
On the other hand, x86 guest_handle_add_offset() effect is volatile
over hypercall continuation.
So xenmem hypercall(more specifically increase_reservation,
decrease_reservaton, populate_memory and exchange) is broken on
ia64 and powerpc.
On the other hand, do_multicall() @ xen/common/multicall.c and
guest_console_write() @ xen/drivers/char/console.c depends on
guest_handle_add_offset() behaviour.

#ifndef ARCH_HAS_XENCOMM is used to solve this issue without breaking
the existing ABI. #ifndef is ugly and ARCH_HAS_XENCOMM should be
used in only XENMEM hypercall.
PATCHNAME: xencomm_and_xenmem_hypercall

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff -r b4e7365d451d -r 9c2edde14184 xen/common/memory.c
--- a/xen/common/memory.c	Fri Nov 10 07:48:25 2006 +0000
+++ b/xen/common/memory.c	Fri Nov 10 19:38:02 2006 +0900
@@ -341,23 +341,35 @@ memory_exchange(XEN_GUEST_HANDLE(xen_mem
         memflags = MEMF_dma;
     }
 
+#ifndef ARCH_HAS_XENCOMM
+    /* on x86 its effect is volatile over hypercall continuation,
+     * so this adjustment is necessary.
+     * OTOH on ia64 and powerpc, guest_handle_add_offset() effects 
+     * persists over hypercall continuation so that this adjustment
+     * isn't necessary.
+     */
     guest_handle_add_offset(exch.in.extent_start, exch.nr_exchanged);
+#endif
     exch.in.nr_extents -= exch.nr_exchanged;
 
     if ( exch.in.extent_order <= exch.out.extent_order )
     {
         in_chunk_order  = exch.out.extent_order - exch.in.extent_order;
         out_chunk_order = 0;
+#ifndef ARCH_HAS_XENCOMM
         guest_handle_add_offset(
             exch.out.extent_start, exch.nr_exchanged >> in_chunk_order);
+#endif
         exch.out.nr_extents -= exch.nr_exchanged >> in_chunk_order;
     }
     else
     {
         in_chunk_order  = 0;
         out_chunk_order = exch.in.extent_order - exch.out.extent_order;
+#ifndef ARCH_HAS_XENCOMM
         guest_handle_add_offset(
             exch.out.extent_start, exch.nr_exchanged << out_chunk_order);
+#endif
         exch.out.nr_extents -= exch.nr_exchanged << out_chunk_order;
     }
 
@@ -379,6 +391,17 @@ memory_exchange(XEN_GUEST_HANDLE(xen_mem
     {
         if ( hypercall_preempt_check() )
         {
+            /* This is for ia64 and powerpc.
+             * on x86 this results in nop because its effect is volatile
+             * over hypercall continuation.
+             */
+            guest_handle_add_offset(exch.in.extent_start, i);
+            if ( exch.in.extent_order <= exch.out.extent_order )
+                guest_handle_add_offset(
+                    exch.out.extent_start, i >> in_chunk_order);
+            else
+                guest_handle_add_offset(
+                    exch.out.extent_start, i << out_chunk_order);
             exch.nr_exchanged += i << in_chunk_order;
             if ( copy_field_to_guest(arg, &exch, nr_exchanged) )
                 return -EFAULT;
@@ -543,8 +566,16 @@ long do_memory_op(unsigned long cmd, XEN
         if ( unlikely(start_extent > reservation.nr_extents) )
             return start_extent;
 
+#ifndef ARCH_HAS_XENCOMM
+        /* on x86 its effect is volatile over hypercall continuation,
+         * so this adjustment is necessary.
+         * OTOH on ia64 and powerpc, guest_handle_add_offset() effects 
+         * persists over hypercall continuation so that this adjustment
+         * isn't necessary.
+         */
         if ( !guest_handle_is_null(reservation.extent_start) )
             guest_handle_add_offset(reservation.extent_start, start_extent);
+#endif
         reservation.nr_extents -= start_extent;
 
         if ( (reservation.address_bits != 0) &&
@@ -596,6 +627,12 @@ long do_memory_op(unsigned long cmd, XEN
         if ( unlikely(reservation.domid != DOMID_SELF) )
             put_domain(d);
 
+        /* This is for ia64 and powerpc.
+         * on x86 this results in nop because its effect is volatile
+         * over hypercall continuation.
+         */
+        if ( rc > 0 && !guest_handle_is_null(reservation.extent_start) )
+            guest_handle_add_offset(reservation.extent_start, rc);
         rc += start_extent;
 
         if ( preempted )
diff -r b4e7365d451d -r 9c2edde14184 xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Fri Nov 10 07:48:25 2006 +0000
+++ b/xen/include/public/arch-ia64.h	Fri Nov 10 19:38:02 2006 +0900
@@ -26,6 +26,7 @@
 #ifndef __HYPERVISOR_IF_IA64_H__
 #define __HYPERVISOR_IF_IA64_H__
 
+#define ARCH_HAS_XENCOMM                1
 /* Structural guest handles introduced in 0x00030201. */
 #if __XEN_INTERFACE_VERSION__ >= 0x00030201
 #define __DEFINE_XEN_GUEST_HANDLE(name, type) \
diff -r b4e7365d451d -r 9c2edde14184 xen/include/public/arch-powerpc.h
--- a/xen/include/public/arch-powerpc.h	Fri Nov 10 07:48:25 2006 +0000
+++ b/xen/include/public/arch-powerpc.h	Fri Nov 10 19:38:02 2006 +0900
@@ -21,6 +21,7 @@
 #ifndef __XEN_PUBLIC_ARCH_PPC_64_H__
 #define __XEN_PUBLIC_ARCH_PPC_64_H__
 
+#define ARCH_HAS_XENCOMM                1
 #define __DEFINE_XEN_GUEST_HANDLE(name, type) \
     typedef struct { \
         int __pad[(sizeof (long long) - sizeof (void *)) / sizeof (int)]; \

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

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

Re: [Xen-devel] [PATCH] xencomm, xenmem and hypercall continuation

[Keir Fraser]

On 10/11/06 10:42, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:

>> This is the best option I think. But I'm loathe to make it part of the
>> guest_handle API. We should avoid getting into this mess in the first place
>> for future hypercalls, so this will be a memory-specific function.
>> 
>> We should stick it at the top of memory.c, with a comment and make it a
>> no-op dependent on something like ARCH_HAS_XENCOMM (or just __ia64__ ||
>> __powerpc__ would be fine, actually, since it's just one place).
> 
> Here is the patch. Powerpc guys may have their own idea, though.
> Unfortunately 4 #ifndef's are needed.

That's still pretty bad. I'll rewrite the hypercall to avoid using
guest_handle_add_offset().

 -- Keir