Re: [PATCH v17 03/13] x86/hvm: Introduce hvm_save_cpu_ctxt_one func

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.08.18 at 17:33, <rcojocaru@bitdefender.com> wrote:
> On 8/22/18 5:02 PM, Alexandru Isaila wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -787,119 +787,126 @@ static int hvm_load_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
>>  HVM_REGISTER_SAVE_RESTORE(TSC_ADJUST, hvm_save_tsc_adjust,
>>                            hvm_load_tsc_adjust, 1, HVMSR_PER_VCPU);
>>  
>> -static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>> +static int hvm_save_cpu_ctxt_one(struct vcpu *v, hvm_domain_context_t *h)
>>  {
>> -    struct vcpu *v;
>> -    struct hvm_hw_cpu ctxt;
>>      struct segment_register seg;
>> +    struct hvm_hw_cpu ctxt = {
>> +        .tsc = hvm_get_guest_tsc_fixed(v, v->domain->arch.hvm_domain.sync_tsc),
>> +        .msr_tsc_aux = hvm_msr_tsc_aux(v),
>> +        .rax = v->arch.user_regs.rax,
>> +        .rbx = v->arch.user_regs.rbx,
>> +        .rcx = v->arch.user_regs.rcx,
>> +        .rdx = v->arch.user_regs.rdx,
>> +        .rbp = v->arch.user_regs.rbp,
>> +        .rsi = v->arch.user_regs.rsi,
>> +        .rdi = v->arch.user_regs.rdi,
>> +        .rsp = v->arch.user_regs.rsp,
>> +        .rip = v->arch.user_regs.rip,
>> +        .rflags = v->arch.user_regs.rflags,
>> +        .r8  = v->arch.user_regs.r8,
>> +        .r9  = v->arch.user_regs.r9,
>> +        .r10 = v->arch.user_regs.r10,
>> +        .r11 = v->arch.user_regs.r11,
>> +        .r12 = v->arch.user_regs.r12,
>> +        .r13 = v->arch.user_regs.r13,
>> +        .r14 = v->arch.user_regs.r14,
>> +        .r15 = v->arch.user_regs.r15,
>> +        .dr0 = v->arch.debugreg[0],
>> +        .dr1 = v->arch.debugreg[1],
>> +        .dr2 = v->arch.debugreg[2],
>> +        .dr3 = v->arch.debugreg[3],
>> +        .dr6 = v->arch.debugreg[6],
>> +        .dr7 = v->arch.debugreg[7],
>> +    };
>>  
>> -    for_each_vcpu ( d, v )
>> +    /*
>> +     * We don't need to save state for a vcpu that is down; the restore
>> +     * code will leave it down if there is nothing saved.
>> +     */
>> +    if ( v->pause_flags & VPF_down )
>> +        return 0;
> 
> Actually, we'd like to remove this if() if possible - even if the VCPU
> is down, we should still be able to query it and receive whatever state
> it is in, according to the Intel SDM, "9.1.1 Processor State After
> Reset". Any objections to this?

Certainly not in this series; it's therefore not really ideal to
discuss this on this thread instead of a separate one.

I'm also not convinced the state you'd receive would
actually be "after reset" state. We may need to synthesize
this if we really wanted to allow this.

> Also, reading the comment and looking at the code, it appears that the
> end of hvm_load_cpu_ctxt() actually wakes the VCPU up:
> 
> 1152     v->arch.debugreg[7] = ctxt.dr7;
> 1153
> 1154     v->arch.vgc_flags = VGCF_online;
> 1155
> 1156     /* Auxiliary processors should be woken immediately. */
> 1157     v->is_initialised = 1;
> 1158     clear_bit(_VPF_down, &v->pause_flags);
> 1159     vcpu_wake(v);
> 1160
> 1161     return 0;
> 1162 }
> 
> which appears to be non-architectural behaviour if we've interpreted
> correctly section 8.4 of Volume 3 of the SDM, as basically saying that
> VCPUs should be awaken by IPIs (though this is outside the scope of this
> patch).

I'm not sure hardware behavior can be used as a reference here:
Hardware doesn't allow loading of state the way we do here. Also
I don't see how/when else you would propose the unpausing would
happen here, in particular in the course of a (live) migration.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel