On 04/12/2019 10:06, Jan Beulich wrote: > On 04.12.2019 10:43, Andrew Cooper wrote: >> The vmcb is zeroed on allocate - drop all explicit writes of 0. Move >> hvm_update_guest_efer() to co-locate it with the other control register >> updates. >> >> Move the BUILD_BUG_ON() into build_assertions(), and add some offset checks >> for fields after the large blocks of reserved fields (as these are the most >> likely to trigger from a mis-edit). Take the opportunity to fold 6 adjacent >> res* fields into one. >> >> Finally, drop all trailing whitespace in the file. >> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> > Reviewed-by: Jan Beulich <jbeul...@suse.com> > albeit with two (optional) suggestions: > >> @@ -297,14 +258,26 @@ void __init setup_vmcb_dump(void) >> >> static void __init __maybe_unused build_assertions(void) >> { >> - struct segment_register sreg; >> + struct vmcb_struct vmcb; >> + >> + /* Build-time check of the VMCB layout. */ >> + BUILD_BUG_ON(sizeof(vmcb) != PAGE_SIZE); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, _pause_filter_thresh) != >> 0x03c); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, _vintr) != >> 0x060); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, eventinj) != >> 0x0a8); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, es) != >> 0x400); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, _cpl) != >> 0x4cb); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, _cr4) != >> 0x548); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, rsp) != >> 0x5d8); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, rax) != >> 0x5f8); >> + BUILD_BUG_ON(offsetof(struct vmcb_struct, _g_pat) != >> 0x668); >> >> /* Check struct segment_register against the VMCB segment layout. */ >> - BUILD_BUG_ON(sizeof(sreg) != 16); >> - BUILD_BUG_ON(sizeof(sreg.sel) != 2); >> - BUILD_BUG_ON(sizeof(sreg.attr) != 2); >> - BUILD_BUG_ON(sizeof(sreg.limit) != 4); >> - BUILD_BUG_ON(sizeof(sreg.base) != 8); >> + BUILD_BUG_ON(sizeof(vmcb.es) != 16); >> + BUILD_BUG_ON(sizeof(vmcb.es.sel) != 2); >> + BUILD_BUG_ON(sizeof(vmcb.es.attr) != 2); >> + BUILD_BUG_ON(sizeof(vmcb.es.limit) != 4); >> + BUILD_BUG_ON(sizeof(vmcb.es.base) != 8); >> BUILD_BUG_ON(offsetof(struct segment_register, sel) != 0); >> BUILD_BUG_ON(offsetof(struct segment_register, attr) != 2); >> BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4); > For the ones only supplying context here, how about using the > shorter offsetof(typeof(vmcb.es), ...), also tying things better > to the prior sizeof() checks? The same, albeit to a lesser degree, > might then go for the earlier block, which could use the shorter > typeof(vmcb).
Fixed. > >> --- a/xen/include/asm-x86/hvm/svm/vmcb.h >> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h >> @@ -406,12 +406,7 @@ struct vmcb_struct { >> u32 _exception_intercepts; /* offset 0x08 - cleanbit 0 */ >> u32 _general1_intercepts; /* offset 0x0C - cleanbit 0 */ >> u32 _general2_intercepts; /* offset 0x10 - cleanbit 0 */ >> - u32 res01; /* offset 0x14 */ >> - u64 res02; /* offset 0x18 */ >> - u64 res03; /* offset 0x20 */ >> - u64 res04; /* offset 0x28 */ >> - u64 res05; /* offset 0x30 */ >> - u32 res06; /* offset 0x38 */ >> + u32 res01[10]; > Was it intentional for the comment to be lost altogether? Yes. The offset is trivial (0x10 + sizeof(u32)) and of no interest. Omitting it increases readability by helping to highlight where the reserved blocks are. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel