Re: [Xen-devel] [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 27.11.17 at 19:08,  wrote:
> On 27/11/17 17:01, Jan Beulich wrote:
> On 26.10.17 at 19:03,  wrote:
>>> +return X86EMUL_OKAY;
>> This and ...
>>
>>> +}
>>> +else
>>> +{
>>> +pagefault_info_t pfinfo;
>>> +int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, 
>>> );
>>> +
>>> +if ( rc == HVMTRANS_bad_linear_to_gfn )
>>> +hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
>>> +if ( rc != HVMTRANS_okay )
>>> +return X86EMUL_EXCEPTION;
>>> +
>>> +return X86EMUL_OKAY;
>> ... this should become a uniform ...
>>
>>> +}
>> ... return here.
> 
> I tried this, but later patches in the series need them to move back. 
> Overall, this layout reduces the code churn across the series.

Ah, I see.

Jan


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

Re: [Xen-devel] [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 26.10.17 at 19:03,  wrote:
> +static int operand_read(void *buf, struct vmx_inst_op *op,
> +struct cpu_user_regs *regs, unsigned int bytes)

const (twice)

> +{
> +if ( op->type == VMX_INST_MEMREG_TYPE_REG )
> +{
> +switch ( bytes )
> +{
> +case 4:
> +*(uint32_t *)buf = reg_read(regs, op->reg_idx);

Looking at patch 7, you leave the upper half of 64-bit variables
uninitialized here as well as in the memory case further down
when passing in a smaller value for "bytes". A decent static
analyzer should flag this, and I think things also wouldn't work
right in a few cases.

Jan


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

Re: [Xen-devel] [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Andrew Cooper]

On 27/11/17 17:01, Jan Beulich wrote:
 On 26.10.17 at 19:03,  wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -361,6 +361,40 @@ static void reg_write(struct cpu_user_regs *regs,
>>  *pval = value;
>>  }
>>  
>> +static int operand_read(void *buf, struct vmx_inst_op *op,
>> +struct cpu_user_regs *regs, unsigned int bytes)
>> +{
>> +if ( op->type == VMX_INST_MEMREG_TYPE_REG )
>> +{
>> +switch ( bytes )
>> +{
>> +case 4:
>> +*(uint32_t *)buf = reg_read(regs, op->reg_idx);
>> +
>> +case 8:
>> +*(uint64_t *)buf = reg_read(regs, op->reg_idx);
>> +
>> +default:
>> +ASSERT_UNREACHABLE();
>> +return X86EMUL_UNHANDLEABLE;
>> +}
> Looks like there are two missing break statements up there.

I fixed these up x86-next.  (Although it was only because Coverity
noticed and complained at me...)

>
>> +return X86EMUL_OKAY;
> This and ...
>
>> +}
>> +else
>> +{
>> +pagefault_info_t pfinfo;
>> +int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, 
>> );
>> +
>> +if ( rc == HVMTRANS_bad_linear_to_gfn )
>> +hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
>> +if ( rc != HVMTRANS_okay )
>> +return X86EMUL_EXCEPTION;
>> +
>> +return X86EMUL_OKAY;
> ... this should become a uniform ...
>
>> +}
> ... return here.

I tried this, but later patches in the series need them to move back. 
Overall, this layout reduces the code churn across the series.

>
>> @@ -440,7 +474,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>  decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
>>  decode->op[0].reg_idx = info.fields.reg1;
>>  if ( poperandS != NULL )
>> -*poperandS = reg_read(regs, decode->op[0].reg_idx);
>> +{
>> +int rc = operand_read(poperandS, >op[0], regs,
>> +  decode->op[0].len);
>> +if ( rc != X86EMUL_OKAY )
> Blank line between declaration(s) and statement(s) please.

I can fix this.

~Andrew

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

Re: [Xen-devel] [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 26.10.17 at 19:03,  wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -361,6 +361,40 @@ static void reg_write(struct cpu_user_regs *regs,
>  *pval = value;
>  }
>  
> +static int operand_read(void *buf, struct vmx_inst_op *op,
> +struct cpu_user_regs *regs, unsigned int bytes)
> +{
> +if ( op->type == VMX_INST_MEMREG_TYPE_REG )
> +{
> +switch ( bytes )
> +{
> +case 4:
> +*(uint32_t *)buf = reg_read(regs, op->reg_idx);
> +
> +case 8:
> +*(uint64_t *)buf = reg_read(regs, op->reg_idx);
> +
> +default:
> +ASSERT_UNREACHABLE();
> +return X86EMUL_UNHANDLEABLE;
> +}

Looks like there are two missing break statements up there.

> +return X86EMUL_OKAY;

This and ...

> +}
> +else
> +{
> +pagefault_info_t pfinfo;
> +int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, );
> +
> +if ( rc == HVMTRANS_bad_linear_to_gfn )
> +hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
> +if ( rc != HVMTRANS_okay )
> +return X86EMUL_EXCEPTION;
> +
> +return X86EMUL_OKAY;

... this should become a uniform ...

> +}

... return here.

> @@ -440,7 +474,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>  decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
>  decode->op[0].reg_idx = info.fields.reg1;
>  if ( poperandS != NULL )
> -*poperandS = reg_read(regs, decode->op[0].reg_idx);
> +{
> +int rc = operand_read(poperandS, >op[0], regs,
> +  decode->op[0].len);
> +if ( rc != X86EMUL_OKAY )

Blank line between declaration(s) and statement(s) please.

Jan


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