Re: [Xen-devel] [PATCH 1/3] x86/emul: Introduce build time assertions for struct segment_register

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/07/17 14:32, Jan Beulich wrote:
>>>> On 03.07.17 at 15:05, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 03/07/17 13:29, Jan Beulich wrote:
>>>>>> On 30.06.17 at 17:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> This structure is shared with hardware in the AMD VMCB.
>>> Indeed, but do we really depend on that in the emulator code?
>>>
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -7899,6 +7899,12 @@ static void __init __maybe_unused 
>>>> build_assertions(void)
>>>>      BUILD_BUG_ON(X86_EVENTTYPE_SW_INTERRUPT != 4);
>>>>      BUILD_BUG_ON(X86_EVENTTYPE_PRI_SW_EXCEPTION != 5);
>>>>      BUILD_BUG_ON(X86_EVENTTYPE_SW_EXCEPTION != 6);
>>>> +
>>>> +    /* 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);
>>> I.e. for these two I don't think I can see any dependency, and ...
>>>
>>>> +    BUILD_BUG_ON(offsetof(struct segment_register, limit) != 4);
>>>> +    BUILD_BUG_ON(offsetof(struct segment_register, base) != 8);
>>> ... for these two I think all we require is >=. Otoh, if these were put
>>> in SVM code, then I could see the point of them being the way they
>>> are.
>> These are indeed only for the SVM code.  See the impending 4/3 patch
>> which I'm about to post.
> Ah, I see. But then I'm still unconvinced we need them here as
> well. I.e. I'd rather see that new patch to replace the one here.

I should have been clearer.  I intend the 4/3 patch to replace this
patch entirely in the series.

>
>>>  I'd then even raise the question whether we wouldn't also want
>>> offsetof() checks.
>> I don't understand; these are offsetof checks.
> Oops, I must have inferred sizeof() from the first one, and with
> the literal numbers being applicable to both this didn't cause any
> inconsistency. In any event - I think we should have both sizeof()
> and offsetof() ones.

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.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel