hvf: Fix segment selector format

[PATCH] hvf: Fix segment selector format

Posted by Jessica Clarke 3 years, 5 months ago

The Requested Privilege Level field is 2 bits, the Table Indicator field
is 1 bit and the Index field is the remaining 15 bits, with TI=0 meaning
GDT and TI=1 meaning LDT.

Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
---
 target/i386/hvf/x86.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
index bacade7b65..ea3e1b86b3 100644
--- a/target/i386/hvf/x86.h
+++ b/target/i386/hvf/x86.h
@@ -214,16 +214,16 @@ static inline uint32_t x86_call_gate_offset(x86_call_gate *gate)
     return (uint32_t)((gate->offset1 << 16) | gate->offset0);
 }
 
-#define LDT_SEL     0
-#define GDT_SEL     1
+#define GDT_SEL     0
+#define LDT_SEL     1
 
 typedef struct x68_segment_selector {
     union {
         uint16_t sel;
         struct {
-            uint16_t rpl:3;
+            uint16_t rpl:2;
             uint16_t ti:1;
-            uint16_t index:12;
+            uint16_t index:13;
         };
     };
 } __attribute__ ((__packed__)) x68_segment_selector;
-- 
2.28.0

Re: [PATCH] hvf: Fix segment selector format

Posted by Paolo Bonzini 3 years, 5 months ago

On 16/11/20 21:04, Jessica Clarke wrote:
> The Requested Privilege Level field is 2 bits, the Table Indicator field
> is 1 bit and the Index field is the remaining 15 bits, with TI=0 meaning
> GDT and TI=1 meaning LDT.
> 
> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
> ---
>   target/i386/hvf/x86.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
> index bacade7b65..ea3e1b86b3 100644
> --- a/target/i386/hvf/x86.h
> +++ b/target/i386/hvf/x86.h
> @@ -214,16 +214,16 @@ static inline uint32_t x86_call_gate_offset(x86_call_gate *gate)
>       return (uint32_t)((gate->offset1 << 16) | gate->offset0);
>   }
>   
> -#define LDT_SEL     0
> -#define GDT_SEL     1
> +#define GDT_SEL     0
> +#define LDT_SEL     1
>   
>   typedef struct x68_segment_selector {
>       union {
>           uint16_t sel;
>           struct {
> -            uint16_t rpl:3;
> +            uint16_t rpl:2;
>               uint16_t ti:1;
> -            uint16_t index:12;
> +            uint16_t index:13;
>           };
>       };
>   } __attribute__ ((__packed__)) x68_segment_selector;
> 

I queued the patch, thanks.

On further look, though, the bitfield part of the struct is almost never 
used, and therefore most uses of the struct itself are more or less 
superfluous (apart from some typechecking).  In particular, 
vmx_read_segment_selector and vmx_write_segment_selector only use the 
16-bit .self field, and the code would be simpler if it was changed to 
just use a uint16_t.

The only place that "needs" the struct is in vmx_handle_task_switch's 
calls to x86_read_segment_descriptor and x86_write_segment_descriptor. 
Those are also the places that benefit from this patch.  But even then, 
for the sake of consistency it would make sense for x86_segment_selector 
to be used only inside those two functions; the arguments could be just 
an uint16_t.

Paolo

Re: [PATCH] hvf: Fix segment selector format

Posted by Roman Bolshakov 3 years, 5 months ago

On Wed, Nov 18, 2020 at 09:58:37AM +0100, Paolo Bonzini wrote:
> On 16/11/20 21:04, Jessica Clarke wrote:
> > The Requested Privilege Level field is 2 bits, the Table Indicator field
> > is 1 bit and the Index field is the remaining 15 bits, with TI=0 meaning
> > GDT and TI=1 meaning LDT.
> > 
> > Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
> > ---
> >   target/i386/hvf/x86.h | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
> > index bacade7b65..ea3e1b86b3 100644
> > --- a/target/i386/hvf/x86.h
> > +++ b/target/i386/hvf/x86.h
> > @@ -214,16 +214,16 @@ static inline uint32_t x86_call_gate_offset(x86_call_gate *gate)
> >       return (uint32_t)((gate->offset1 << 16) | gate->offset0);
> >   }
> > -#define LDT_SEL     0
> > -#define GDT_SEL     1
> > +#define GDT_SEL     0
> > +#define LDT_SEL     1
> >   typedef struct x68_segment_selector {
> >       union {
> >           uint16_t sel;
> >           struct {
> > -            uint16_t rpl:3;
> > +            uint16_t rpl:2;
> >               uint16_t ti:1;
> > -            uint16_t index:12;
> > +            uint16_t index:13;
> >           };
> >       };
> >   } __attribute__ ((__packed__)) x68_segment_selector;
> > 
> 
> I queued the patch, thanks.
> 
> On further look, though, the bitfield part of the struct is almost never
> used, and therefore most uses of the struct itself are more or less
> superfluous (apart from some typechecking).  In particular,
> vmx_read_segment_selector and vmx_write_segment_selector only use the 16-bit
> .self field, and the code would be simpler if it was changed to just use a
> uint16_t.
> 

IIRC, that's because vmx_handle_task_switch is incomplete and needs
improvement. Certain task switches aren't implemented.

> The only place that "needs" the struct is in vmx_handle_task_switch's calls
> to x86_read_segment_descriptor and x86_write_segment_descriptor. Those are
> also the places that benefit from this patch.  But even then, for the sake
> of consistency it would make sense for x86_segment_selector to be used only
> inside those two functions; the arguments could be just an uint16_t.
> 

Reusing some bits of TCG for task switching would be the most helpful
from functional perspective and to avoid code duplication.

Thanks,
Roman

Re: [PATCH] hvf: Fix segment selector format

Posted by Roman Bolshakov 3 years, 5 months ago

On Mon, Nov 16, 2020 at 08:04:14PM +0000, Jessica Clarke wrote:
> The Requested Privilege Level field is 2 bits, the Table Indicator field
> is 1 bit and the Index field is the remaining 15 bits, with TI=0 meaning
> GDT and TI=1 meaning LDT.
> 
> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
> ---
>  target/i386/hvf/x86.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
> index bacade7b65..ea3e1b86b3 100644
> --- a/target/i386/hvf/x86.h
> +++ b/target/i386/hvf/x86.h
> @@ -214,16 +214,16 @@ static inline uint32_t x86_call_gate_offset(x86_call_gate *gate)
>      return (uint32_t)((gate->offset1 << 16) | gate->offset0);
>  }
>  
> -#define LDT_SEL     0
> -#define GDT_SEL     1
> +#define GDT_SEL     0
> +#define LDT_SEL     1
>  
>  typedef struct x68_segment_selector {
>      union {
>          uint16_t sel;
>          struct {
> -            uint16_t rpl:3;
> +            uint16_t rpl:2;
>              uint16_t ti:1;
> -            uint16_t index:12;
> +            uint16_t index:13;
>          };
>      };
>  } __attribute__ ((__packed__)) x68_segment_selector;
> -- 
> 2.28.0
> 

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman