[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/3] x86/svm: Drop svm_segment_register_t
>>> On 03.07.17 at 15:39, <andrew.cooper3@xxxxxxxxxx> wrote: > On 03/07/17 14:37, Jan Beulich wrote: >>>>> On 03.07.17 at 15:10, <andrew.cooper3@xxxxxxxxxx> wrote: >>> --- a/xen/arch/x86/hvm/svm/vmcb.c >>> +++ b/xen/arch/x86/hvm/svm/vmcb.c >>> @@ -310,6 +310,15 @@ void __init setup_vmcb_dump(void) >>> register_keyhandler('v', vmcb_dump, "dump AMD-V VMCBs", 1); >>> } >>> >>> +static void __init __maybe_unused build_assertions(void) >>> +{ >>> + /* Check struct segment_register against the VMCB segment layout. */ >>> + BUILD_BUG_ON(sizeof(struct segment_register) != 16); >>> + BUILD_BUG_ON(offsetof(struct segment_register, attr) != 2); >>> + BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4); >>> + BUILD_BUG_ON(offsetof(struct segment_register, base) != 8); >>> +} >> As said in reply to patch 1, I think we want to check both position >> and size here. With respective sizeof() checks added (and both >> for the so far missing sel field) >> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> > > Lets merge that part of the thread in here: > > I don't see what the individual sizeof() checks gains us. The only > field where a sizeof check would be useful is the attr union, but > offsetof(following field) is the only way to infer the size, as the > union is anonymous. > > For the other fields, we'd be checking sizeof(uint{16,32,64}_t) being > correct, which is overkill. Well, my line of thinking is that if someone changed the structure definition, despite the comment not realizing it's tied to something hardware determines, they should see a build failure, no matter whether this affects field offsets, sizes, or both. In a way we really mean to check here that what is being used in the structure definition are uint{16,32,64}_t; ideally we'd even be able to check these are all unsigned types. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |