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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.18 at 16:32, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -349,20 +349,33 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
>      return ret;
>  }
>  
> -static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> +void vmce_save_vcpu_ctxt_one(struct vcpu *v, struct hvm_vmce_vcpu *ctxt)
> +{
> +    ctxt->caps = v->arch.vmce.mcg_cap;
> +    ctxt->mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
> +    ctxt->mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
> +    ctxt->mcg_ext_ctl = v->arch.vmce.mcg_ext_ctl;
> +}
> +
> +static int vmce_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h, 
> unsigned int instance)
>  {
>      struct vcpu *v;
>      int err = 0;
>  
> -    for_each_vcpu ( d, v )
> +    if( instance < d->max_vcpus )
> +    {
> +        struct hvm_vmce_vcpu ctxt;
> +
> +        v = d->vcpu[instance];
> +        vmce_save_vcpu_ctxt_one(v, &ctxt);

Please add a NULL check for v between these two lines, like we do everywhere
else when accessing d->vcpu[].

> +        err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
> +    }
> +    else for_each_vcpu ( d, v )
>      {
> -        struct hvm_vmce_vcpu ctxt = {
> -            .caps = v->arch.vmce.mcg_cap,
> -            .mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2,
> -            .mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2,
> -            .mcg_ext_ctl = v->arch.vmce.mcg_ext_ctl,
> -        };
> +        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;

Note the recurring pattern throughout the patch: 

    if ( instance < d->max_vcpus )
    {
        ...
        xyz_save_one();
    }
    else for_each_vcpu ( d, v )
    {
        ...
        xyz_save_one();
    }
    
I think you want to move this into the caller, such that each specific handler
only deals with a single instance. I also think you want to split the patch,
introducing the save_one() functions one at a time (used by their save()
counterparts), and only then dropping the save() ones in favor of doing the
loop in the caller. The final patch would then add the single instance case
you're after.

Also please pay attention to coding style.

Jan



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