Re: [Xen-devel] [PATCH 4/3] x86/svm: Drop svm_segment_register_t

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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