| Message ID | 592E8B33020000780015DFC4@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 31/05/17 08:21, Jan Beulich wrote: > Prevent accidental mistakes by not requiring explicit types to be > specified in the macro invocations. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I am not a fan of these accessors being macro-generated; I've lost count of the number of times I've tried greping for one of them, just to finally remember that they can't be searched for. OTOH, this change doesn't make that problem worse, and does fix one issue in the current setup. One comment however... > /* Updates are all via hvm_set_segment_register(). */ > -/* VMCB_ACCESSORS(svm_segment_register_t, gdtr, dt) */ > -/* VMCB_ACCESSORS(svm_segment_register_t, idtr, dt) */ > -/* VMCB_ACCESSORS(svm_segment_register_t, cs, seg) */ > -/* VMCB_ACCESSORS(svm_segment_register_t, ds, seg) */ > -/* VMCB_ACCESSORS(svm_segment_register_t, es, seg) */ > -/* VMCB_ACCESSORS(svm_segment_register_t, ss, seg) */ > -VMCB_ACCESSORS(u8, cpl, seg) > -VMCB_ACCESSORS(u64, cr2, cr2) > -VMCB_ACCESSORS(u64, debugctlmsr, lbr) > -VMCB_ACCESSORS(u64, lastbranchfromip, lbr) > -VMCB_ACCESSORS(u64, lastbranchtoip, lbr) > -VMCB_ACCESSORS(u64, lastintfromip, lbr) > -VMCB_ACCESSORS(u64, lastinttoip, lbr) > +/* VMCB_ACCESSORS(gdtr, dt) */ > +/* VMCB_ACCESSORS(idtr, dt) */ > +/* VMCB_ACCESSORS(cs, seg) */ > +/* VMCB_ACCESSORS(ds, seg) */ > +/* VMCB_ACCESSORS(es, seg) */ > +/* VMCB_ACCESSORS(ss, seg) */ I'd just drop these entirely. I can't see any need for them to be introduced, but even if a need does arise, its not like they are hard to introduce from first principles. ~Andrew
>>> On 31.05.17 at 13:25, <andrew.cooper3@citrix.com> wrote: > On 31/05/17 08:21, Jan Beulich wrote: >> Prevent accidental mistakes by not requiring explicit types to be >> specified in the macro invocations. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I am not a fan of these accessors being macro-generated; I've lost count > of the number of times I've tried greping for one of them, just to > finally remember that they can't be searched for. > > OTOH, this change doesn't make that problem worse, and does fix one > issue in the current setup. One comment however... > >> /* Updates are all via hvm_set_segment_register(). */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, gdtr, dt) */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, idtr, dt) */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, cs, seg) */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, ds, seg) */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, es, seg) */ >> -/* VMCB_ACCESSORS(svm_segment_register_t, ss, seg) */ >> -VMCB_ACCESSORS(u8, cpl, seg) >> -VMCB_ACCESSORS(u64, cr2, cr2) >> -VMCB_ACCESSORS(u64, debugctlmsr, lbr) >> -VMCB_ACCESSORS(u64, lastbranchfromip, lbr) >> -VMCB_ACCESSORS(u64, lastbranchtoip, lbr) >> -VMCB_ACCESSORS(u64, lastintfromip, lbr) >> -VMCB_ACCESSORS(u64, lastinttoip, lbr) >> +/* VMCB_ACCESSORS(gdtr, dt) */ >> +/* VMCB_ACCESSORS(idtr, dt) */ >> +/* VMCB_ACCESSORS(cs, seg) */ >> +/* VMCB_ACCESSORS(ds, seg) */ >> +/* VMCB_ACCESSORS(es, seg) */ >> +/* VMCB_ACCESSORS(ss, seg) */ > > I'd just drop these entirely. I can't see any need for them to be > introduced, but even if a need does arise, its not like they are hard to > introduce from first principles. No problem, but I'll wait to see the SVM maintainers' opinion(s). Jan
On 05/31/2017 07:56 AM, Jan Beulich wrote: >>>> On 31.05.17 at 13:25, <andrew.cooper3@citrix.com> wrote: >> On 31/05/17 08:21, Jan Beulich wrote: >>> Prevent accidental mistakes by not requiring explicit types to be >>> specified in the macro invocations. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> I am not a fan of these accessors being macro-generated; I've lost count >> of the number of times I've tried greping for one of them, just to >> finally remember that they can't be searched for. >> >> OTOH, this change doesn't make that problem worse, and does fix one >> issue in the current setup. One comment however... >> >>> /* Updates are all via hvm_set_segment_register(). */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, gdtr, dt) */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, idtr, dt) */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, cs, seg) */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, ds, seg) */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, es, seg) */ >>> -/* VMCB_ACCESSORS(svm_segment_register_t, ss, seg) */ >>> -VMCB_ACCESSORS(u8, cpl, seg) >>> -VMCB_ACCESSORS(u64, cr2, cr2) >>> -VMCB_ACCESSORS(u64, debugctlmsr, lbr) >>> -VMCB_ACCESSORS(u64, lastbranchfromip, lbr) >>> -VMCB_ACCESSORS(u64, lastbranchtoip, lbr) >>> -VMCB_ACCESSORS(u64, lastintfromip, lbr) >>> -VMCB_ACCESSORS(u64, lastinttoip, lbr) >>> +/* VMCB_ACCESSORS(gdtr, dt) */ >>> +/* VMCB_ACCESSORS(idtr, dt) */ >>> +/* VMCB_ACCESSORS(cs, seg) */ >>> +/* VMCB_ACCESSORS(ds, seg) */ >>> +/* VMCB_ACCESSORS(es, seg) */ >>> +/* VMCB_ACCESSORS(ss, seg) */ >> I'd just drop these entirely. I can't see any need for them to be >> introduced, but even if a need does arise, its not like they are hard to >> introduce from first principles. > No problem, but I'll wait to see the SVM maintainers' opinion(s). I don't have an opinion one way or the other. Either way looks fine to me. -boris
--- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -544,51 +544,54 @@ void svm_intercept_msr(struct vcpu *v, u * VMCB accessor functions. */ -#define VMCB_ACCESSORS(_type, _name, _cleanbit) \ -static inline void vmcb_set_##_name(struct vmcb_struct *vmcb, _type value) \ -{ \ - vmcb->_##_name = value; \ - vmcb->cleanbits.fields._cleanbit = 0; \ -} \ -static inline _type vmcb_get_##_name(const struct vmcb_struct *vmcb) \ -{ \ - return vmcb->_##_name; \ +#define VMCB_ACCESSORS(name, cleanbit) \ +static inline void \ +vmcb_set_ ## name(struct vmcb_struct *vmcb, \ + typeof(vmcb->_ ## name) value) \ +{ \ + vmcb->_ ## name = value; \ + vmcb->cleanbits.fields.cleanbit = 0; \ +} \ +static inline typeof(alloc_vmcb()->_ ## name) \ +vmcb_get_ ## name(const struct vmcb_struct *vmcb) \ +{ \ + return vmcb->_ ## name; \ } -VMCB_ACCESSORS(u32, cr_intercepts, intercepts) -VMCB_ACCESSORS(u32, dr_intercepts, intercepts) -VMCB_ACCESSORS(u32, exception_intercepts, intercepts) -VMCB_ACCESSORS(u32, general1_intercepts, intercepts) -VMCB_ACCESSORS(u32, general2_intercepts, intercepts) -VMCB_ACCESSORS(u16, pause_filter_count, intercepts) -VMCB_ACCESSORS(u64, tsc_offset, intercepts) -VMCB_ACCESSORS(u64, iopm_base_pa, iopm) -VMCB_ACCESSORS(u64, msrpm_base_pa, iopm) -VMCB_ACCESSORS(u32, guest_asid, asid) -VMCB_ACCESSORS(vintr_t, vintr, tpr) -VMCB_ACCESSORS(u64, np_enable, np) -VMCB_ACCESSORS(u64, h_cr3, np) -VMCB_ACCESSORS(u64, g_pat, np) -VMCB_ACCESSORS(u64, cr0, cr) -VMCB_ACCESSORS(u64, cr3, cr) -VMCB_ACCESSORS(u64, cr4, cr) -VMCB_ACCESSORS(u64, efer, cr) -VMCB_ACCESSORS(u64, dr6, dr) -VMCB_ACCESSORS(u64, dr7, dr) +VMCB_ACCESSORS(cr_intercepts, intercepts) +VMCB_ACCESSORS(dr_intercepts, intercepts) +VMCB_ACCESSORS(exception_intercepts, intercepts) +VMCB_ACCESSORS(general1_intercepts, intercepts) +VMCB_ACCESSORS(general2_intercepts, intercepts) +VMCB_ACCESSORS(pause_filter_count, intercepts) +VMCB_ACCESSORS(tsc_offset, intercepts) +VMCB_ACCESSORS(iopm_base_pa, iopm) +VMCB_ACCESSORS(msrpm_base_pa, iopm) +VMCB_ACCESSORS(guest_asid, asid) +VMCB_ACCESSORS(vintr, tpr) +VMCB_ACCESSORS(np_enable, np) +VMCB_ACCESSORS(h_cr3, np) +VMCB_ACCESSORS(g_pat, np) +VMCB_ACCESSORS(cr0, cr) +VMCB_ACCESSORS(cr3, cr) +VMCB_ACCESSORS(cr4, cr) +VMCB_ACCESSORS(efer, cr) +VMCB_ACCESSORS(dr6, dr) +VMCB_ACCESSORS(dr7, dr) /* Updates are all via hvm_set_segment_register(). */ -/* VMCB_ACCESSORS(svm_segment_register_t, gdtr, dt) */ -/* VMCB_ACCESSORS(svm_segment_register_t, idtr, dt) */ -/* VMCB_ACCESSORS(svm_segment_register_t, cs, seg) */ -/* VMCB_ACCESSORS(svm_segment_register_t, ds, seg) */ -/* VMCB_ACCESSORS(svm_segment_register_t, es, seg) */ -/* VMCB_ACCESSORS(svm_segment_register_t, ss, seg) */ -VMCB_ACCESSORS(u8, cpl, seg) -VMCB_ACCESSORS(u64, cr2, cr2) -VMCB_ACCESSORS(u64, debugctlmsr, lbr) -VMCB_ACCESSORS(u64, lastbranchfromip, lbr) -VMCB_ACCESSORS(u64, lastbranchtoip, lbr) -VMCB_ACCESSORS(u64, lastintfromip, lbr) -VMCB_ACCESSORS(u64, lastinttoip, lbr) +/* VMCB_ACCESSORS(gdtr, dt) */ +/* VMCB_ACCESSORS(idtr, dt) */ +/* VMCB_ACCESSORS(cs, seg) */ +/* VMCB_ACCESSORS(ds, seg) */ +/* VMCB_ACCESSORS(es, seg) */ +/* VMCB_ACCESSORS(ss, seg) */ +VMCB_ACCESSORS(cpl, seg) +VMCB_ACCESSORS(cr2, cr2) +VMCB_ACCESSORS(debugctlmsr, lbr) +VMCB_ACCESSORS(lastbranchfromip, lbr) +VMCB_ACCESSORS(lastbranchtoip, lbr) +VMCB_ACCESSORS(lastintfromip, lbr) +VMCB_ACCESSORS(lastinttoip, lbr) #undef VMCB_ACCESSORS