Re: [Xen-devel] [PATCH 1/5] x86emul: support UMIP

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/09/16 14:42, Jan Beulich wrote:
> To make this complete, also add support for SLDT and STR. Note that by
> just looking at the guest CR4 bit, this is independent of actually
> making available the UMIP feature to guests.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -182,7 +182,7 @@ static const opcode_desc_t opcode_table[
>  
>  static const opcode_desc_t twobyte_table[256] = {
>      /* 0x00 - 0x07 */
> -    SrcMem16|ModRM, ImplicitOps|ModRM, ModRM, ModRM,
> +    ModRM, ImplicitOps|ModRM, ModRM, ModRM,
>      0, ImplicitOps, ImplicitOps, ImplicitOps,
>      /* 0x08 - 0x0F */
>      ImplicitOps, ImplicitOps, 0, ImplicitOps,
> @@ -421,6 +421,7 @@ typedef union {
>  /* Control register flags. */
>  #define CR0_PE    (1<<0)
>  #define CR4_TSD   (1<<2)
> +#define CR4_UMIP  (1<<11)
>  
>  /* EFLAGS bit definitions. */
>  #define EFLG_VIP  (1<<20)
> @@ -1484,6 +1485,17 @@ static bool is_aligned(enum x86_segment
>      return !((reg.base + offs) & (size - 1));
>  }
>  
> +static bool is_umip(struct x86_emulate_ctxt *ctxt,
> +                    const struct x86_emulate_ops *ops)

is_umip is an odd way of phrasing this.  umip_enabled() or
is_umip_enabled() would be better.

> +{
> +    unsigned long cr4;
> +
> +    /* Intentionally not using mode_ring0() here to avoid its fail_if(). */
> +    return get_cpl(ctxt, ops) > 0 &&
> +           ops->read_cr && ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY &&
> +           (cr4 & CR4_UMIP);
> +}
> +
>  /* Inject a software interrupt/exception, emulating if needed. */
>  static int inject_swint(enum x86_swint_type type,
>                          uint8_t vector, uint8_t insn_len,
> @@ -2051,10 +2063,20 @@ x86_decode(
>              break;
>  
>          case ext_0f:
> -        case ext_0f3a:
> -        case ext_8f08:
> -        case ext_8f09:
> -        case ext_8f0a:
> +            switch ( b )
> +            {
> +            case 0x00: /* Grp6 */
> +                switch ( modrm_reg & 6 )
> +                {
> +                case 0:
> +                    d |= DstMem | SrcImplicit | Mov;
> +                    break;
> +                case 2: case 4:
> +                    d |= SrcMem16;
> +                    break;
> +                }
> +                break;
> +            }
>              break;
>  
>          case ext_0f38:
> @@ -2070,6 +2092,12 @@ x86_decode(
>              }
>              break;
>  
> +        case ext_0f3a:
> +        case ext_8f08:
> +        case ext_8f09:
> +        case ext_8f0a:
> +            break;
> +
>          default:
>              ASSERT_UNREACHABLE();
>          }
> @@ -4177,14 +4205,31 @@ x86_emulate(
>          }
>          break;
>  
> -    case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */
> -        fail_if((modrm_reg & 6) != 2);
> +    case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */ {

Newline for {

> +        enum x86_segment seg = (modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr;
> +
> +        fail_if(modrm_reg & 4);
>          generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
> -        generate_exception_if(!mode_ring0(), EXC_GP, 0);
> -        if ( (rc = load_seg((modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr,
> -                            src.val, 0, NULL, ctxt, ops)) != 0 )
> -            goto done;
> +        if ( modrm_reg & 2 )

This needs to be (modrm_reg & 6) == 2.  Otherwise, the /6 and /7
encodings will also raise #GP when they should raise #UD

Actually thinking about it, could we just have a full switch here like
other Grp $N decodes?

~Andrew

> +        {
> +            generate_exception_if(!mode_ring0(), EXC_GP, 0);
> +            if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 )
> +                goto done;
> +        }
> +        else
> +        {
> +            struct segment_register reg;
> +
> +            generate_exception_if(is_umip(ctxt, ops), EXC_GP, 0);
> +            fail_if(!ops->read_segment);
> +            if ( (rc = ops->read_segment(seg, &reg, ctxt)) != 0 )
> +                goto done;
> +            dst.val = reg.sel;
> +            if ( dst.type == OP_MEM )
> +                dst.bytes = 2;
> +        }
>          break;
> +    }
>  
>      case X86EMUL_OPC(0x0f, 0x01): /* Grp7 */ {
>          struct segment_register reg;
> @@ -4282,6 +4327,7 @@ x86_emulate(
>          case 0: /* sgdt */
>          case 1: /* sidt */
>              generate_exception_if(ea.type != OP_MEM, EXC_UD, -1);
> +            generate_exception_if(is_umip(ctxt, ops), EXC_GP, 0);
>              fail_if(ops->read_segment == NULL);
>              if ( (rc = ops->read_segment((modrm_reg & 1) ?
>                                           x86_seg_idtr : x86_seg_gdtr,
> @@ -4316,6 +4362,7 @@ x86_emulate(
>                  goto done;
>              break;
>          case 4: /* smsw */
> +            generate_exception_if(is_umip(ctxt, ops), EXC_GP, 0);
>              ea.bytes = (ea.type == OP_MEM) ? 2 : op_bytes;
>              dst = ea;
>              fail_if(ops->read_cr == NULL);
>
>


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