[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 08/15] x86/cpu: Remove loop form vmce_save_vcpu_ctxt() func
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |