Re: [PATCH] hvf: Fix segment selector format

[Roman Bolshakov]

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