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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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);

?

> @@ -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.

> +        host_and_vcpu_must_have(gfni);
> +        if ( vex.opcx == vex_none )
> +            goto simd_0f38_common;
> +        else

Pointless else.

> +            goto simd_0f_avx;

vex.w needs to be checked before this goto.

> @@ -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.

> +
> +

Please don't introduce double blank lines.

Jan


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