* [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode
@ 2014-01-21 15:33 Jan Beulich
2014-01-21 16:55 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2014-01-21 15:33 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 5535 bytes --]
Since 64-bit PV uses separate kernel and user mode page tables, kernel
addresses (as usually provided via VCPUOP_register_runstate_memory_area
and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
necessarily accessible when the respective updating occurs. Add logic
for toggle_guest_mode() to take care of this (if necessary) the next
time the vCPU switches to kernel mode.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
}
/* Update per-VCPU guest runstate shared memory area (if registered). */
-static void update_runstate_area(struct vcpu *v)
+bool_t update_runstate_area(const struct vcpu *v)
{
if ( guest_handle_is_null(runstate_guest(v)) )
- return;
+ return 1;
if ( has_32bit_shinfo(v->domain) )
{
@@ -1334,10 +1334,18 @@ static void update_runstate_area(struct
XLAT_vcpu_runstate_info(&info, &v->runstate);
__copy_to_guest(v->runstate_guest.compat, &info, 1);
- return;
+ return 1;
}
- __copy_to_guest(runstate_guest(v), &v->runstate, 1);
+ return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
+ sizeof(v->runstate);
+}
+
+static void _update_runstate_area(struct vcpu *v)
+{
+ if ( !update_runstate_area(v) && is_pv_vcpu(v) &&
+ !(v->arch.flags & TF_kernel_mode) )
+ v->arch.pv_vcpu.need_update_runstate_area = 1;
}
static inline int need_full_gdt(struct vcpu *v)
@@ -1443,8 +1451,8 @@ void context_switch(struct vcpu *prev, s
flush_tlb_mask(&dirty_mask);
}
- if (prev != next)
- update_runstate_area(prev);
+ if ( prev != next )
+ _update_runstate_area(prev);
if ( is_hvm_vcpu(prev) )
{
@@ -1497,8 +1505,8 @@ void context_switch(struct vcpu *prev, s
context_saved(prev);
- if (prev != next)
- update_runstate_area(next);
+ if ( prev != next )
+ _update_runstate_area(next);
/* Ensure that the vcpu has an up-to-date time base. */
update_vcpu_system_time(next);
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -740,7 +740,6 @@ static void __update_vcpu_system_time(st
{
struct cpu_time *t;
struct vcpu_time_info *u, _u;
- XEN_GUEST_HANDLE(vcpu_time_info_t) user_u;
struct domain *d = v->domain;
s_time_t tsc_stamp = 0;
@@ -805,19 +804,31 @@ static void __update_vcpu_system_time(st
/* 3. Update guest kernel version. */
u->version = version_update_end(u->version);
- user_u = v->arch.time_info_guest;
- if ( !guest_handle_is_null(user_u) )
- {
- /* 1. Update userspace version. */
- __copy_field_to_guest(user_u, &_u, version);
- wmb();
- /* 2. Update all other userspavce fields. */
- __copy_to_guest(user_u, &_u, 1);
- wmb();
- /* 3. Update userspace version. */
- _u.version = version_update_end(_u.version);
- __copy_field_to_guest(user_u, &_u, version);
- }
+ if ( !update_secondary_system_time(v, &_u) && is_pv_domain(d) &&
+ !is_pv_32bit_domain(d) && !(v->arch.flags & TF_kernel_mode) )
+ v->arch.pv_vcpu.pending_system_time = _u;
+}
+
+bool_t update_secondary_system_time(const struct vcpu *v,
+ struct vcpu_time_info *u)
+{
+ XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
+
+ if ( guest_handle_is_null(user_u) )
+ return 1;
+
+ /* 1. Update userspace version. */
+ if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
+ return 0;
+ wmb();
+ /* 2. Update all other userspace fields. */
+ __copy_to_guest(user_u, u, 1);
+ wmb();
+ /* 3. Update userspace version. */
+ u->version = version_update_end(u->version);
+ __copy_field_to_guest(user_u, u, version);
+
+ return 1;
}
void update_vcpu_system_time(struct vcpu *v)
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
#else
write_ptbase(v);
#endif
+
+ if ( !(v->arch.flags & TF_kernel_mode) )
+ return;
+
+ if ( v->arch.pv_vcpu.need_update_runstate_area &&
+ update_runstate_area(v) )
+ v->arch.pv_vcpu.need_update_runstate_area = 0;
+
+ if ( v->arch.pv_vcpu.pending_system_time.version &&
+ update_secondary_system_time(v,
+ &v->arch.pv_vcpu.pending_system_time) )
+ v->arch.pv_vcpu.pending_system_time.version = 0;
}
unsigned long do_iret(void)
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -373,6 +373,10 @@ struct pv_vcpu
/* Current LDT details. */
unsigned long shadow_ldt_mapcnt;
spinlock_t shadow_ldt_lock;
+
+ /* Deferred VA-based update state. */
+ bool_t need_update_runstate_area;
+ struct vcpu_time_info pending_system_time;
};
struct arch_vcpu
@@ -445,6 +449,10 @@ struct arch_vcpu
#define hvm_vmx hvm_vcpu.u.vmx
#define hvm_svm hvm_vcpu.u.svm
+bool_t update_runstate_area(const struct vcpu *);
+bool_t update_secondary_system_time(const struct vcpu *,
+ struct vcpu_time_info *);
+
void vcpu_show_execution_state(struct vcpu *);
void vcpu_show_registers(const struct vcpu *);
[-- Attachment #2: x86-deferred-va-updates.patch --]
[-- Type: text/plain, Size: 5615 bytes --]
x86: don't drop guest visible state updates when 64-bit PV guest is in user mode
Since 64-bit PV uses separate kernel and user mode page tables, kernel
addresses (as usually provided via VCPUOP_register_runstate_memory_area
and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
necessarily accessible when the respective updating occurs. Add logic
for toggle_guest_mode() to take care of this (if necessary) the next
time the vCPU switches to kernel mode.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
}
/* Update per-VCPU guest runstate shared memory area (if registered). */
-static void update_runstate_area(struct vcpu *v)
+bool_t update_runstate_area(const struct vcpu *v)
{
if ( guest_handle_is_null(runstate_guest(v)) )
- return;
+ return 1;
if ( has_32bit_shinfo(v->domain) )
{
@@ -1334,10 +1334,18 @@ static void update_runstate_area(struct
XLAT_vcpu_runstate_info(&info, &v->runstate);
__copy_to_guest(v->runstate_guest.compat, &info, 1);
- return;
+ return 1;
}
- __copy_to_guest(runstate_guest(v), &v->runstate, 1);
+ return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
+ sizeof(v->runstate);
+}
+
+static void _update_runstate_area(struct vcpu *v)
+{
+ if ( !update_runstate_area(v) && is_pv_vcpu(v) &&
+ !(v->arch.flags & TF_kernel_mode) )
+ v->arch.pv_vcpu.need_update_runstate_area = 1;
}
static inline int need_full_gdt(struct vcpu *v)
@@ -1443,8 +1451,8 @@ void context_switch(struct vcpu *prev, s
flush_tlb_mask(&dirty_mask);
}
- if (prev != next)
- update_runstate_area(prev);
+ if ( prev != next )
+ _update_runstate_area(prev);
if ( is_hvm_vcpu(prev) )
{
@@ -1497,8 +1505,8 @@ void context_switch(struct vcpu *prev, s
context_saved(prev);
- if (prev != next)
- update_runstate_area(next);
+ if ( prev != next )
+ _update_runstate_area(next);
/* Ensure that the vcpu has an up-to-date time base. */
update_vcpu_system_time(next);
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -740,7 +740,6 @@ static void __update_vcpu_system_time(st
{
struct cpu_time *t;
struct vcpu_time_info *u, _u;
- XEN_GUEST_HANDLE(vcpu_time_info_t) user_u;
struct domain *d = v->domain;
s_time_t tsc_stamp = 0;
@@ -805,19 +804,31 @@ static void __update_vcpu_system_time(st
/* 3. Update guest kernel version. */
u->version = version_update_end(u->version);
- user_u = v->arch.time_info_guest;
- if ( !guest_handle_is_null(user_u) )
- {
- /* 1. Update userspace version. */
- __copy_field_to_guest(user_u, &_u, version);
- wmb();
- /* 2. Update all other userspavce fields. */
- __copy_to_guest(user_u, &_u, 1);
- wmb();
- /* 3. Update userspace version. */
- _u.version = version_update_end(_u.version);
- __copy_field_to_guest(user_u, &_u, version);
- }
+ if ( !update_secondary_system_time(v, &_u) && is_pv_domain(d) &&
+ !is_pv_32bit_domain(d) && !(v->arch.flags & TF_kernel_mode) )
+ v->arch.pv_vcpu.pending_system_time = _u;
+}
+
+bool_t update_secondary_system_time(const struct vcpu *v,
+ struct vcpu_time_info *u)
+{
+ XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
+
+ if ( guest_handle_is_null(user_u) )
+ return 1;
+
+ /* 1. Update userspace version. */
+ if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
+ return 0;
+ wmb();
+ /* 2. Update all other userspace fields. */
+ __copy_to_guest(user_u, u, 1);
+ wmb();
+ /* 3. Update userspace version. */
+ u->version = version_update_end(u->version);
+ __copy_field_to_guest(user_u, u, version);
+
+ return 1;
}
void update_vcpu_system_time(struct vcpu *v)
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
#else
write_ptbase(v);
#endif
+
+ if ( !(v->arch.flags & TF_kernel_mode) )
+ return;
+
+ if ( v->arch.pv_vcpu.need_update_runstate_area &&
+ update_runstate_area(v) )
+ v->arch.pv_vcpu.need_update_runstate_area = 0;
+
+ if ( v->arch.pv_vcpu.pending_system_time.version &&
+ update_secondary_system_time(v,
+ &v->arch.pv_vcpu.pending_system_time) )
+ v->arch.pv_vcpu.pending_system_time.version = 0;
}
unsigned long do_iret(void)
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -373,6 +373,10 @@ struct pv_vcpu
/* Current LDT details. */
unsigned long shadow_ldt_mapcnt;
spinlock_t shadow_ldt_lock;
+
+ /* Deferred VA-based update state. */
+ bool_t need_update_runstate_area;
+ struct vcpu_time_info pending_system_time;
};
struct arch_vcpu
@@ -445,6 +449,10 @@ struct arch_vcpu
#define hvm_vmx hvm_vcpu.u.vmx
#define hvm_svm hvm_vcpu.u.svm
+bool_t update_runstate_area(const struct vcpu *);
+bool_t update_secondary_system_time(const struct vcpu *,
+ struct vcpu_time_info *);
+
void vcpu_show_execution_state(struct vcpu *);
void vcpu_show_registers(const struct vcpu *);
[-- 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] 4+ messages in thread* Re: [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode
2014-01-21 15:33 [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode Jan Beulich
@ 2014-01-21 16:55 ` Andrew Cooper
2014-01-21 17:01 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-01-21 16:55 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 6538 bytes --]
On 21/01/2014 15:33, Jan Beulich wrote:
> Since 64-bit PV uses separate kernel and user mode page tables, kernel
> addresses (as usually provided via VCPUOP_register_runstate_memory_area
> and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
> necessarily accessible when the respective updating occurs. Add logic
> for toggle_guest_mode() to take care of this (if necessary) the next
> time the vCPU switches to kernel mode.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
> }
>
> /* Update per-VCPU guest runstate shared memory area (if registered). */
> -static void update_runstate_area(struct vcpu *v)
> +bool_t update_runstate_area(const struct vcpu *v)
Can you adjust the comment to indicate what the return value means. The
logic is quite opaque, but I believe it is "true if the runstate has
been suitably updated".
> {
> if ( guest_handle_is_null(runstate_guest(v)) )
> - return;
> + return 1;
>
> if ( has_32bit_shinfo(v->domain) )
> {
> @@ -1334,10 +1334,18 @@ static void update_runstate_area(struct
>
> XLAT_vcpu_runstate_info(&info, &v->runstate);
> __copy_to_guest(v->runstate_guest.compat, &info, 1);
> - return;
> + return 1;
> }
>
> - __copy_to_guest(runstate_guest(v), &v->runstate, 1);
> + return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> + sizeof(v->runstate);
> +}
> +
> +static void _update_runstate_area(struct vcpu *v)
> +{
> + if ( !update_runstate_area(v) && is_pv_vcpu(v) &&
> + !(v->arch.flags & TF_kernel_mode) )
> + v->arch.pv_vcpu.need_update_runstate_area = 1;
> }
>
> static inline int need_full_gdt(struct vcpu *v)
> @@ -1443,8 +1451,8 @@ void context_switch(struct vcpu *prev, s
> flush_tlb_mask(&dirty_mask);
> }
>
> - if (prev != next)
> - update_runstate_area(prev);
> + if ( prev != next )
> + _update_runstate_area(prev);
>
> if ( is_hvm_vcpu(prev) )
> {
> @@ -1497,8 +1505,8 @@ void context_switch(struct vcpu *prev, s
>
> context_saved(prev);
>
> - if (prev != next)
> - update_runstate_area(next);
> + if ( prev != next )
> + _update_runstate_area(next);
>
> /* Ensure that the vcpu has an up-to-date time base. */
> update_vcpu_system_time(next);
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -740,7 +740,6 @@ static void __update_vcpu_system_time(st
> {
> struct cpu_time *t;
> struct vcpu_time_info *u, _u;
> - XEN_GUEST_HANDLE(vcpu_time_info_t) user_u;
> struct domain *d = v->domain;
> s_time_t tsc_stamp = 0;
>
> @@ -805,19 +804,31 @@ static void __update_vcpu_system_time(st
> /* 3. Update guest kernel version. */
> u->version = version_update_end(u->version);
>
> - user_u = v->arch.time_info_guest;
> - if ( !guest_handle_is_null(user_u) )
> - {
> - /* 1. Update userspace version. */
> - __copy_field_to_guest(user_u, &_u, version);
> - wmb();
> - /* 2. Update all other userspavce fields. */
> - __copy_to_guest(user_u, &_u, 1);
> - wmb();
> - /* 3. Update userspace version. */
> - _u.version = version_update_end(_u.version);
> - __copy_field_to_guest(user_u, &_u, version);
> - }
> + if ( !update_secondary_system_time(v, &_u) && is_pv_domain(d) &&
> + !is_pv_32bit_domain(d) && !(v->arch.flags & TF_kernel_mode) )
> + v->arch.pv_vcpu.pending_system_time = _u;
> +}
> +
> +bool_t update_secondary_system_time(const struct vcpu *v,
> + struct vcpu_time_info *u)
> +{
> + XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
> +
> + if ( guest_handle_is_null(user_u) )
> + return 1;
> +
> + /* 1. Update userspace version. */
> + if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> + return 0;
> + wmb();
> + /* 2. Update all other userspace fields. */
> + __copy_to_guest(user_u, u, 1);
> + wmb();
> + /* 3. Update userspace version. */
> + u->version = version_update_end(u->version);
> + __copy_field_to_guest(user_u, u, version);
> +
> + return 1;
> }
>
> void update_vcpu_system_time(struct vcpu *v)
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
> #else
> write_ptbase(v);
> #endif
> +
> + if ( !(v->arch.flags & TF_kernel_mode) )
> + return;
> +
> + if ( v->arch.pv_vcpu.need_update_runstate_area &&
> + update_runstate_area(v) )
> + v->arch.pv_vcpu.need_update_runstate_area = 0;
> +
> + if ( v->arch.pv_vcpu.pending_system_time.version &&
> + update_secondary_system_time(v,
> + &v->arch.pv_vcpu.pending_system_time) )
> + v->arch.pv_vcpu.pending_system_time.version = 0;
What would happen now if a guest kernel loads a faulting address for its
runstate info (or edits its pagetables so a previously good address now
faults)? It appears as if we could retry writing to the same bad
address each time we try to toggle into kernel mode.
Given that we know we are running on the correct set of pagetables, I
think both of these pending variable can be unconditionally cleared,
whether or not update{runstate_area,secondary_system_time} succeed or fail.
~Andrew
> }
>
> unsigned long do_iret(void)
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -373,6 +373,10 @@ struct pv_vcpu
> /* Current LDT details. */
> unsigned long shadow_ldt_mapcnt;
> spinlock_t shadow_ldt_lock;
> +
> + /* Deferred VA-based update state. */
> + bool_t need_update_runstate_area;
> + struct vcpu_time_info pending_system_time;
> };
>
> struct arch_vcpu
> @@ -445,6 +449,10 @@ struct arch_vcpu
> #define hvm_vmx hvm_vcpu.u.vmx
> #define hvm_svm hvm_vcpu.u.svm
>
> +bool_t update_runstate_area(const struct vcpu *);
> +bool_t update_secondary_system_time(const struct vcpu *,
> + struct vcpu_time_info *);
> +
> void vcpu_show_execution_state(struct vcpu *);
> void vcpu_show_registers(const struct vcpu *);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 7464 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] 4+ messages in thread* Re: [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode
2014-01-21 16:55 ` Andrew Cooper
@ 2014-01-21 17:01 ` Jan Beulich
2014-01-21 17:06 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2014-01-21 17:01 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 21.01.14 at 17:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 21/01/2014 15:33, Jan Beulich wrote:
>> Since 64-bit PV uses separate kernel and user mode page tables, kernel
>> addresses (as usually provided via VCPUOP_register_runstate_memory_area
>> and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
>> necessarily accessible when the respective updating occurs. Add logic
>> for toggle_guest_mode() to take care of this (if necessary) the next
>> time the vCPU switches to kernel mode.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
>> }
>>
>> /* Update per-VCPU guest runstate shared memory area (if registered). */
>> -static void update_runstate_area(struct vcpu *v)
>> +bool_t update_runstate_area(const struct vcpu *v)
>
> Can you adjust the comment to indicate what the return value means. The
> logic is quite opaque, but I believe it is "true if the runstate has
> been suitably updated".
Sigh - yes, I could of course. But it looks quite obvious to me.
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
>> #else
>> write_ptbase(v);
>> #endif
>> +
>> + if ( !(v->arch.flags & TF_kernel_mode) )
>> + return;
>> +
>> + if ( v->arch.pv_vcpu.need_update_runstate_area &&
>> + update_runstate_area(v) )
>> + v->arch.pv_vcpu.need_update_runstate_area = 0;
>> +
>> + if ( v->arch.pv_vcpu.pending_system_time.version &&
>> + update_secondary_system_time(v,
>> + &v->arch.pv_vcpu.pending_system_time) )
>> + v->arch.pv_vcpu.pending_system_time.version = 0;
>
> What would happen now if a guest kernel loads a faulting address for its
> runstate info (or edits its pagetables so a previously good address now
> faults)? It appears as if we could retry writing to the same bad
> address each time we try to toggle into kernel mode.
>
> Given that we know we are running on the correct set of pagetables, I
> think both of these pending variable can be unconditionally cleared,
> whether or not update{runstate_area,secondary_system_time} succeed or fail.
We could, but do you really think there's much harm in us keeping
this logically consistent? The only entity impacted is going to be
the "offending" domain, as we're wasting more of its time doing the
mode switch for it.
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode
2014-01-21 17:01 ` Jan Beulich
@ 2014-01-21 17:06 ` Andrew Cooper
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2014-01-21 17:06 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 21/01/2014 17:01, Jan Beulich wrote:
>>>> On 21.01.14 at 17:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 21/01/2014 15:33, Jan Beulich wrote:
>>> Since 64-bit PV uses separate kernel and user mode page tables, kernel
>>> addresses (as usually provided via VCPUOP_register_runstate_memory_area
>>> and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
>>> necessarily accessible when the respective updating occurs. Add logic
>>> for toggle_guest_mode() to take care of this (if necessary) the next
>>> time the vCPU switches to kernel mode.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
>>> }
>>>
>>> /* Update per-VCPU guest runstate shared memory area (if registered). */
>>> -static void update_runstate_area(struct vcpu *v)
>>> +bool_t update_runstate_area(const struct vcpu *v)
>> Can you adjust the comment to indicate what the return value means. The
>> logic is quite opaque, but I believe it is "true if the runstate has
>> been suitably updated".
> Sigh - yes, I could of course. But it looks quite obvious to me.
>
>>> --- a/xen/arch/x86/x86_64/traps.c
>>> +++ b/xen/arch/x86/x86_64/traps.c
>>> @@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
>>> #else
>>> write_ptbase(v);
>>> #endif
>>> +
>>> + if ( !(v->arch.flags & TF_kernel_mode) )
>>> + return;
>>> +
>>> + if ( v->arch.pv_vcpu.need_update_runstate_area &&
>>> + update_runstate_area(v) )
>>> + v->arch.pv_vcpu.need_update_runstate_area = 0;
>>> +
>>> + if ( v->arch.pv_vcpu.pending_system_time.version &&
>>> + update_secondary_system_time(v,
>>> + &v->arch.pv_vcpu.pending_system_time) )
>>> + v->arch.pv_vcpu.pending_system_time.version = 0;
>> What would happen now if a guest kernel loads a faulting address for its
>> runstate info (or edits its pagetables so a previously good address now
>> faults)? It appears as if we could retry writing to the same bad
>> address each time we try to toggle into kernel mode.
>>
>> Given that we know we are running on the correct set of pagetables, I
>> think both of these pending variable can be unconditionally cleared,
>> whether or not update{runstate_area,secondary_system_time} succeed or fail.
> We could, but do you really think there's much harm in us keeping
> this logically consistent? The only entity impacted is going to be
> the "offending" domain, as we're wasting more of its time doing the
> mode switch for it.
>
> Jan
>
I suppose not - an extra pagefault or two for a toggle into kernel mode
is hardly the biggest of overheads.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-21 17:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-21 15:33 [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode Jan Beulich
2014-01-21 16:55 ` Andrew Cooper
2014-01-21 17:01 ` Jan Beulich
2014-01-21 17:06 ` Andrew Cooper
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.