|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 24/28] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
El 21/08/15 a les 22.36, Andrew Cooper ha escrit:
> On 21/08/15 17:53, Roger Pau Monne wrote:
>> Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down and
>> VCPUOP_is_up hypercalls from HVM guests.
>>
>> This patch introduces a new structure (vcpu_hvm_context) that should be used
>> in conjuction with the VCPUOP_initialise hypercall in order to initialize
>> vCPUs for HVM guests.
>>
>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
>> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>> ---
>> Changes since v4:
>> - Don't assume mode is 64B, add an explicit check.
>> - Don't set TF_kernel_mode, it is only needed for PV guests.
>> - Don't set CR0_ET unconditionally.
>> ---
>> xen/arch/arm/domain.c | 24 ++++++
>> xen/arch/x86/domain.c | 164
>> +++++++++++++++++++++++++++++++++++++
>> xen/arch/x86/hvm/hvm.c | 8 ++
>> xen/common/domain.c | 16 +---
>> xen/include/public/hvm/hvm_vcpu.h | 168
>> ++++++++++++++++++++++++++++++++++++++
>> xen/include/xen/domain.h | 2 +
>> 6 files changed, 367 insertions(+), 15 deletions(-)
>> create mode 100644 xen/include/public/hvm/hvm_vcpu.h
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index b2bfc7d..b20035d 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -752,6 +752,30 @@ int arch_set_info_guest(
>> return 0;
>> }
>>
>> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> + struct vcpu_guest_context *ctxt;
>> + struct domain *d = current->domain;
>> + int rc;
>> +
>> + if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
>> + return -ENOMEM;
>
> I have posted my "remove alloc_vcpu_guest_context()" patch to the list
> for reference as it interacts with this patch. I don't mind rebasing
> it, but it might also influence this patch.
Thanks, I was planning to add such a patch to the series because of your
comments in the previous round, but completely forgot about it, sorry.
I don't mind picking it up and adding it to my series if now it's too
late in the release process to commit it.
>> +
>> + if ( copy_from_guest(ctxt, arg, 1) )
>> + {
>> + free_vcpu_guest_context(ctxt);
>> + return -EFAULT;
>> + }
>> +
>> + domain_lock(d);
>> + rc = v->is_initialised ? -EEXIST : arch_set_info_guest(v, ctxt);
>> + domain_unlock(d);
>> +
>> + free_vcpu_guest_context(ctxt);
>> +
>> + return rc;
>> +}
>> +
>> int arch_vcpu_reset(struct vcpu *v)
>> {
>> vcpu_end_shutdown_deferral(v);
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 8fe95f7..23ff14c 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -37,6 +37,7 @@
>> #include <xen/wait.h>
>> #include <xen/guest_access.h>
>> #include <public/sysctl.h>
>> +#include <public/hvm/hvm_vcpu.h>
>> #include <asm/regs.h>
>> #include <asm/mc146818rtc.h>
>> #include <asm/system.h>
>> @@ -1140,6 +1141,169 @@ int arch_set_info_guest(
>> #undef c
>> }
>>
>> +/* Called by VCPUOP_initialise for HVM guests. */
>> +static int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx)
>> +{
>> + struct segment_register seg;
>> +
>> +#define get_context_seg(ctx, seg, f)
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.seg##_##f :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.seg##_##f :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_64B ? (ctx)->cpu_regs.x86_64.seg##_##f :
>> \
>> + ({ panic("Invalid vCPU mode %u requested\n", (ctx)->mode); 0; })
>
> panic() is far too severe. domain_crash() would be better. with an
> early exit.
>
>> +
>> +#define get_context_gpr(ctx, gpr)
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.gpr :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.e##gpr :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_64B ? (ctx)->cpu_regs.x86_64.r##gpr :
>> \
>> + ({ panic("Invalid vCPU mode %u requested\n", (ctx)->mode); 0; })
>> +
>> +#define get_context_field(ctx, field)
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.field :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.field :
>> \
>> + (ctx)->mode == VCPU_HVM_MODE_64B ? (ctx)->cpu_regs.x86_64.field :
>> \
>> + ({ panic("Invalid vCPU mode %u requested\n", (ctx)->mode); 0; })
>> +
>> + if ( ctx->mode != VCPU_HVM_MODE_16B && ctx->mode != VCPU_HVM_MODE_32B &&
>> + ctx->mode != VCPU_HVM_MODE_64B )
>> + return -EINVAL;
>
> For readability (and style), I would suggest formatting this as
>
> if ( !((ctx->mode == VCPU_HVM_MODE_16B) ||
> (ctx->mode == VCPU_HVM_MODE_32B) ||
> (ctx->mode == VCPU_HVM_MODE_64B)) )
> return -EINVAL;
>
>> +
>> + memset(&seg, 0, sizeof(seg));
>> +
>> + if ( !paging_mode_hap(v->domain) )
>> + v->arch.guest_table = pagetable_null();
>> +
>> + v->arch.user_regs.rax = get_context_gpr(ctx, ax);
>> + v->arch.user_regs.rcx = get_context_gpr(ctx, cx);
>> + v->arch.user_regs.rdx = get_context_gpr(ctx, dx);
>> + v->arch.user_regs.rbx = get_context_gpr(ctx, bx);
>> + v->arch.user_regs.rsp = get_context_gpr(ctx, sp);
>> + v->arch.user_regs.rbp = get_context_gpr(ctx, bp);
>> + v->arch.user_regs.rsi = get_context_gpr(ctx, si);
>> + v->arch.user_regs.rdi = get_context_gpr(ctx, di);
>> + v->arch.user_regs.rip = get_context_gpr(ctx, ip);
>> + v->arch.user_regs.rflags = get_context_gpr(ctx, flags);
>
> All these hidden conditionals cause the compiler to generate a 2K
> function, a large quantity of which are conditional jumps.
I was expecting the compiler to be clever here and realize ctx->mode is
always the same and perform some kind of clever optimization, but I
guess this is too much.
> I did some experimentation, available from
> git://xenbits.xen.org/people/andrewcoop/xen.git wip-dmlite-v5-refactor
>
> Bloat-o-meter indicates:
>
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-559 (-559)
> function old new delta
> arch_set_info_hvm_guest 2209 1650 -559
>
> And looking at the disassembly, those -559 are mostly cmp/jXX
> constructs, and the dead panic() calls.
>
> The code is now longer, but I don't think it detracts from the
> readability, and it will certainly be faster to execute.
>
> What do you think? If others agree, you are welcome to fold the patch
> into your series.
That looks fine IMHO, I can fold it into this patch and add your SoB if
that's fine.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |