Re: [Xen-devel] [PATCH v3] arch/x86: Add registers to vm_event

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.10.18 at 15:10, <aisaila@xxxxxxxxxxxxxxx> wrote:
> On 25.10.2018 14:55, Jan Beulich wrote:
>>>>> On 18.10.18 at 11:02, <aisaila@xxxxxxxxxxxxxxx> wrote:
>>> @@ -157,6 +157,19 @@
>>>   #define VM_EVENT_X86_CR4    2
>>>   #define VM_EVENT_X86_XCR0   3
>>>   
>>> +struct x86_selector_reg {
>>> +    union
>>> +    {
>>> +        uint64_t bytes;
>>> +        struct
>>> +        {
>>> +            uint32_t base;
>>> +            uint32_t limit  :    20;
>>> +            uint32_t ar     :    12;
>>> +        } fields;
>>> +    };
>>> +};
>> 
>> I don't understand why sel was moved out. Are you tight on
>> space here, such that you can't tolerate the padding bytes?
> 
> It was dropped on Andrew's suggestion. We are ok with it in the 
> structure so if is ok by you I can add it back.
> 
>> 
>> I also question the need for a union here. You don't use
>> .bytes anywhere afaics.
> 
> Right now there is no use for the .bytes field and it was put for 
> further usage. I can drop this in the next version.
> 
>> 
>> Finally - what meaning to the low (or high) 4 bits of "ar"
>> carry?
> 
> If I correctly understand the question, we use ar bits to  determine the 
> running mode of the guest.

Actually I was wrongly thinking 4 of the bits would be unused. With
there not being any unused bits, the layout - not matching the LAR
insn output - should at least be clarified in a comment.

>>> @@ -193,7 +206,19 @@ struct vm_event_regs_x86 {
>>>       uint64_t msr_lstar;
>>>       uint64_t fs_base;
>>>       uint64_t gs_base;
>> 
>> You previously removed them, and I think that was correct.
>> The field in the union should be uint64_t. Right now you leave
>> fs.base and gs.base uninitialized.
>> 
> 
> We want the structure to be tight so that is why .base is uint32. If we 
> move the fs/gs base in the new structure and make base to be uint64 then 
> there are some useless bits there.

Then you're still wasting 8 bytes for the unused FS/GS base sub-fields.

> The question is if we can leave the code like this and init de remaining 
> fields? From what I am seeing the fs/gs base should remain uint64.

If packing is important, I'd suggest separate structure types for
CS/SS/DS/ES and FS/GS.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel