[PATCH for-next] x86/pv: Factor out the calculation of LDT/GDT descriptor pointers

[PATCH for-next] x86/pv: Factor out the calculation of LDT/GDT descriptor pointers

[Andrew Cooper]

Rather than opencoding it in two places.  While only used in the PV emulation
code, this helper is in principle usable anywhere in the hypervisor.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/pv/emul-gate-op.c |  5 +----
 xen/arch/x86/pv/emulate.c      |  6 +-----
 xen/arch/x86/pv/emulate.h      | 11 +++++++++++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
index 0f89c91..14ce95e 100644
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -54,11 +54,8 @@ static int read_gate_descriptor(unsigned int gate_sel,
                                 unsigned int *ar)
 {
     struct desc_struct desc;
-    const struct desc_struct *pdesc;
+    const struct desc_struct *pdesc = gdt_ldt_desc_ptr(gate_sel);
 
-    pdesc = (const struct desc_struct *)
-        (!(gate_sel & 4) ? GDT_VIRT_START(v) : LDT_VIRT_START(v))
-        + (gate_sel >> 3);
     if ( (gate_sel < 4) ||
          ((gate_sel >= FIRST_RESERVED_GDT_BYTE) && !(gate_sel & 4)) ||
          __get_user(desc, pdesc) )
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 5750c76..1b60911 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -33,11 +33,7 @@ int pv_emul_read_descriptor(unsigned int sel, const struct vcpu *v,
 
     if ( sel < 4)
         desc.b = desc.a = 0;
-    else if ( __get_user(desc,
-                         (const struct desc_struct *)(!(sel & 4)
-                                                      ? GDT_VIRT_START(v)
-                                                      : LDT_VIRT_START(v))
-                         + (sel >> 3)) )
+    else if ( __get_user(desc, gdt_ldt_desc_ptr(sel)) )
         return 0;
     if ( !insn_fetch )
         desc.b &= ~_SEGMENT_L;
diff --git a/xen/arch/x86/pv/emulate.h b/xen/arch/x86/pv/emulate.h
index 656c12f..9d58794 100644
--- a/xen/arch/x86/pv/emulate.h
+++ b/xen/arch/x86/pv/emulate.h
@@ -1,6 +1,7 @@
 #ifndef __PV_EMULATE_H__
 #define __PV_EMULATE_H__
 
+#include <asm/processor.h>
 #include <asm/x86_emulate.h>
 
 int pv_emul_read_descriptor(unsigned int sel, const struct vcpu *v,
@@ -16,4 +17,14 @@ static inline int pv_emul_is_mem_write(const struct x86_emulate_state *state,
                                               : X86EMUL_UNHANDLEABLE;
 }
 
+/* Return a pointer to the GDT/LDT descriptor referenced by sel. */
+static inline const struct desc_struct *gdt_ldt_desc_ptr(unsigned int sel)
+{
+    const struct vcpu *curr = current;
+    const struct desc_struct *tbl = (void *)
+        ((sel & X86_XEC_TI) ? LDT_VIRT_START(curr) : GDT_VIRT_START(curr));
+
+    return &tbl[sel >> 3];
+}
+
 #endif /* __PV_EMULATE_H__ */
-- 
2.1.4


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

Re: [PATCH for-next] x86/pv: Factor out the calculation of LDT/GDT descriptor pointers

[Wei Liu]

On Tue, Oct 17, 2017 at 04:05:23PM +0100, Andrew Cooper wrote:
> Rather than opencoding it in two places.  While only used in the PV emulation
> code, this helper is in principle usable anywhere in the hypervisor.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-next] x86/pv: Factor out the calculation of LDT/GDT descriptor pointers

[Jan Beulich]

>>> On 17.10.17 at 17:05, <andrew.cooper3@citrix.com> wrote:
> @@ -16,4 +17,14 @@ static inline int pv_emul_is_mem_write(const struct x86_emulate_state *state,
>                                                : X86EMUL_UNHANDLEABLE;
>  }
>  
> +/* Return a pointer to the GDT/LDT descriptor referenced by sel. */
> +static inline const struct desc_struct *gdt_ldt_desc_ptr(unsigned int sel)

I guess returning a pointer to const here is on the assumption that
you hope we would never have a need to fiddle with the descriptor?

> +{
> +    const struct vcpu *curr = current;
> +    const struct desc_struct *tbl = (void *)
> +        ((sel & X86_XEC_TI) ? LDT_VIRT_START(curr) : GDT_VIRT_START(curr));

While the two happen to match, using an error code related
constant with something named "selector" doesn't look to be
really correct. But given the match, I don't mind it being this
way.

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH for-next] x86/pv: Factor out the calculation of LDT/GDT descriptor pointers

[Andrew Cooper]

On 26/10/17 16:06, Jan Beulich wrote:
>>>> On 17.10.17 at 17:05, <andrew.cooper3@citrix.com> wrote:
>> @@ -16,4 +17,14 @@ static inline int pv_emul_is_mem_write(const struct x86_emulate_state *state,
>>                                                : X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> +/* Return a pointer to the GDT/LDT descriptor referenced by sel. */
>> +static inline const struct desc_struct *gdt_ldt_desc_ptr(unsigned int sel)
> I guess returning a pointer to const here is on the assumption that
> you hope we would never have a need to fiddle with the descriptor?

PV guests only have a single method to alter the entries of exiting
descriptor tables, HYPERVISOR_update_descriptor(), and that is indexed
by machine address.

We don't even support updates via emulation, and I don't expect this to
change moving forwards.

Of course, we can drop the const if things do change in the future.

>
>> +{
>> +    const struct vcpu *curr = current;
>> +    const struct desc_struct *tbl = (void *)
>> +        ((sel & X86_XEC_TI) ? LDT_VIRT_START(curr) : GDT_VIRT_START(curr));
> While the two happen to match, using an error code related
> constant with something named "selector" doesn't look to be
> really correct. But given the match, I don't mind it being this
> way.
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

As some point, I was thinking of moving some of my XTF x86 library into
Xen, and clean up the multiple/inconsistent definitions we have of
various architectural bits, but that is a job for a different day.

~Andrew

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