Re: [Xen-devel] [PATCH v4 2/2] x86/domctl: Don't pause the whole domain if only getting vcpu state

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.05.18 at 18:04, <JBeulich@xxxxxxxx> wrote:
>>>> On 07.05.18 at 10:24, <aisaila@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> @@ -357,20 +357,14 @@ void vmce_save_vcpu_ctxt_one(struct vcpu *v, struct 
>> hvm_vmce_vcpu *ctxt)
>>      ctxt->mcg_ext_ctl = v->arch.vmce.mcg_ext_ctl;
>>  }
>>  
>> -static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>> +static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h,
>> +                               struct vcpu *v)
>>  {
>> -    struct vcpu *v;
>>      int err = 0;
>> +    struct hvm_vmce_vcpu ctxt;
>>  
>> -    for_each_vcpu ( d, v )
>> -    {
>> -        struct hvm_vmce_vcpu ctxt;
>> -
>> -        vmce_save_vcpu_ctxt_one(v, &ctxt);
>> -        err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
>> -        if ( err )
>> -            break;
>> -    }
>> +    vmce_save_vcpu_ctxt_one(v, &ctxt);
>> +    err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
>>  
>>      return err;
>>  }
> 
> At the example of this one: The idea of breaking out the patch introducing
> the _one() functions was to avoid restructuring in this patch like what you
> do here. Any such change not strictly fitting under the title of this patch
> should be broken out. There may be multiple steps involved here.
> 
> As it stands, the function is now no longer meaningfully different from
> vmce_save_vcpu_ctxt_one(), and this pattern recurs. Such redundancy
> is undesirable. Together with you now passing v and d (when just v
> would suffice) I think you want to further re-structure how handling of
> save/restore happens, such that no stub functions like the one here
> remain. IOW after having introduced the _one() functions, a second
> transformation would be expected to eliminate the original ones, with
> (as you do here) the loop moving into the caller.

I think I should further clarify this reply of mine: The goal of the
transformation should be that in the end we continue to have a single
load and a single save function for every save type. These should be
referenced by HVM_REGISTER_SAVE_RESTORE() or whatever clone of
it may turn out necessary (note how its uses currently specify
HVMSR_PER_VCPU, which may in the end no longer be necessary).
That will also make the asymmetry go away that the save functions
currently iterate over all vCPU-s, while the load functions don't (it'll
presumably remain to be that way for multi-instance types where it's
not the vCPU that is determining the instance, like the PIC).

Ideally save functions would further match load ones by the caller
specifying the instance. Suitable unique return values may need to be
used to signal the caller when to end the iteration. For example, the
functions could return a "next instance" indicator. (I think you agree
that this is an unavoidable difference to the load functions, where the
instance comes with the load record.)

In any event, just to re-iterate - the final patch under this title should
preferably not have a need to touch any of the save functions; all of
this should be done in prereq changes. That'll then allow to focus on
just the specific bit of new behavior you want when reviewing this
(presumably last) patch of the series.

Jan



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