diff mbox series

[05/13] i386: hvf: Use ins_len to advance IP

Message ID 20200528193758.51454-6-r.bolshakov@yadro.com
State New
Headers show
Series i386: hvf: Remove HVFX86EmulatorState | expand

Commit Message

Roman Bolshakov May 28, 2020, 7:37 p.m. UTC 
There's no need to read VMCS twice, instruction length is already
available in ins_len.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 target/i386/hvf/hvf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé June 4, 2020, 6:39 a.m. UTC | #1 
On 5/28/20 9:37 PM, Roman Bolshakov wrote:
> There's no need to read VMCS twice, instruction length is already
> available in ins_len.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  target/i386/hvf/hvf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index 9ccdb7e7c7..8ff1d25521 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -871,7 +871,7 @@ int hvf_vcpu_exec(CPUState *cpu)
>              } else {
>                  simulate_wrmsr(cpu);
>              }
> -            RIP(env) += rvmcs(cpu->hvf_fd, VMCS_EXIT_INSTRUCTION_LENGTH);
> +            RIP(env) += ins_len;

I'd feel safer if you change ins_len to uint64_t first.

>              store_regs(cpu);
>              break;
>          }
>
Paolo Bonzini June 4, 2020, 6:15 p.m. UTC | #2 
On 04/06/20 08:39, Philippe Mathieu-Daudé wrote:
>>                  simulate_wrmsr(cpu);
>>              }
>> -            RIP(env) += rvmcs(cpu->hvf_fd, VMCS_EXIT_INSTRUCTION_LENGTH);
>> +            RIP(env) += ins_len;
> I'd feel safer if you change ins_len to uint64_t first.
> 

Why?  It will never be more than 15 (it's also a 32-bit field in the VMCS).

Paolo
Philippe Mathieu-Daudé June 5, 2020, 2:29 p.m. UTC | #3 
On 6/4/20 8:15 PM, Paolo Bonzini wrote:
> On 04/06/20 08:39, Philippe Mathieu-Daudé wrote:
>>>                  simulate_wrmsr(cpu);
>>>              }
>>> -            RIP(env) += rvmcs(cpu->hvf_fd, VMCS_EXIT_INSTRUCTION_LENGTH);
>>> +            RIP(env) += ins_len;
>> I'd feel safer if you change ins_len to uint64_t first.
>>
> 
> Why?  It will never be more than 15 (it's also a 32-bit field in the VMCS).

Indeed, I am now seeing the comment in target/i386/hvf/vmcs.h:132

  /* 32-bit read-only data fields */
  #define VMCS_EXIT_INSTRUCTION_LENGTH 0x0000440C

So:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks Paolo.
diff mbox series

Patch

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 9ccdb7e7c7..8ff1d25521 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -871,7 +871,7 @@  int hvf_vcpu_exec(CPUState *cpu)
             } else {
                 simulate_wrmsr(cpu);
             }
-            RIP(env) += rvmcs(cpu->hvf_fd, VMCS_EXIT_INSTRUCTION_LENGTH);
+            RIP(env) += ins_len;
             store_regs(cpu);
             break;
         }