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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/08/15 11:43, Roger Pau Monné wrote:
> 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.

The patch in definitely 4.7 material at this point.  I will respin it
with an improved commit message, per konrads implied request.

>>> +
>>> +    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.

C may not assume that ctx->$FOO it doesn't alias v->$BAR, and therefore
that ctx->mode doesn't change as a result of writing into v.

I tried experimenting with a "const uint32_t mode = ctx->mode" but even
that wasn't sufficient for the compiler to optimise the branches away.

Perhaps some C11 "restrict" keywords might have helped.  I didn't
investigate.

>
>> 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.

Completely fine.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel