[PATCH v3] arch/x86: Add registers to vm_event

[PATCH v3] arch/x86: Add registers to vm_event

[Alexandru Stefan ISAILA]

This patch adds a couple of regs to the vm_event that are used by
the introspection. The base, limit and ar
bits are compressed into a uint64_t union so as not to enlarge the
vm_event.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V2:
	- Keep fs/gs_base in 64bit long format
	- Pull the sel out of x86_selector_reg
	- Reformat limit field assignment
	- Redesigned vm_event_pack_segment_register().
---
 xen/arch/x86/vm_event.c       | 65 ++++++++++++++++++++++++++++++-----
 xen/include/public/vm_event.h | 29 ++++++++++++++--
 2 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 15de43c3e6..803f97011b 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -122,11 +122,60 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
     v->arch.monitor.next_interrupt_enabled = true;
 }
 
+static void vm_event_pack_segment_register(enum x86_segment segment,
+                                           struct vm_event_regs_x86 *reg)
+{
+    struct segment_register seg;
+
+    hvm_get_segment_register(current, segment, &seg);
+
+    switch ( segment )
+    {
+    case x86_seg_ss:
+        reg->ss.fields.base = seg.base;
+        reg->ss.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->ss.fields.ar = seg.attr;
+        reg->ss_sel = seg.sel;
+        break;
+    case x86_seg_fs:
+        reg->fs_base = seg.base;
+        reg->fs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->fs.fields.ar = seg.attr;
+        reg->fs_sel = seg.sel;
+        break;
+    case x86_seg_gs:
+        reg->gs_base = seg.base;
+        reg->gs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->gs.fields.ar = seg.attr;
+        reg->gs_sel = seg.sel;
+        break;
+    case x86_seg_cs:
+        reg->cs.fields.base = seg.base;
+        reg->cs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->cs.fields.ar = seg.attr;
+        reg->cs_sel = seg.sel;
+        break;
+    case x86_seg_ds:
+        reg->ds.fields.base = seg.base;
+        reg->ds.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->ds.fields.ar = seg.attr;
+        reg->ds_sel = seg.sel;
+        break;
+    case x86_seg_es:
+        reg->es.fields.base = seg.base;
+        reg->es.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
+        reg->es.fields.ar = seg.attr;
+        reg->es_sel = seg.sel;
+        break;
+    default:
+        break;
+    }
+}
+
 void vm_event_fill_regs(vm_event_request_t *req)
 {
 #ifdef CONFIG_HVM
     const struct cpu_user_regs *regs = guest_cpu_user_regs();
-    struct segment_register seg;
     struct hvm_hw_cpu ctxt = {};
     struct vcpu *curr = current;
 
@@ -170,14 +219,14 @@ void vm_event_fill_regs(vm_event_request_t *req)
     req->data.regs.x86.msr_star = ctxt.msr_star;
     req->data.regs.x86.msr_lstar = ctxt.msr_lstar;
 
-    hvm_get_segment_register(curr, x86_seg_fs, &seg);
-    req->data.regs.x86.fs_base = seg.base;
-
-    hvm_get_segment_register(curr, x86_seg_gs, &seg);
-    req->data.regs.x86.gs_base = seg.base;
+    vm_event_pack_segment_register(x86_seg_fs, &req->data.regs.x86);
+    vm_event_pack_segment_register(x86_seg_gs, &req->data.regs.x86);
+    vm_event_pack_segment_register(x86_seg_cs, &req->data.regs.x86);
+    vm_event_pack_segment_register(x86_seg_ss, &req->data.regs.x86);
+    vm_event_pack_segment_register(x86_seg_ds, &req->data.regs.x86);
+    vm_event_pack_segment_register(x86_seg_es, &req->data.regs.x86);
 
-    hvm_get_segment_register(curr, x86_seg_cs, &seg);
-    req->data.regs.x86.cs_arbytes = seg.attr;
+    req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
 #endif
 }
 
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 36e3f4685d..0415ea607d 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -29,7 +29,7 @@
 
 #include "xen.h"
 
-#define VM_EVENT_INTERFACE_VERSION 0x00000003
+#define VM_EVENT_INTERFACE_VERSION 0x00000004
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
@@ -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;
+    };
+};
+
 /*
  * Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
  * so as to not fill the vm_event ring buffer too quickly.
@@ -193,7 +206,19 @@ struct vm_event_regs_x86 {
     uint64_t msr_lstar;
     uint64_t fs_base;
     uint64_t gs_base;
-    uint32_t cs_arbytes;
+    struct x86_selector_reg cs;
+    struct x86_selector_reg ss;
+    struct x86_selector_reg ds;
+    struct x86_selector_reg es;
+    struct x86_selector_reg fs;
+    struct x86_selector_reg gs;
+    uint64_t shadow_gs;
+    uint16_t cs_sel;
+    uint16_t ss_sel;
+    uint16_t ds_sel;
+    uint16_t es_sel;
+    uint16_t fs_sel;
+    uint16_t gs_sel;
     uint32_t _pad;
 };
 
-- 
2.17.1


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

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

[Tamas K Lengyel]

On Thu, Oct 18, 2018 at 3:02 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> This patch adds a couple of regs to the vm_event that are used by
> the introspection. The base, limit and ar
> bits are compressed into a uint64_t union so as not to enlarge the
> vm_event.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

LGTM

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

Re: PING: [PATCH v3] arch/x86: Add registers to vm_event

[Alexandru Stefan ISAILA]

Any thoughts on this are appreciated.

Thanks,
Alex

On 18.10.2018 12:02, Alexandru Stefan ISAILA wrote:
> This patch adds a couple of regs to the vm_event that are used by
> the introspection. The base, limit and ar
> bits are compressed into a uint64_t union so as not to enlarge the
> vm_event.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> 
> ---
> Changes since V2:
> 	- Keep fs/gs_base in 64bit long format
> 	- Pull the sel out of x86_selector_reg
> 	- Reformat limit field assignment
> 	- Redesigned vm_event_pack_segment_register().
> ---
>   xen/arch/x86/vm_event.c       | 65 ++++++++++++++++++++++++++++++-----
>   xen/include/public/vm_event.h | 29 ++++++++++++++--
>   2 files changed, 84 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 15de43c3e6..803f97011b 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -122,11 +122,60 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
>       v->arch.monitor.next_interrupt_enabled = true;
>   }
>   
> +static void vm_event_pack_segment_register(enum x86_segment segment,
> +                                           struct vm_event_regs_x86 *reg)
> +{
> +    struct segment_register seg;
> +
> +    hvm_get_segment_register(current, segment, &seg);
> +
> +    switch ( segment )
> +    {
> +    case x86_seg_ss:
> +        reg->ss.fields.base = seg.base;
> +        reg->ss.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->ss.fields.ar = seg.attr;
> +        reg->ss_sel = seg.sel;
> +        break;
> +    case x86_seg_fs:
> +        reg->fs_base = seg.base;
> +        reg->fs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->fs.fields.ar = seg.attr;
> +        reg->fs_sel = seg.sel;
> +        break;
> +    case x86_seg_gs:
> +        reg->gs_base = seg.base;
> +        reg->gs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->gs.fields.ar = seg.attr;
> +        reg->gs_sel = seg.sel;
> +        break;
> +    case x86_seg_cs:
> +        reg->cs.fields.base = seg.base;
> +        reg->cs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->cs.fields.ar = seg.attr;
> +        reg->cs_sel = seg.sel;
> +        break;
> +    case x86_seg_ds:
> +        reg->ds.fields.base = seg.base;
> +        reg->ds.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->ds.fields.ar = seg.attr;
> +        reg->ds_sel = seg.sel;
> +        break;
> +    case x86_seg_es:
> +        reg->es.fields.base = seg.base;
> +        reg->es.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->es.fields.ar = seg.attr;
> +        reg->es_sel = seg.sel;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
>   void vm_event_fill_regs(vm_event_request_t *req)
>   {
>   #ifdef CONFIG_HVM
>       const struct cpu_user_regs *regs = guest_cpu_user_regs();
> -    struct segment_register seg;
>       struct hvm_hw_cpu ctxt = {};
>       struct vcpu *curr = current;
>   
> @@ -170,14 +219,14 @@ void vm_event_fill_regs(vm_event_request_t *req)
>       req->data.regs.x86.msr_star = ctxt.msr_star;
>       req->data.regs.x86.msr_lstar = ctxt.msr_lstar;
>   
> -    hvm_get_segment_register(curr, x86_seg_fs, &seg);
> -    req->data.regs.x86.fs_base = seg.base;
> -
> -    hvm_get_segment_register(curr, x86_seg_gs, &seg);
> -    req->data.regs.x86.gs_base = seg.base;
> +    vm_event_pack_segment_register(x86_seg_fs, &req->data.regs.x86);
> +    vm_event_pack_segment_register(x86_seg_gs, &req->data.regs.x86);
> +    vm_event_pack_segment_register(x86_seg_cs, &req->data.regs.x86);
> +    vm_event_pack_segment_register(x86_seg_ss, &req->data.regs.x86);
> +    vm_event_pack_segment_register(x86_seg_ds, &req->data.regs.x86);
> +    vm_event_pack_segment_register(x86_seg_es, &req->data.regs.x86);
>   
> -    hvm_get_segment_register(curr, x86_seg_cs, &seg);
> -    req->data.regs.x86.cs_arbytes = seg.attr;
> +    req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>   #endif
>   }
>   
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 36e3f4685d..0415ea607d 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -29,7 +29,7 @@
>   
>   #include "xen.h"
>   
> -#define VM_EVENT_INTERFACE_VERSION 0x00000003
> +#define VM_EVENT_INTERFACE_VERSION 0x00000004
>   
>   #if defined(__XEN__) || defined(__XEN_TOOLS__)
>   
> @@ -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;
> +    };
> +};
> +
>   /*
>    * Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM
>    * so as to not fill the vm_event ring buffer too quickly.
> @@ -193,7 +206,19 @@ struct vm_event_regs_x86 {
>       uint64_t msr_lstar;
>       uint64_t fs_base;
>       uint64_t gs_base;
> -    uint32_t cs_arbytes;
> +    struct x86_selector_reg cs;
> +    struct x86_selector_reg ss;
> +    struct x86_selector_reg ds;
> +    struct x86_selector_reg es;
> +    struct x86_selector_reg fs;
> +    struct x86_selector_reg gs;
> +    uint64_t shadow_gs;
> +    uint16_t cs_sel;
> +    uint16_t ss_sel;
> +    uint16_t ds_sel;
> +    uint16_t es_sel;
> +    uint16_t fs_sel;
> +    uint16_t gs_sel;
>       uint32_t _pad;
>   };
>   
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: PING: [PATCH v3] arch/x86: Add registers to vm_event

[Tamas K Lengyel]

On Tue, Oct 23, 2018 at 2:37 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> wrote:
>
> Any thoughts on this are appreciated.

FYI I already acked this.

Tamas

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

Re: PING: [PATCH v3] arch/x86: Add registers to vm_event

[Alexandru Stefan ISAILA]

Yes, sorry this was aimed at Andrew and Jan(when he will be in).

Alex

On 24.10.2018 19:48, Tamas K Lengyel wrote:
> On Tue, Oct 23, 2018 at 2:37 AM Alexandru Stefan ISAILA
> <aisaila@bitdefender.com> wrote:
>>
>> Any thoughts on this are appreciated.
> 
> FYI I already acked this.
> 
> Tamas
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 18.10.18 at 11:02, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -122,11 +122,60 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
>      v->arch.monitor.next_interrupt_enabled = true;
>  }
>  
> +static void vm_event_pack_segment_register(enum x86_segment segment,
> +                                           struct vm_event_regs_x86 *reg)
> +{
> +    struct segment_register seg;
> +
> +    hvm_get_segment_register(current, segment, &seg);
> +
> +    switch ( segment )
> +    {
> +    case x86_seg_ss:
> +        reg->ss.fields.base = seg.base;
> +        reg->ss.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->ss.fields.ar = seg.attr;
> +        reg->ss_sel = seg.sel;
> +        break;
> +    case x86_seg_fs:

Blank lines between individual case blocks please.

> +        reg->fs_base = seg.base;
> +        reg->fs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->fs.fields.ar = seg.attr;
> +        reg->fs_sel = seg.sel;
> +        break;
> +    case x86_seg_gs:
> +        reg->gs_base = seg.base;
> +        reg->gs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->gs.fields.ar = seg.attr;
> +        reg->gs_sel = seg.sel;
> +        break;
> +    case x86_seg_cs:
> +        reg->cs.fields.base = seg.base;
> +        reg->cs.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->cs.fields.ar = seg.attr;
> +        reg->cs_sel = seg.sel;
> +        break;
> +    case x86_seg_ds:
> +        reg->ds.fields.base = seg.base;
> +        reg->ds.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->ds.fields.ar = seg.attr;
> +        reg->ds_sel = seg.sel;
> +        break;
> +    case x86_seg_es:
> +        reg->es.fields.base = seg.base;
> +        reg->es.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
> +        reg->es.fields.ar = seg.attr;
> +        reg->es_sel = seg.sel;
> +        break;
> +    default:
> +        break;

Either add ASSERT_UNREACHABLE() or drop the default case.

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

I also question the need for a union here. You don't use
.bytes anywhere afaics.

Finally - what meaning to the low (or high) 4 bits of "ar"
carry?

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

Jan



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

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

[Alexandru Stefan ISAILA]

On 25.10.2018 14:55, Jan Beulich wrote:
>>>> On 18.10.18 at 11:02, <aisaila@bitdefender.com> wrote:
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -122,11 +122,60 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
>>       v->arch.monitor.next_interrupt_enabled = true;
>>   }
>>   
>> +static void vm_event_pack_segment_register(enum x86_segment segment,
>> +                                           struct vm_event_regs_x86 *reg)
>> +{
>> +    struct segment_register seg;
>> +
>> +    hvm_get_segment_register(current, segment, &seg);
>> +
>> +    switch ( segment )
>> +    {
>> +    case x86_seg_ss:
>> +        reg->ss.fields.base = seg.base;
>> +        reg->ss.fields.limit = seg.g ? seg.limit >> 12 : seg.limit;
>> +        reg->ss.fields.ar = seg.attr;
>> +        reg->ss_sel = seg.sel;
>> +        break;
>> +    case x86_seg_fs:
> 
> Blank lines between individual case blocks please.

Will add

(...)
> 
> Either add ASSERT_UNREACHABLE() or drop the default case.

I will drop it in the next version.

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

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

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.

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

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

[Razvan Cojocaru]

On 10/25/18 4:10 PM, Alexandru Stefan ISAILA wrote:
> On 25.10.2018 14:55, Jan Beulich wrote:
>>>>> On 18.10.18 at 11:02, <aisaila@bitdefender.com> wrote:
>>> +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.

We are tight on space. The ring buffer is only 4K, and it already can
only fit about 10 sync vm_events before VCPUs need to be blocked waiting
for a place in the buffer. That's why we've attempted the packing.

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

That's what we need the CS .ar for: looking at CS_AR_BYTES_D and
CS_AR_BYTES_L, the rest we pass along to the introspection engine - I'd
have to check with them for more details if requested.

But I think we've probably misunderstood the question.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 25.10.18 at 15:10, <aisaila@bitdefender.com> wrote:
> On 25.10.2018 14:55, Jan Beulich wrote:
>>>>> On 18.10.18 at 11:02, <aisaila@bitdefender.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel