Re: [Xen-devel] [PATCH v7 08/15] x86/cpu: Remove loop form vmce_save_vcpu_ctxt() func

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.06.18 at 14:46, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Alexandru Stefan ISAILA [mailto:aisaila@xxxxxxxxxxxxxxx]
>> Sent: 08 June 2018 09:51
>> On Vi, 2018-06-08 at 08:33 +0000, Paul Durrant wrote:
>> > > From: Alexandru Isaila [mailto:aisaila@xxxxxxxxxxxxxxx]
>> > > Sent: 07 June 2018 15:59
>> > > Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>> > > ---
>> > >  xen/arch/x86/cpu/mcheck/vmce.c | 27 +++++++--------------------
>> > >  1 file changed, 7 insertions(+), 20 deletions(-)
>> > >
>> > > diff --git a/xen/arch/x86/cpu/mcheck/vmce.c
>> > > b/xen/arch/x86/cpu/mcheck/vmce.c
>> > > index 404f27e..ead1f73 100644
>> > > --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> > > +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> > > @@ -349,30 +349,17 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
>> > >      return ret;
>> > >  }
>> > >
>> > > -static 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)
>> > >  {
>> > > -    struct vcpu *v;
>> > > -    int err = 0;
>> > > -
>> > > -    for_each_vcpu ( d, v )
>> > > -    {
>> > > -        struct hvm_vmce_vcpu ctxt;
>> > > +    struct hvm_vmce_vcpu ctxt;
>> > > +    struct vcpu *v = NULL;
>> > >
>> > > -        vmce_save_vcpu_ctxt_one(v, &ctxt);
>> > > -        err = hvm_save_entry(VMCE_VCPU, v->vcpu_id, h, &ctxt);
>> > > -        if ( err )
>> > > -            break;
>> > > -    }
>> > > +    ctxt.caps = v->arch.vmce.mcg_cap;
>> > There's a typo in the commit title (s/form/from), but I don't
>> > understand what you're doing here. You set v to NULL above and
>> > dereference it below. AFAICT, until patch #15 is applied context
>> > saving will be completely broken.
>> Yes, this is true, but it could't find a better way to split the last
>> patch further.
> 
> Can't you do it (something like) this way?
> 
> - Each of patches #1 - #7 register their save_one handler via an extra arg 
> to HVM_REGISTER_SAVE_RESTORE (and hence extra field in hvm_sr_handlers)

I think either there should be a 1st patch introducing the new field and macro
arg, or patches 1...7 remain the way they are and patch 8 introduces and
uses that field without otherwise touching the handlers. In any event all later
patches then shift down by one in numbering; apart from the numbering I
mostly agree with ...

> - Move (current) patch #15 to patch #8 but have it call the save_one 
> handlers
> - Then have 7 patches that remove the now redundant save handlers, renaming 
> XXX_save_one to XXX_save and passing NULL as the now useless argument to 
> HVM_REGISTER_SAVE_RESTORE
> - Then have a final patch deleting the useless arg from 
> HVM_REGISTER_SAVE_RESTORE, cleaning up the callers and also renaming the 
> field in hvm_sr_handlers from save_one to save.

... all of this. However, I have to admit I'm not certain yet whether the
extra argument can indeed go away again in the end: There are save
records which aren't per-vCPU, and I'm not convinced we want to alter
their handling.

Jan



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