Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
>>> On 22.04.14 at 20:34, wrote: > With this patch: > [...] > And with an HVM guest poking at the rest of VCPUOPs: VCPUOP_get_physid, > VCPUOP_initialise, and VCPUOP_send_nmi - either before the CPU is up or > when it is up - work. > > That is: the VCPUOP_get_physid would return -EINVAL; VCPUOP_initialise > would return either -EEXIST or 0, and in either case > the content of the ctxt was full of zeros. Good. > The VCPUOP_send_nmi did cause the HVM to get an NMI and it spitted out > 'Dazed and confused'. It also noticed corruption: > > [3.611742] Corrupted low memory at c000fffc (fffc phys) = 00029b00 > [2.386785] Corrupted low memory at 8800fff8 (fff8 phys) = > 29900 > > Which is odd because there does not seem to be anything in the path > of hypervisor that would cause this. Indeed. This looks a little like a segment descriptor got modified here with a descriptor table base of zero and a selector of 0xfff8. That corruption needs to be hunted down in any case before enabling VCPUOP_send_nmi for HVM. Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On 22.04.14 at 20:34, konrad.w...@oracle.com wrote: With this patch: [...] And with an HVM guest poking at the rest of VCPUOPs: VCPUOP_get_physid, VCPUOP_initialise, and VCPUOP_send_nmi - either before the CPU is up or when it is up - work. That is: the VCPUOP_get_physid would return -EINVAL; VCPUOP_initialise would return either -EEXIST or 0, and in either case the content of the ctxt was full of zeros. Good. The VCPUOP_send_nmi did cause the HVM to get an NMI and it spitted out 'Dazed and confused'. It also noticed corruption: [3.611742] Corrupted low memory at c000fffc (fffc phys) = 00029b00 [2.386785] Corrupted low memory at 8800fff8 (fff8 phys) = 29900 Which is odd because there does not seem to be anything in the path of hypervisor that would cause this. Indeed. This looks a little like a segment descriptor got modified here with a descriptor table base of zero and a selector of 0xfff8. That corruption needs to be hunted down in any case before enabling VCPUOP_send_nmi for HVM. Jan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On Wed, Apr 09, 2014 at 04:36:53PM +0100, Jan Beulich wrote:
> >>> On 09.04.14 at 17:27, wrote:
> > On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote:
> >> >>> On 08.04.14 at 19:25, wrote:
> >> > --- a/xen/arch/x86/hvm/hvm.c
> >> > +++ b/xen/arch/x86/hvm/hvm.c
> >> > @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op(
> >> > case VCPUOP_stop_singleshot_timer:
> >> > case VCPUOP_register_vcpu_info:
> >> > case VCPUOP_register_vcpu_time_memory_area:
> >> > +case VCPUOP_down:
> >> > +case VCPUOP_up:
> >> > +case VCPUOP_is_up:
> >>
> >> This, if I checked it properly, leaves only VCPUOP_initialise,
> >> VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM.
> >> None of which look inherently bad to be used by HVM (but
> >> VCPUOP_initialise certainly would need closer checking), so I
> >> wonder whether either the wrapper shouldn't be dropped altogether
> >> or at least be converted from a white list approach to a black list one.
> >
> > I was being conservative here because I did not want to allow the
> > other ones without at least testing it.
> >
> > Perhaps that can be done as a seperate patch and this just as
> > a bug-fix?
>
> I'm clearly not in favor of the patch as is - minimally I'd want it to
> convert the white list to a black list. And once you do this it would
> seem rather natural to not pointlessly add entries.
With this patch:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b5b92fe..5eee790 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3455,34 +3455,6 @@ static long hvm_physdev_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
}
}
-static long hvm_vcpu_op(
-int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-long rc;
-
-switch ( cmd )
-{
-case VCPUOP_register_runstate_memory_area:
-case VCPUOP_get_runstate_info:
-case VCPUOP_set_periodic_timer:
-case VCPUOP_stop_periodic_timer:
-case VCPUOP_set_singleshot_timer:
-case VCPUOP_stop_singleshot_timer:
-case VCPUOP_register_vcpu_info:
-case VCPUOP_register_vcpu_time_memory_area:
-case VCPUOP_down:
-case VCPUOP_up:
-case VCPUOP_is_up:
-rc = do_vcpu_op(cmd, vcpuid, arg);
-break;
-default:
-rc = -ENOSYS;
-break;
-}
-
-return rc;
-}
-
typedef unsigned long hvm_hypercall_t(
unsigned long, unsigned long, unsigned long, unsigned long, unsigned long,
unsigned long);
@@ -3517,30 +3489,6 @@ static long hvm_memory_op_compat32(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
return compat_memory_op(cmd, arg);
}
-static long hvm_vcpu_op_compat32(
-int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-long rc;
-
-switch ( cmd )
-{
-case VCPUOP_register_runstate_memory_area:
-case VCPUOP_get_runstate_info:
-case VCPUOP_set_periodic_timer:
-case VCPUOP_stop_periodic_timer:
-case VCPUOP_set_singleshot_timer:
-case VCPUOP_stop_singleshot_timer:
-case VCPUOP_register_vcpu_info:
-case VCPUOP_register_vcpu_time_memory_area:
-rc = compat_vcpu_op(cmd, vcpuid, arg);
-break;
-default:
-rc = -ENOSYS;
-break;
-}
-
-return rc;
-}
static long hvm_physdev_op_compat32(
int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -3563,7 +3511,7 @@ static long hvm_physdev_op_compat32(
static hvm_hypercall_t *const hvm_hypercall64_table[NR_hypercalls] = {
[ __HYPERVISOR_memory_op ] = (hvm_hypercall_t *)hvm_memory_op,
[ __HYPERVISOR_grant_table_op ] = (hvm_hypercall_t *)hvm_grant_table_op,
-[ __HYPERVISOR_vcpu_op ] = (hvm_hypercall_t *)hvm_vcpu_op,
+HYPERCALL(vcpu_op),
[ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op,
HYPERCALL(xen_version),
HYPERCALL(console_io),
@@ -3583,7 +3531,7 @@ static hvm_hypercall_t *const
hvm_hypercall64_table[NR_hypercalls] = {
static hvm_hypercall_t *const hvm_hypercall32_table[NR_hypercalls] = {
[ __HYPERVISOR_memory_op ] = (hvm_hypercall_t *)hvm_memory_op_compat32,
[ __HYPERVISOR_grant_table_op ] = (hvm_hypercall_t
*)hvm_grant_table_op_compat32,
-[ __HYPERVISOR_vcpu_op ] = (hvm_hypercall_t *)hvm_vcpu_op_compat32,
+COMPAT_CALL(vcpu_op),
[ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op_compat32,
COMPAT_CALL(xen_version),
HYPERCALL(console_io),
And with an HVM guest poking at the rest of VCPUOPs: VCPUOP_get_physid,
VCPUOP_initialise, and VCPUOP_send_nmi - either before the CPU is up or
when it is up - work.
That is: the VCPUOP_get_physid would return -EINVAL; VCPUOP_initialise
would return either -EEXIST or 0, and in either case
the content of the ctxt was full of zeros.
The VCPUOP_send_nmi did cause the HVM to get an NMI and it spitted out
'Dazed and confused'. It also noticed corruption:
[3.611742] Corrupted low memory at c000fffc (fffc phys) = 00029b00
[2.386785] Corrupted low memory at 8800fff8 (fff8 phys) =
29900
Which
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On Wed, Apr 09, 2014 at 04:36:53PM +0100, Jan Beulich wrote:
On 09.04.14 at 17:27, konrad.w...@oracle.com wrote:
On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote:
On 08.04.14 at 19:25, kon...@kernel.org wrote:
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3470,6 +3470,9 @@ static long hvm_vcpu_op(
case VCPUOP_stop_singleshot_timer:
case VCPUOP_register_vcpu_info:
case VCPUOP_register_vcpu_time_memory_area:
+case VCPUOP_down:
+case VCPUOP_up:
+case VCPUOP_is_up:
This, if I checked it properly, leaves only VCPUOP_initialise,
VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM.
None of which look inherently bad to be used by HVM (but
VCPUOP_initialise certainly would need closer checking), so I
wonder whether either the wrapper shouldn't be dropped altogether
or at least be converted from a white list approach to a black list one.
I was being conservative here because I did not want to allow the
other ones without at least testing it.
Perhaps that can be done as a seperate patch and this just as
a bug-fix?
I'm clearly not in favor of the patch as is - minimally I'd want it to
convert the white list to a black list. And once you do this it would
seem rather natural to not pointlessly add entries.
With this patch:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b5b92fe..5eee790 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3455,34 +3455,6 @@ static long hvm_physdev_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
}
}
-static long hvm_vcpu_op(
-int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-long rc;
-
-switch ( cmd )
-{
-case VCPUOP_register_runstate_memory_area:
-case VCPUOP_get_runstate_info:
-case VCPUOP_set_periodic_timer:
-case VCPUOP_stop_periodic_timer:
-case VCPUOP_set_singleshot_timer:
-case VCPUOP_stop_singleshot_timer:
-case VCPUOP_register_vcpu_info:
-case VCPUOP_register_vcpu_time_memory_area:
-case VCPUOP_down:
-case VCPUOP_up:
-case VCPUOP_is_up:
-rc = do_vcpu_op(cmd, vcpuid, arg);
-break;
-default:
-rc = -ENOSYS;
-break;
-}
-
-return rc;
-}
-
typedef unsigned long hvm_hypercall_t(
unsigned long, unsigned long, unsigned long, unsigned long, unsigned long,
unsigned long);
@@ -3517,30 +3489,6 @@ static long hvm_memory_op_compat32(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
return compat_memory_op(cmd, arg);
}
-static long hvm_vcpu_op_compat32(
-int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
-{
-long rc;
-
-switch ( cmd )
-{
-case VCPUOP_register_runstate_memory_area:
-case VCPUOP_get_runstate_info:
-case VCPUOP_set_periodic_timer:
-case VCPUOP_stop_periodic_timer:
-case VCPUOP_set_singleshot_timer:
-case VCPUOP_stop_singleshot_timer:
-case VCPUOP_register_vcpu_info:
-case VCPUOP_register_vcpu_time_memory_area:
-rc = compat_vcpu_op(cmd, vcpuid, arg);
-break;
-default:
-rc = -ENOSYS;
-break;
-}
-
-return rc;
-}
static long hvm_physdev_op_compat32(
int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -3563,7 +3511,7 @@ static long hvm_physdev_op_compat32(
static hvm_hypercall_t *const hvm_hypercall64_table[NR_hypercalls] = {
[ __HYPERVISOR_memory_op ] = (hvm_hypercall_t *)hvm_memory_op,
[ __HYPERVISOR_grant_table_op ] = (hvm_hypercall_t *)hvm_grant_table_op,
-[ __HYPERVISOR_vcpu_op ] = (hvm_hypercall_t *)hvm_vcpu_op,
+HYPERCALL(vcpu_op),
[ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op,
HYPERCALL(xen_version),
HYPERCALL(console_io),
@@ -3583,7 +3531,7 @@ static hvm_hypercall_t *const
hvm_hypercall64_table[NR_hypercalls] = {
static hvm_hypercall_t *const hvm_hypercall32_table[NR_hypercalls] = {
[ __HYPERVISOR_memory_op ] = (hvm_hypercall_t *)hvm_memory_op_compat32,
[ __HYPERVISOR_grant_table_op ] = (hvm_hypercall_t
*)hvm_grant_table_op_compat32,
-[ __HYPERVISOR_vcpu_op ] = (hvm_hypercall_t *)hvm_vcpu_op_compat32,
+COMPAT_CALL(vcpu_op),
[ __HYPERVISOR_physdev_op ] = (hvm_hypercall_t *)hvm_physdev_op_compat32,
COMPAT_CALL(xen_version),
HYPERCALL(console_io),
And with an HVM guest poking at the rest of VCPUOPs: VCPUOP_get_physid,
VCPUOP_initialise, and VCPUOP_send_nmi - either before the CPU is up or
when it is up - work.
That is: the VCPUOP_get_physid would return -EINVAL; VCPUOP_initialise
would return either -EEXIST or 0, and in either case
the content of the ctxt was full of zeros.
The VCPUOP_send_nmi did cause the HVM to get an NMI and it spitted out
'Dazed and confused'. It also noticed corruption:
[3.611742] Corrupted low memory at c000fffc (fffc phys) = 00029b00
[2.386785] Corrupted low memory at 8800fff8 (fff8 phys) =
29900
Which is odd because there does not seem to be
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
>>> On 09.04.14 at 17:27, wrote: > On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote: >> >>> On 08.04.14 at 19:25, wrote: >> > --- a/xen/arch/x86/hvm/hvm.c >> > +++ b/xen/arch/x86/hvm/hvm.c >> > @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( >> > case VCPUOP_stop_singleshot_timer: >> > case VCPUOP_register_vcpu_info: >> > case VCPUOP_register_vcpu_time_memory_area: >> > +case VCPUOP_down: >> > +case VCPUOP_up: >> > +case VCPUOP_is_up: >> >> This, if I checked it properly, leaves only VCPUOP_initialise, >> VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. >> None of which look inherently bad to be used by HVM (but >> VCPUOP_initialise certainly would need closer checking), so I >> wonder whether either the wrapper shouldn't be dropped altogether >> or at least be converted from a white list approach to a black list one. > > I was being conservative here because I did not want to allow the > other ones without at least testing it. > > Perhaps that can be done as a seperate patch and this just as > a bug-fix? I'm clearly not in favor of the patch as is - minimally I'd want it to convert the white list to a black list. And once you do this it would seem rather natural to not pointlessly add entries. Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote: > >>> On 08.04.14 at 19:25, wrote: > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( > > case VCPUOP_stop_singleshot_timer: > > case VCPUOP_register_vcpu_info: > > case VCPUOP_register_vcpu_time_memory_area: > > +case VCPUOP_down: > > +case VCPUOP_up: > > +case VCPUOP_is_up: > > This, if I checked it properly, leaves only VCPUOP_initialise, > VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. > None of which look inherently bad to be used by HVM (but > VCPUOP_initialise certainly would need closer checking), so I > wonder whether either the wrapper shouldn't be dropped altogether > or at least be converted from a white list approach to a black list one. I was being conservative here because I did not want to allow the other ones without at least testing it. Perhaps that can be done as a seperate patch and this just as a bug-fix? > > Jan > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
>>> On 08.04.14 at 19:25, wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( > case VCPUOP_stop_singleshot_timer: > case VCPUOP_register_vcpu_info: > case VCPUOP_register_vcpu_time_memory_area: > +case VCPUOP_down: > +case VCPUOP_up: > +case VCPUOP_is_up: This, if I checked it properly, leaves only VCPUOP_initialise, VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. None of which look inherently bad to be used by HVM (but VCPUOP_initialise certainly would need closer checking), so I wonder whether either the wrapper shouldn't be dropped altogether or at least be converted from a white list approach to a black list one. Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On 08.04.14 at 19:25, kon...@kernel.org wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( case VCPUOP_stop_singleshot_timer: case VCPUOP_register_vcpu_info: case VCPUOP_register_vcpu_time_memory_area: +case VCPUOP_down: +case VCPUOP_up: +case VCPUOP_is_up: This, if I checked it properly, leaves only VCPUOP_initialise, VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. None of which look inherently bad to be used by HVM (but VCPUOP_initialise certainly would need closer checking), so I wonder whether either the wrapper shouldn't be dropped altogether or at least be converted from a white list approach to a black list one. Jan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote: On 08.04.14 at 19:25, kon...@kernel.org wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( case VCPUOP_stop_singleshot_timer: case VCPUOP_register_vcpu_info: case VCPUOP_register_vcpu_time_memory_area: +case VCPUOP_down: +case VCPUOP_up: +case VCPUOP_is_up: This, if I checked it properly, leaves only VCPUOP_initialise, VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. None of which look inherently bad to be used by HVM (but VCPUOP_initialise certainly would need closer checking), so I wonder whether either the wrapper shouldn't be dropped altogether or at least be converted from a white list approach to a black list one. I was being conservative here because I did not want to allow the other ones without at least testing it. Perhaps that can be done as a seperate patch and this just as a bug-fix? Jan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [XEN PATCH 1/2] hvm: Support more than 32 VCPUS when migrating.
On 09.04.14 at 17:27, konrad.w...@oracle.com wrote: On Wed, Apr 09, 2014 at 10:06:12AM +0100, Jan Beulich wrote: On 08.04.14 at 19:25, kon...@kernel.org wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3470,6 +3470,9 @@ static long hvm_vcpu_op( case VCPUOP_stop_singleshot_timer: case VCPUOP_register_vcpu_info: case VCPUOP_register_vcpu_time_memory_area: +case VCPUOP_down: +case VCPUOP_up: +case VCPUOP_is_up: This, if I checked it properly, leaves only VCPUOP_initialise, VCPUOP_send_nmi, and VCPUOP_get_physid disallowed for HVM. None of which look inherently bad to be used by HVM (but VCPUOP_initialise certainly would need closer checking), so I wonder whether either the wrapper shouldn't be dropped altogether or at least be converted from a white list approach to a black list one. I was being conservative here because I did not want to allow the other ones without at least testing it. Perhaps that can be done as a seperate patch and this just as a bug-fix? I'm clearly not in favor of the patch as is - minimally I'd want it to convert the white list to a black list. And once you do this it would seem rather natural to not pointlessly add entries. Jan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
