Re: [Xen-devel] [PATCH v3 1/4] x86emul: Support GFNI insns

[Yang Zhong <yang.zhong@xxxxxxxxx>]

On Mon, Nov 27, 2017 at 08:42:29AM -0700, Jan Beulich wrote:
> >>> On 10.11.17 at 11:36, <yang.zhong@xxxxxxxxx> wrote:
> > Signed-off-by: Yang Zhong <yang.zhong@xxxxxxxxx>
> 
> First and foremost - did you try out your own patch? There not being
> any (minimal) test added makes this at least questionable.
> 
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -385,6 +385,7 @@ static const struct {
> >      [0x40] = { .simd_size = simd_packed_int },
> >      [0x41] = { .simd_size = simd_packed_int, .two_op = 1 },
> >      [0xc8 ... 0xcd] = { .simd_size = simd_other },
> > +    [0xcf] = { .simd_size = simd_other },
> 
> Why simd_other? And if that's really the right choice, where do you
> set op_bytes, which is required for this attribute due to
> 
>     if ( state->simd_size )
>     {
>         generate_exception_if(!op_bytes, EXC_UD);
> 
>?

  Hello Jan,

  Thanks for review patch!
  I checked the Galois Field related theorem, this should be simd_size = 
simd_packed_int.

  I will change this, thanks!

  Regards,

  Yang

> 
> > @@ -7356,6 +7359,14 @@ x86_emulate(
> >          op_bytes = 16;
> >          goto simd_0f38_common;
> >  
> > +    case X86EMUL_OPC_66(0x0f38, 0xcf):       /* gf2p8mulb xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f38, 0xcf):   /* vgf2p8mulb xmm/m128,xmm*/
> 
> Please provide correct comments - the VEX variant comes with two
> sizes. Also please add the missing blanks at the end.

  I will change those comments, thanks!
> 
> > +        host_and_vcpu_must_have(gfni);
> > +        if ( vex.opcx == vex_none )
> > +            goto simd_0f38_common;
> > +        else
> 
> Pointless else.

  Thanks, this else is not useful.
> 
> > +            goto simd_0f_avx;
> 
> vex.w needs to be checked before this goto.

  I checked the document again, there is no vex.w exception to check.
  If vex.w is necessary, i will add this check in here. thanks!

> 
> > @@ -7741,6 +7752,17 @@ x86_emulate(
> >          op_bytes = 16;
> >          goto simd_0f3a_common;
> >  
> > +    case X86EMUL_OPC_66(0x0f3a, 0xce):     /* gf2p8affineqb 
> > $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xce): /* vgf2p8affineqb 
> > $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_66(0x0f3a, 0xcf):     /* gf2p8affineinvqb 
> > $imm8,xmm/m128,xmm*/
> > +    case X86EMUL_OPC_VEX_66(0x0f3a, 0xcf): /* vgf2p8affineinvqb 
> > $imm8,xmm/m128,xmm*/
> > +        host_and_vcpu_must_have(gfni);
> > +        if ( vex.opcx == vex_none )
> > +            goto simd_0f3a_common;
> > +        else
> > +            goto simd_0f_imm8_avx;
> 
> Similar comments apply here.

  I will remove else and add vex.w check here as above, thanks!
> 
> > +
> > +
> 
> Please don't introduce double blank lines.

  thanks, i will be care of this.
> 
> Jan

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