* [PATCH 0/2] x86/HVM: XSA-89 follow-ups
@ 2014-03-25 15:51 Jan Beulich
2014-03-25 16:03 ` [PATCH 1/2] x86/HVM: simplify do_hvm_op() Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Jan Beulich @ 2014-03-25 15:51 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
1: simplify do_hvm_op()
2: fix preemption handling in do_hvm_op()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] x86/HVM: simplify do_hvm_op()
2014-03-25 15:51 [PATCH 0/2] x86/HVM: XSA-89 follow-ups Jan Beulich
@ 2014-03-25 16:03 ` Jan Beulich
2014-03-25 17:26 ` Andrew Cooper
2014-03-25 16:03 ` [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op() Jan Beulich
2014-03-27 11:20 ` [PATCH 0/2] x86/HVM: XSA-89 follow-ups Tim Deegan
2 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2014-03-25 16:03 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]
- boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
HVMOP_set_mem_access: all of these already check for overflow, so
there's no need to range check the first _and_ last PFN (checking
the last one suffices)
- copying back interface structures that were previously copied from
guest memory can use __copy_to_...(), since copy_from_...() already
did the address range validation
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
a.value = d->arch.hvm_domain.params[a.index];
break;
}
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
}
HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
@@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail3;
rc = -EINVAL;
- if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail3;
@@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
/* Check for continuation if it's not the last interation */
if ( a.nr > 0 && hypercall_preempt_check() )
{
- if ( copy_to_guest(arg, &a, 1) )
+ if ( __copy_to_guest(arg, &a, 1) )
rc = -EFAULT;
else
rc = -EAGAIN;
@@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
a.mem_type = HVMMEM_ram_rw;
else
a.mem_type = HVMMEM_mmio_dm;
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
}
param_fail_getmemtype:
@@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail4;
rc = -EINVAL;
- if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail4;
@@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
/* Check for continuation if it's not the last interation */
if ( a.nr > 0 && hypercall_preempt_check() )
{
- if ( copy_to_guest(arg, &a, 1) )
+ if ( __copy_to_guest(arg, &a, 1) )
rc = -EFAULT;
else
rc = -EAGAIN;
@@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
rc = -EINVAL;
if ( (a.first_pfn != ~0ull) &&
- ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
- ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
+ (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
goto param_fail5;
rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
@@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail6;
a.hvmmem_access = access;
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
param_fail6:
rcu_unlock_domain(d);
[-- Attachment #2: x86-HVM-op-simplify.patch --]
[-- Type: text/plain, Size: 3813 bytes --]
x86/HVM: simplify do_hvm_op()
- boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
HVMOP_set_mem_access: all of these already check for overflow, so
there's no need to range check the first _and_ last PFN (checking
the last one suffices)
- copying back interface structures that were previously copied from
guest memory can use __copy_to_...(), since copy_from_...() already
did the address range validation
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
a.value = d->arch.hvm_domain.params[a.index];
break;
}
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
}
HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
@@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail3;
rc = -EINVAL;
- if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail3;
@@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
/* Check for continuation if it's not the last interation */
if ( a.nr > 0 && hypercall_preempt_check() )
{
- if ( copy_to_guest(arg, &a, 1) )
+ if ( __copy_to_guest(arg, &a, 1) )
rc = -EFAULT;
else
rc = -EAGAIN;
@@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
a.mem_type = HVMMEM_ram_rw;
else
a.mem_type = HVMMEM_mmio_dm;
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
}
param_fail_getmemtype:
@@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail4;
rc = -EINVAL;
- if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail4;
@@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
/* Check for continuation if it's not the last interation */
if ( a.nr > 0 && hypercall_preempt_check() )
{
- if ( copy_to_guest(arg, &a, 1) )
+ if ( __copy_to_guest(arg, &a, 1) )
rc = -EFAULT;
else
rc = -EAGAIN;
@@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
rc = -EINVAL;
if ( (a.first_pfn != ~0ull) &&
- ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
- ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
- ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
+ (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
goto param_fail5;
rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
@@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail6;
a.hvmmem_access = access;
- rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+ rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
param_fail6:
rcu_unlock_domain(d);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-25 15:51 [PATCH 0/2] x86/HVM: XSA-89 follow-ups Jan Beulich
2014-03-25 16:03 ` [PATCH 1/2] x86/HVM: simplify do_hvm_op() Jan Beulich
@ 2014-03-25 16:03 ` Jan Beulich
[not found] ` <CAGU+auuMgkBWobj=wpi6908mZuaYhHogc6j9F6gwdeUAGm=2DA@mail.gmail.com>
2014-03-26 14:42 ` George Dunlap
2014-03-27 11:20 ` [PATCH 0/2] x86/HVM: XSA-89 follow-ups Tim Deegan
2 siblings, 2 replies; 13+ messages in thread
From: Jan Beulich @ 2014-03-25 16:03 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 9492 bytes --]
Just like previously done for some mem-op hypercalls, undo preemption
using the interface structures (altering it in ways the caller may not
expect) and replace it by storing the continuation point in the high
bits of sub-operation argument.
This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
(operation already limited to 1Gb worth of pages) and struct
xen_hvm_modified_memory to be only 32 bits wide, consistent with those
of struct xen_set_mem{type,access}. If that's not acceptable for some
reason, we'd need to shrink the HVMOP_op_bits (while still enforcing
the [then higher] limit resulting from the need to be able to encode
the continuation).
Whether (and if so how) to adjust xc_hvm_track_dirty_vram(),
xc_hvm_modified_memory(), xc_hvm_set_mem_type(), and
xc_hvm_set_mem_access() to reflect the 32-bit restriction on "nr" is
unclear: If the APIs need to remain stable, all four functions should
probably check that there was no truncation. Preferably their
parameters would be changed to uint32_t or unsigned int, though.
As a minor cleanup, along with introducing the switch-wide "pfn" the
redundant "d" is also being converted to a switch-wide one.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4050,20 +4050,25 @@ static int hvm_replace_event_channel(str
return 0;
}
+#define HVMOP_op_bits 32
+
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
{
struct domain *curr_d = current->domain;
+ unsigned long start_iter = op >> HVMOP_op_bits;
long rc = 0;
- switch ( op )
+ switch ( op &= ((1UL << HVMOP_op_bits) - 1) )
{
+ struct domain *d;
+ unsigned long pfn;
+
case HVMOP_set_param:
case HVMOP_get_param:
{
struct xen_hvm_param a;
struct hvm_ioreq_page *iorp;
- struct domain *d;
struct vcpu *v;
if ( copy_from_guest(&a, arg, 1) )
@@ -4327,7 +4332,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_track_dirty_vram:
{
struct xen_hvm_track_dirty_vram a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4368,7 +4372,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_modified_memory:
{
struct xen_hvm_modified_memory a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4386,7 +4389,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail3;
rc = -EINVAL;
- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail3;
@@ -4394,9 +4398,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
if ( !paging_mode_log_dirty(d) )
goto param_fail3;
- while ( a.nr > 0 )
+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn )
{
- unsigned long pfn = a.first_pfn;
struct page_info *page;
page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
@@ -4409,16 +4412,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
put_page(page);
}
- a.first_pfn++;
- a.nr--;
+ ++pfn;
+ ++start_iter;
/* Check for continuation if it's not the last interation */
- if ( a.nr > 0 && hypercall_preempt_check() )
+ if ( a.nr > start_iter && hypercall_preempt_check() )
{
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ rc = -EAGAIN;
break;
}
}
@@ -4431,7 +4431,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_get_mem_type:
{
struct xen_hvm_get_mem_type a;
- struct domain *d;
p2m_type_t t;
if ( copy_from_guest(&a, arg, 1) )
@@ -4475,7 +4474,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_set_mem_type:
{
struct xen_hvm_set_mem_type a;
- struct domain *d;
/* Interface types to internal p2m types */
static const p2m_type_t memtype[] = {
@@ -4500,20 +4498,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail4;
rc = -EINVAL;
- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail4;
if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
goto param_fail4;
- while ( a.nr )
+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn )
{
- unsigned long pfn = a.first_pfn;
- p2m_type_t t;
- p2m_type_t nt;
- mfn_t mfn;
- mfn = get_gfn_unshare(d, pfn, &t);
+ p2m_type_t t, nt;
+
+ get_gfn_unshare(d, pfn, &t);
if ( p2m_is_paging(t) )
{
put_gfn(d, pfn);
@@ -4550,16 +4547,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
put_gfn(d, pfn);
- a.first_pfn++;
- a.nr--;
+ ++pfn;
+ ++start_iter;
/* Check for continuation if it's not the last interation */
- if ( a.nr > 0 && hypercall_preempt_check() )
+ if ( a.nr > start_iter && hypercall_preempt_check() )
{
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ rc = -EAGAIN;
goto param_fail4;
}
}
@@ -4574,7 +4568,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_set_mem_access:
{
struct xen_hvm_set_mem_access a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4593,19 +4586,17 @@ long do_hvm_op(unsigned long op, XEN_GUE
rc = -EINVAL;
if ( (a.first_pfn != ~0ull) &&
- (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ (a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
goto param_fail5;
- rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
+ rc = p2m_set_mem_access(d, a.first_pfn + start_iter, a.nr - start_iter,
+ a.hvmmem_access);
if ( rc > 0 )
{
- a.first_pfn += a.nr - rc;
- a.nr = rc;
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ start_iter = a.nr - rc;
+ rc = -EAGAIN;
}
param_fail5:
@@ -4616,7 +4607,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_get_mem_access:
{
struct xen_hvm_get_mem_access a;
- struct domain *d;
hvmmem_access_t access;
if ( copy_from_guest(&a, arg, 1) )
@@ -4653,7 +4643,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_pagetable_dying:
{
struct xen_hvm_pagetable_dying a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4706,7 +4695,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_inject_trap:
{
xen_hvm_inject_trap_t tr;
- struct domain *d;
struct vcpu *v;
if ( copy_from_guest(&tr, arg, 1 ) )
@@ -4754,8 +4742,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
if ( rc == -EAGAIN )
- rc = hypercall_create_continuation(
- __HYPERVISOR_hvm_op, "lh", op, arg);
+ rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+ op | (start_iter << HVMOP_op_bits),
+ arg);
return rc;
}
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -90,10 +90,10 @@ typedef enum {
struct xen_hvm_track_dirty_vram {
/* Domain to be tracked. */
domid_t domid;
+ /* Number of pages to track. */
+ uint32_t nr;
/* First pfn to track. */
uint64_aligned_t first_pfn;
- /* Number of pages to track. */
- uint64_aligned_t nr;
/* OUT variable. */
/* Dirty bitmap buffer. */
XEN_GUEST_HANDLE_64(uint8) dirty_bitmap;
@@ -106,10 +106,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_track_di
struct xen_hvm_modified_memory {
/* Domain to be updated. */
domid_t domid;
+ /* Number of pages. */
+ uint32_t nr;
/* First pfn. */
uint64_aligned_t first_pfn;
- /* Number of pages. */
- uint64_aligned_t nr;
};
typedef struct xen_hvm_modified_memory xen_hvm_modified_memory_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_modified_memory_t);
[-- Attachment #2: x86-HVM-op-preemption.patch --]
[-- Type: text/plain, Size: 9539 bytes --]
x86/HVM: fix preemption handling in do_hvm_op()
Just like previously done for some mem-op hypercalls, undo preemption
using the interface structures (altering it in ways the caller may not
expect) and replace it by storing the continuation point in the high
bits of sub-operation argument.
This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
(operation already limited to 1Gb worth of pages) and struct
xen_hvm_modified_memory to be only 32 bits wide, consistent with those
of struct xen_set_mem{type,access}. If that's not acceptable for some
reason, we'd need to shrink the HVMOP_op_bits (while still enforcing
the [then higher] limit resulting from the need to be able to encode
the continuation).
Whether (and if so how) to adjust xc_hvm_track_dirty_vram(),
xc_hvm_modified_memory(), xc_hvm_set_mem_type(), and
xc_hvm_set_mem_access() to reflect the 32-bit restriction on "nr" is
unclear: If the APIs need to remain stable, all four functions should
probably check that there was no truncation. Preferably their
parameters would be changed to uint32_t or unsigned int, though.
As a minor cleanup, along with introducing the switch-wide "pfn" the
redundant "d" is also being converted to a switch-wide one.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4050,20 +4050,25 @@ static int hvm_replace_event_channel(str
return 0;
}
+#define HVMOP_op_bits 32
+
long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
{
struct domain *curr_d = current->domain;
+ unsigned long start_iter = op >> HVMOP_op_bits;
long rc = 0;
- switch ( op )
+ switch ( op &= ((1UL << HVMOP_op_bits) - 1) )
{
+ struct domain *d;
+ unsigned long pfn;
+
case HVMOP_set_param:
case HVMOP_get_param:
{
struct xen_hvm_param a;
struct hvm_ioreq_page *iorp;
- struct domain *d;
struct vcpu *v;
if ( copy_from_guest(&a, arg, 1) )
@@ -4327,7 +4332,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_track_dirty_vram:
{
struct xen_hvm_track_dirty_vram a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4368,7 +4372,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_modified_memory:
{
struct xen_hvm_modified_memory a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4386,7 +4389,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail3;
rc = -EINVAL;
- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail3;
@@ -4394,9 +4398,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
if ( !paging_mode_log_dirty(d) )
goto param_fail3;
- while ( a.nr > 0 )
+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn )
{
- unsigned long pfn = a.first_pfn;
struct page_info *page;
page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
@@ -4409,16 +4412,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
put_page(page);
}
- a.first_pfn++;
- a.nr--;
+ ++pfn;
+ ++start_iter;
/* Check for continuation if it's not the last interation */
- if ( a.nr > 0 && hypercall_preempt_check() )
+ if ( a.nr > start_iter && hypercall_preempt_check() )
{
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ rc = -EAGAIN;
break;
}
}
@@ -4431,7 +4431,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_get_mem_type:
{
struct xen_hvm_get_mem_type a;
- struct domain *d;
p2m_type_t t;
if ( copy_from_guest(&a, arg, 1) )
@@ -4475,7 +4474,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_set_mem_type:
{
struct xen_hvm_set_mem_type a;
- struct domain *d;
/* Interface types to internal p2m types */
static const p2m_type_t memtype[] = {
@@ -4500,20 +4498,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
goto param_fail4;
rc = -EINVAL;
- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ if ( a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
goto param_fail4;
if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
goto param_fail4;
- while ( a.nr )
+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn )
{
- unsigned long pfn = a.first_pfn;
- p2m_type_t t;
- p2m_type_t nt;
- mfn_t mfn;
- mfn = get_gfn_unshare(d, pfn, &t);
+ p2m_type_t t, nt;
+
+ get_gfn_unshare(d, pfn, &t);
if ( p2m_is_paging(t) )
{
put_gfn(d, pfn);
@@ -4550,16 +4547,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
put_gfn(d, pfn);
- a.first_pfn++;
- a.nr--;
+ ++pfn;
+ ++start_iter;
/* Check for continuation if it's not the last interation */
- if ( a.nr > 0 && hypercall_preempt_check() )
+ if ( a.nr > start_iter && hypercall_preempt_check() )
{
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ rc = -EAGAIN;
goto param_fail4;
}
}
@@ -4574,7 +4568,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_set_mem_access:
{
struct xen_hvm_set_mem_access a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4593,19 +4586,17 @@ long do_hvm_op(unsigned long op, XEN_GUE
rc = -EINVAL;
if ( (a.first_pfn != ~0ull) &&
- (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+ (a.nr < start_iter ||
+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
goto param_fail5;
- rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
+ rc = p2m_set_mem_access(d, a.first_pfn + start_iter, a.nr - start_iter,
+ a.hvmmem_access);
if ( rc > 0 )
{
- a.first_pfn += a.nr - rc;
- a.nr = rc;
- if ( __copy_to_guest(arg, &a, 1) )
- rc = -EFAULT;
- else
- rc = -EAGAIN;
+ start_iter = a.nr - rc;
+ rc = -EAGAIN;
}
param_fail5:
@@ -4616,7 +4607,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_get_mem_access:
{
struct xen_hvm_get_mem_access a;
- struct domain *d;
hvmmem_access_t access;
if ( copy_from_guest(&a, arg, 1) )
@@ -4653,7 +4643,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_pagetable_dying:
{
struct xen_hvm_pagetable_dying a;
- struct domain *d;
if ( copy_from_guest(&a, arg, 1) )
return -EFAULT;
@@ -4706,7 +4695,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
case HVMOP_inject_trap:
{
xen_hvm_inject_trap_t tr;
- struct domain *d;
struct vcpu *v;
if ( copy_from_guest(&tr, arg, 1 ) )
@@ -4754,8 +4742,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
if ( rc == -EAGAIN )
- rc = hypercall_create_continuation(
- __HYPERVISOR_hvm_op, "lh", op, arg);
+ rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+ op | (start_iter << HVMOP_op_bits),
+ arg);
return rc;
}
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -90,10 +90,10 @@ typedef enum {
struct xen_hvm_track_dirty_vram {
/* Domain to be tracked. */
domid_t domid;
+ /* Number of pages to track. */
+ uint32_t nr;
/* First pfn to track. */
uint64_aligned_t first_pfn;
- /* Number of pages to track. */
- uint64_aligned_t nr;
/* OUT variable. */
/* Dirty bitmap buffer. */
XEN_GUEST_HANDLE_64(uint8) dirty_bitmap;
@@ -106,10 +106,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_track_di
struct xen_hvm_modified_memory {
/* Domain to be updated. */
domid_t domid;
+ /* Number of pages. */
+ uint32_t nr;
/* First pfn. */
uint64_aligned_t first_pfn;
- /* Number of pages. */
- uint64_aligned_t nr;
};
typedef struct xen_hvm_modified_memory xen_hvm_modified_memory_t;
DEFINE_XEN_GUEST_HANDLE(xen_hvm_modified_memory_t);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86/HVM: simplify do_hvm_op()
2014-03-25 16:03 ` [PATCH 1/2] x86/HVM: simplify do_hvm_op() Jan Beulich
@ 2014-03-25 17:26 ` Andrew Cooper
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2014-03-25 17:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 4107 bytes --]
On 25/03/14 16:03, Jan Beulich wrote:
> - boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
> HVMOP_set_mem_access: all of these already check for overflow, so
> there's no need to range check the first _and_ last PFN (checking
> the last one suffices)
> - copying back interface structures that were previously copied from
> guest memory can use __copy_to_...(), since copy_from_...() already
> did the address range validation
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> a.value = d->arch.hvm_domain.params[a.index];
> break;
> }
> - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> }
>
> HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
> @@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> goto param_fail3;
>
> rc = -EINVAL;
> - if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
> - ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> + if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
> goto param_fail3;
>
> @@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> /* Check for continuation if it's not the last interation */
> if ( a.nr > 0 && hypercall_preempt_check() )
> {
> - if ( copy_to_guest(arg, &a, 1) )
> + if ( __copy_to_guest(arg, &a, 1) )
> rc = -EFAULT;
> else
> rc = -EAGAIN;
> @@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> a.mem_type = HVMMEM_ram_rw;
> else
> a.mem_type = HVMMEM_mmio_dm;
> - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> }
>
> param_fail_getmemtype:
> @@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> goto param_fail4;
>
> rc = -EINVAL;
> - if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
> - ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> + if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
> goto param_fail4;
>
> @@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> /* Check for continuation if it's not the last interation */
> if ( a.nr > 0 && hypercall_preempt_check() )
> {
> - if ( copy_to_guest(arg, &a, 1) )
> + if ( __copy_to_guest(arg, &a, 1) )
> rc = -EFAULT;
> else
> rc = -EAGAIN;
> @@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
>
> rc = -EINVAL;
> if ( (a.first_pfn != ~0ull) &&
> - ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
> - ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> - ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> + (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> + ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> goto param_fail5;
>
> rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
> @@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
> goto param_fail6;
>
> a.hvmmem_access = access;
> - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>
> param_fail6:
> rcu_unlock_domain(d);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 4965 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
[not found] ` <CAGU+auuMgkBWobj=wpi6908mZuaYhHogc6j9F6gwdeUAGm=2DA@mail.gmail.com>
@ 2014-03-25 18:40 ` Aravindh Puthiyaparambil (aravindp)
2014-03-26 10:06 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Aravindh Puthiyaparambil (aravindp) @ 2014-03-25 18:40 UTC (permalink / raw)
To: Jan Beulich (JBeulich@suse.com), xen-devel
Cc: Tim Deegan (tim@xen.org), Keir Fraser (keir@xen.org)
>Just like previously done for some mem-op hypercalls, undo preemption
>using the interface structures (altering it in ways the caller may not
>expect) and replace it by storing the continuation point in the high bits of sub-
>operation argument.
>
>This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
>(operation already limited to 1Gb worth of pages) and struct
>xen_hvm_modified_memory to be only 32 bits wide, consistent with those of
>struct xen_set_mem{type,access}. If that's not acceptable for some reason,
>we'd need to shrink the HVMOP_op_bits (while still enforcing the [then
>higher] limit resulting from the need to be able to encode the continuation).
>
>Whether (and if so how) to adjust xc_hvm_track_dirty_vram(),
>xc_hvm_modified_memory(), xc_hvm_set_mem_type(), and
>xc_hvm_set_mem_access() to reflect the 32-bit restriction on "nr" is
>unclear: If the APIs need to remain stable, all four functions should probably
>check that there was no truncation. Preferably their parameters would be
>changed to uint32_t or unsigned int, though.
I am adding mem_access support to PV domains. As part of that I am adding a more generic xc_set/get_mem_access() APIs and deprecating the xc_hvm_set/get_mem_access() APIs. At the moment I have the xc_hvm_set/get_mem_access() APIs calling the newer ones. I am also folding the mem_access ops under XENMEM_access_op and deprecating the HVMOPs. Maybe as part of this I can change the nr parameter to uint32_t. Please advice and I will make it part of the patch which should be ready in a couple of weeks.
Thanks,
Aravindh
>As a minor cleanup, along with introducing the switch-wide "pfn" the
>redundant "d" is also being converted to a switch-wide one.
>
>Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>--- a/xen/arch/x86/hvm/hvm.c
>+++ b/xen/arch/x86/hvm/hvm.c
>@@ -4050,20 +4050,25 @@ static int hvm_replace_event_channel(str
> return 0;
> }
>
>+#define HVMOP_op_bits 32
>+
> long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>
> {
> struct domain *curr_d = current->domain;
>+ unsigned long start_iter = op >> HVMOP_op_bits;
> long rc = 0;
>
>- switch ( op )
>+ switch ( op &= ((1UL << HVMOP_op_bits) - 1) )
> {
>+ struct domain *d;
>+ unsigned long pfn;
>+
> case HVMOP_set_param:
> case HVMOP_get_param:
> {
> struct xen_hvm_param a;
> struct hvm_ioreq_page *iorp;
>- struct domain *d;
> struct vcpu *v;
>
> if ( copy_from_guest(&a, arg, 1) ) @@ -4327,7 +4332,6 @@ long
>do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_track_dirty_vram:
> {
> struct xen_hvm_track_dirty_vram a;
>- struct domain *d;
>
> if ( copy_from_guest(&a, arg, 1) )
> return -EFAULT;
>@@ -4368,7 +4372,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_modified_memory:
> {
> struct xen_hvm_modified_memory a;
>- struct domain *d;
>
> if ( copy_from_guest(&a, arg, 1) )
> return -EFAULT;
>@@ -4386,7 +4389,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
> goto param_fail3;
>
> rc = -EINVAL;
>- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>+ if ( a.nr < start_iter ||
>+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
> goto param_fail3;
>
>@@ -4394,9 +4398,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
> if ( !paging_mode_log_dirty(d) )
> goto param_fail3;
>
>- while ( a.nr > 0 )
>+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn
>+ )
> {
>- unsigned long pfn = a.first_pfn;
> struct page_info *page;
>
> page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE); @@ -
>4409,16 +4412,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
> put_page(page);
> }
>
>- a.first_pfn++;
>- a.nr--;
>+ ++pfn;
>+ ++start_iter;
>
> /* Check for continuation if it's not the last interation */
>- if ( a.nr > 0 && hypercall_preempt_check() )
>+ if ( a.nr > start_iter && hypercall_preempt_check() )
> {
>- if ( __copy_to_guest(arg, &a, 1) )
>- rc = -EFAULT;
>- else
>- rc = -EAGAIN;
>+ rc = -EAGAIN;
> break;
> }
> }
>@@ -4431,7 +4431,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_get_mem_type:
> {
> struct xen_hvm_get_mem_type a;
>- struct domain *d;
> p2m_type_t t;
>
> if ( copy_from_guest(&a, arg, 1) ) @@ -4475,7 +4474,6 @@ long
>do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_set_mem_type:
> {
> struct xen_hvm_set_mem_type a;
>- struct domain *d;
>
> /* Interface types to internal p2m types */
> static const p2m_type_t memtype[] = { @@ -4500,20 +4498,19 @@ long
>do_hvm_op(unsigned long op, XEN_GUE
> goto param_fail4;
>
> rc = -EINVAL;
>- if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>+ if ( a.nr < start_iter ||
>+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
> goto param_fail4;
>
> if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> goto param_fail4;
>
>- while ( a.nr )
>+ for ( pfn = a.first_pfn + start_iter; a.nr > start_iter; ++pfn
>+ )
> {
>- unsigned long pfn = a.first_pfn;
>- p2m_type_t t;
>- p2m_type_t nt;
>- mfn_t mfn;
>- mfn = get_gfn_unshare(d, pfn, &t);
>+ p2m_type_t t, nt;
>+
>+ get_gfn_unshare(d, pfn, &t);
> if ( p2m_is_paging(t) )
> {
> put_gfn(d, pfn);
>@@ -4550,16 +4547,13 @@ long do_hvm_op(unsigned long op, XEN_GUE
> }
> put_gfn(d, pfn);
>
>- a.first_pfn++;
>- a.nr--;
>+ ++pfn;
>+ ++start_iter;
>
> /* Check for continuation if it's not the last interation */
>- if ( a.nr > 0 && hypercall_preempt_check() )
>+ if ( a.nr > start_iter && hypercall_preempt_check() )
> {
>- if ( __copy_to_guest(arg, &a, 1) )
>- rc = -EFAULT;
>- else
>- rc = -EAGAIN;
>+ rc = -EAGAIN;
> goto param_fail4;
> }
> }
>@@ -4574,7 +4568,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_set_mem_access:
> {
> struct xen_hvm_set_mem_access a;
>- struct domain *d;
>
> if ( copy_from_guest(&a, arg, 1) )
> return -EFAULT;
>@@ -4593,19 +4586,17 @@ long do_hvm_op(unsigned long op, XEN_GUE
>
> rc = -EINVAL;
> if ( (a.first_pfn != ~0ull) &&
>- (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
>+ (a.nr < start_iter ||
>+ ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> goto param_fail5;
>
>- rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
>+ rc = p2m_set_mem_access(d, a.first_pfn + start_iter, a.nr - start_iter,
>+ a.hvmmem_access);
> if ( rc > 0 )
> {
>- a.first_pfn += a.nr - rc;
>- a.nr = rc;
>- if ( __copy_to_guest(arg, &a, 1) )
>- rc = -EFAULT;
>- else
>- rc = -EAGAIN;
>+ start_iter = a.nr - rc;
>+ rc = -EAGAIN;
> }
>
> param_fail5:
>@@ -4616,7 +4607,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_get_mem_access:
> {
> struct xen_hvm_get_mem_access a;
>- struct domain *d;
> hvmmem_access_t access;
>
> if ( copy_from_guest(&a, arg, 1) ) @@ -4653,7 +4643,6 @@ long
>do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_pagetable_dying:
> {
> struct xen_hvm_pagetable_dying a;
>- struct domain *d;
>
> if ( copy_from_guest(&a, arg, 1) )
> return -EFAULT;
>@@ -4706,7 +4695,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
> case HVMOP_inject_trap:
> {
> xen_hvm_inject_trap_t tr;
>- struct domain *d;
> struct vcpu *v;
>
> if ( copy_from_guest(&tr, arg, 1 ) ) @@ -4754,8 +4742,9 @@ long
>do_hvm_op(unsigned long op, XEN_GUE
> }
>
> if ( rc == -EAGAIN )
>- rc = hypercall_create_continuation(
>- __HYPERVISOR_hvm_op, "lh", op, arg);
>+ rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>+ op | (start_iter << HVMOP_op_bits),
>+ arg);
>
> return rc;
> }
>--- a/xen/include/public/hvm/hvm_op.h
>+++ b/xen/include/public/hvm/hvm_op.h
>@@ -90,10 +90,10 @@ typedef enum {
> struct xen_hvm_track_dirty_vram {
> /* Domain to be tracked. */
> domid_t domid;
>+ /* Number of pages to track. */
>+ uint32_t nr;
> /* First pfn to track. */
> uint64_aligned_t first_pfn;
>- /* Number of pages to track. */
>- uint64_aligned_t nr;
> /* OUT variable. */
> /* Dirty bitmap buffer. */
> XEN_GUEST_HANDLE_64(uint8) dirty_bitmap; @@ -106,10 +106,10 @@
>DEFINE_XEN_GUEST_HANDLE(xen_hvm_track_di
> struct xen_hvm_modified_memory {
> /* Domain to be updated. */
> domid_t domid;
>+ /* Number of pages. */
>+ uint32_t nr;
> /* First pfn. */
> uint64_aligned_t first_pfn;
>- /* Number of pages. */
>- uint64_aligned_t nr;
> };
> typedef struct xen_hvm_modified_memory xen_hvm_modified_memory_t;
>DEFINE_XEN_GUEST_HANDLE(xen_hvm_modified_memory_t);
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-25 18:40 ` Aravindh Puthiyaparambil (aravindp)
@ 2014-03-26 10:06 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2014-03-26 10:06 UTC (permalink / raw)
To: Aravindh Puthiyaparambil (aravindp), xen-devel
Cc: Tim Deegan (tim@xen.org), Keir Fraser (keir@xen.org)
>>> On 25.03.14 at 19:40, <aravindp@cisco.com> wrote:
>> Just like previously done for some mem-op hypercalls, undo preemption
>>using the interface structures (altering it in ways the caller may not
>>expect) and replace it by storing the continuation point in the high bits of
> sub-
>>operation argument.
>>
>>This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
>>(operation already limited to 1Gb worth of pages) and struct
>>xen_hvm_modified_memory to be only 32 bits wide, consistent with those of
>>struct xen_set_mem{type,access}. If that's not acceptable for some reason,
>>we'd need to shrink the HVMOP_op_bits (while still enforcing the [then
>>higher] limit resulting from the need to be able to encode the continuation).
>>
>>Whether (and if so how) to adjust xc_hvm_track_dirty_vram(),
>>xc_hvm_modified_memory(), xc_hvm_set_mem_type(), and
>>xc_hvm_set_mem_access() to reflect the 32-bit restriction on "nr" is
>>unclear: If the APIs need to remain stable, all four functions should
> probably
>>check that there was no truncation. Preferably their parameters would be
>>changed to uint32_t or unsigned int, though.
>
> I am adding mem_access support to PV domains. As part of that I am adding a
> more generic xc_set/get_mem_access() APIs and deprecating the
> xc_hvm_set/get_mem_access() APIs. At the moment I have the
> xc_hvm_set/get_mem_access() APIs calling the newer ones. I am also folding
> the mem_access ops under XENMEM_access_op and deprecating the HVMOPs. Maybe
> as part of this I can change the nr parameter to uint32_t. Please advice and
> I will make it part of the patch which should be ready in a couple of weeks.
You'll need to think about how to handle preemption anyway there,
and I would guess that you'd want to go with the same method used
here (due to the alternatives being more complicated). Hence you'll
need to introduce an upper bound on the number of pages anyway,
and one way that could be done would be by limiting the respective
interface structure field to 32 bits.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-25 16:03 ` [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op() Jan Beulich
[not found] ` <CAGU+auuMgkBWobj=wpi6908mZuaYhHogc6j9F6gwdeUAGm=2DA@mail.gmail.com>
@ 2014-03-26 14:42 ` George Dunlap
2014-03-26 15:32 ` Ian Campbell
2014-03-26 15:44 ` Jan Beulich
1 sibling, 2 replies; 13+ messages in thread
From: George Dunlap @ 2014-03-26 14:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Anthony PERARD, xen-devel, Keir Fraser, Ian Jackson, Stefano Stabellini
On Tue, Mar 25, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> Just like previously done for some mem-op hypercalls, undo preemption
> using the interface structures (altering it in ways the caller may not
> expect) and replace it by storing the continuation point in the high
> bits of sub-operation argument.
>
> This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
> (operation already limited to 1Gb worth of pages) and struct
> xen_hvm_modified_memory to be only 32 bits wide, consistent with those
> of struct xen_set_mem{type,access}. If that's not acceptable for some
> reason, we'd need to shrink the HVMOP_op_bits (while still enforcing
> the [then higher] limit resulting from the need to be able to encode
> the continuation).
So these calls are made directly by qemu -- and I thought that we were
trying to have a stable ABI with qemu so that people could pop in any
(recent) version of qemu and have it Just Work.
Would this have an impact on that? It seems like if qemu is
dynamically linked to libxc, and libxc is matched to an appropriate
hypervisor, that shouldn't be an issue.
However, at the moment libxc exposes "nr" as uint64_t; leaving it that
way would be very misleading.
Is there a real problem with "altering [interface structures] in a way
the caller may not expect"? Can't we just document that Xen may
change some of the structures?
-George
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-26 14:42 ` George Dunlap
@ 2014-03-26 15:32 ` Ian Campbell
2014-03-26 15:44 ` Jan Beulich
1 sibling, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-26 15:32 UTC (permalink / raw)
To: George Dunlap
Cc: Keir Fraser, Ian Jackson, Stefano Stabellini, Jan Beulich,
Anthony PERARD, xen-devel
On Wed, 2014-03-26 at 14:42 +0000, George Dunlap wrote:
> On Tue, Mar 25, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > Just like previously done for some mem-op hypercalls, undo preemption
> > using the interface structures (altering it in ways the caller may not
> > expect) and replace it by storing the continuation point in the high
> > bits of sub-operation argument.
> >
> > This also changes the "nr" fields of struct xen_hvm_track_dirty_vram
> > (operation already limited to 1Gb worth of pages) and struct
> > xen_hvm_modified_memory to be only 32 bits wide, consistent with those
> > of struct xen_set_mem{type,access}. If that's not acceptable for some
> > reason, we'd need to shrink the HVMOP_op_bits (while still enforcing
> > the [then higher] limit resulting from the need to be able to encode
> > the continuation).
>
> So these calls are made directly by qemu -- and I thought that we were
> trying to have a stable ABI with qemu so that people could pop in any
> (recent) version of qemu and have it Just Work.
I don't think we've said that, we've always been quite explicit that
libxc is an unstable API.
AFAIK qemu contains a compat shim which allows it to compile against
multiple libxc versions. If libxc is to change again then qemu might
need updating.
To some extent we've resisted gratuitous cleanups etc to libxc if they
would impact e.g. qemu, but if a change is required then we should go
ahead with it (NB: I've not looked into the actual proposal in this
thread to know which it is).
Maybe we should make stronger guarantees about interfaces used by device
models but that process would have to start with an enumeration of
exactly what those interfaces are. *If* we were to then go ahead with
that then I would advocate moving those interfaces into a library other
than libxc so it is clear what is what.
Ian.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-26 14:42 ` George Dunlap
2014-03-26 15:32 ` Ian Campbell
@ 2014-03-26 15:44 ` Jan Beulich
2014-03-26 17:06 ` George Dunlap
1 sibling, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2014-03-26 15:44 UTC (permalink / raw)
To: George Dunlap
Cc: Anthony PERARD, Ian Jackson, Keir Fraser, Stefano Stabellini, xen-devel
>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
> Is there a real problem with "altering [interface structures] in a way
> the caller may not expect"? Can't we just document that Xen may
> change some of the structures?
The two main problems here, just like with the memory op we had to
change before 4.4, are
- we can't and shouldn't assume that qemu is the only consumer, and
hence surprises to the caller must be avoided
- no interface anywhere in the tree should set bad precedents, or
else keeping people from doing the same wrong thing elsewhere is
much harder to achieve (i.e. just like in so many other places
recently, I'm advocating for consistency throughout the source
base here)
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-26 15:44 ` Jan Beulich
@ 2014-03-26 17:06 ` George Dunlap
2014-03-27 8:05 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: George Dunlap @ 2014-03-26 17:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Anthony PERARD, Ian Jackson, Keir Fraser, Stefano Stabellini, xen-devel
On 03/26/2014 03:44 PM, Jan Beulich wrote:
>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
>> Is there a real problem with "altering [interface structures] in a way
>> the caller may not expect"? Can't we just document that Xen may
>> change some of the structures?
> The two main problems here, just like with the memory op we had to
> change before 4.4, are
> - we can't and shouldn't assume that qemu is the only consumer, and
> hence surprises to the caller must be avoided
> - no interface anywhere in the tree should set bad precedents, or
> else keeping people from doing the same wrong thing elsewhere is
> much harder to achieve (i.e. just like in so many other places
> recently, I'm advocating for consistency throughout the source
> base here)
"Surprises to the caller" can be avoided by adding comments in the
public header. The hypercall can't be used without someone looking at
the structure definition. :-)
"Setting a bad precedent" is a reasonable argument *if* we're not
constrained by ABI / API compatibility. I agree with IanC, that libxc
should be an internal-only, unstable interface; and that we should
probably come up with some more reasonable way to support "external"
APIs like this for device models.
-George
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-26 17:06 ` George Dunlap
@ 2014-03-27 8:05 ` Jan Beulich
2014-03-27 10:16 ` Ian Campbell
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2014-03-27 8:05 UTC (permalink / raw)
To: George Dunlap
Cc: Anthony PERARD, Ian Jackson, Keir Fraser, Stefano Stabellini, xen-devel
>>> On 26.03.14 at 18:06, <george.dunlap@eu.citrix.com> wrote:
> On 03/26/2014 03:44 PM, Jan Beulich wrote:
>>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
>>> Is there a real problem with "altering [interface structures] in a way
>>> the caller may not expect"? Can't we just document that Xen may
>>> change some of the structures?
>> The two main problems here, just like with the memory op we had to
>> change before 4.4, are
>> - we can't and shouldn't assume that qemu is the only consumer, and
>> hence surprises to the caller must be avoided
>> - no interface anywhere in the tree should set bad precedents, or
>> else keeping people from doing the same wrong thing elsewhere is
>> much harder to achieve (i.e. just like in so many other places
>> recently, I'm advocating for consistency throughout the source
>> base here)
>
> "Surprises to the caller" can be avoided by adding comments in the
> public header. The hypercall can't be used without someone looking at
> the structure definition. :-)
But see - the public header doesn't have any such comments right
now, and hence I'm implying it to state that the structures don't
change except for fields designated for output.
> "Setting a bad precedent" is a reasonable argument *if* we're not
> constrained by ABI / API compatibility. I agree with IanC, that libxc
> should be an internal-only, unstable interface; and that we should
> probably come up with some more reasonable way to support "external"
> APIs like this for device models.
And again - you don't mention this aspect at all - consistency is quite
significant an aspect. And the interface stability requirements at the
libxc layer are completely independent of the ones of the hypercalls.
Back when I joined the Xen project, it was already the case that tools-
only interfaces were allowed to change at any time, and to my
knowledge this didn't change. Hence if we fix the hypercalls here and
libxc would have to retain its current interface, we'd need to adjust
libxc accordingly (as said in the original submission, some change to
the library is going to be at least desirable anyway).
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op()
2014-03-27 8:05 ` Jan Beulich
@ 2014-03-27 10:16 ` Ian Campbell
0 siblings, 0 replies; 13+ messages in thread
From: Ian Campbell @ 2014-03-27 10:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, George Dunlap, Ian Jackson, Stefano Stabellini,
Anthony PERARD, xen-devel
On Thu, 2014-03-27 at 08:05 +0000, Jan Beulich wrote:
> >>> On 26.03.14 at 18:06, <george.dunlap@eu.citrix.com> wrote:
> > On 03/26/2014 03:44 PM, Jan Beulich wrote:
> >>>>> On 26.03.14 at 15:42, <George.Dunlap@eu.citrix.com> wrote:
> >>> Is there a real problem with "altering [interface structures] in a way
> >>> the caller may not expect"? Can't we just document that Xen may
> >>> change some of the structures?
> >> The two main problems here, just like with the memory op we had to
> >> change before 4.4, are
> >> - we can't and shouldn't assume that qemu is the only consumer, and
> >> hence surprises to the caller must be avoided
> >> - no interface anywhere in the tree should set bad precedents, or
> >> else keeping people from doing the same wrong thing elsewhere is
> >> much harder to achieve (i.e. just like in so many other places
> >> recently, I'm advocating for consistency throughout the source
> >> base here)
> >
> > "Surprises to the caller" can be avoided by adding comments in the
> > public header. The hypercall can't be used without someone looking at
> > the structure definition. :-)
>
> But see - the public header doesn't have any such comments right
> now, and hence I'm implying it to state that the structures don't
> change except for fields designated for output.
>
> > "Setting a bad precedent" is a reasonable argument *if* we're not
> > constrained by ABI / API compatibility. I agree with IanC, that libxc
> > should be an internal-only, unstable interface; and that we should
> > probably come up with some more reasonable way to support "external"
> > APIs like this for device models.
>
> And again - you don't mention this aspect at all - consistency is quite
> significant an aspect. And the interface stability requirements at the
> libxc layer are completely independent of the ones of the hypercalls.
> Back when I joined the Xen project, it was already the case that tools-
> only interfaces were allowed to change at any time, and to my
> knowledge this didn't change.
Yes, to be clear I was only talking about the libxc A[BP]I.
I think for the purposes of supporting external device models and such
we would only need to declare a stable library wrapper interface, not
change any of the rules regarding the underlying hypercalls (i.e.
sysctl, domctl remain non-stable interfaces). If the hypercalls change
*and* we have declared a stable library interface over them then the
library would have to do impedance matching.
That's part of the reason I would insist on an enumeration and audit of
the library interfaces which we were to declare stable for the use of
qemu etc. As with the libxl stable API declaration we would want to make
sure that the interface was supportable as a frozen API going forward
before committing to it long term. I very much doubt that is the case of
many of the libxc interfaces right now.
Ian.
> Hence if we fix the hypercalls here and
> libxc would have to retain its current interface, we'd need to adjust
> libxc accordingly (as said in the original submission, some change to
> the library is going to be at least desirable anyway).
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] x86/HVM: XSA-89 follow-ups
2014-03-25 15:51 [PATCH 0/2] x86/HVM: XSA-89 follow-ups Jan Beulich
2014-03-25 16:03 ` [PATCH 1/2] x86/HVM: simplify do_hvm_op() Jan Beulich
2014-03-25 16:03 ` [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op() Jan Beulich
@ 2014-03-27 11:20 ` Tim Deegan
2 siblings, 0 replies; 13+ messages in thread
From: Tim Deegan @ 2014-03-27 11:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
At 15:51 +0000 on 25 Mar (1395759111), Jan Beulich wrote:
> 1: simplify do_hvm_op()
> 2: fix preemption handling in do_hvm_op()
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-03-27 11:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-25 15:51 [PATCH 0/2] x86/HVM: XSA-89 follow-ups Jan Beulich
2014-03-25 16:03 ` [PATCH 1/2] x86/HVM: simplify do_hvm_op() Jan Beulich
2014-03-25 17:26 ` Andrew Cooper
2014-03-25 16:03 ` [PATCH 2/2] x86/HVM: fix preemption handling in do_hvm_op() Jan Beulich
[not found] ` <CAGU+auuMgkBWobj=wpi6908mZuaYhHogc6j9F6gwdeUAGm=2DA@mail.gmail.com>
2014-03-25 18:40 ` Aravindh Puthiyaparambil (aravindp)
2014-03-26 10:06 ` Jan Beulich
2014-03-26 14:42 ` George Dunlap
2014-03-26 15:32 ` Ian Campbell
2014-03-26 15:44 ` Jan Beulich
2014-03-26 17:06 ` George Dunlap
2014-03-27 8:05 ` Jan Beulich
2014-03-27 10:16 ` Ian Campbell
2014-03-27 11:20 ` [PATCH 0/2] x86/HVM: XSA-89 follow-ups Tim Deegan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.