Re: [PATCH v5 24/28] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

["Roger Pau Monné" <roger.pau@citrix.com>]

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@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>> 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@lists.xen.org
http://lists.xen.org/xen-devel