Re: [PATCH 12/13] i386: hvf: Move mmio_buf into CPUX86State

[Claudio Fontana <cfontana@suse.de>]

On 6/4/20 8:27 PM, Paolo Bonzini wrote:
> On 28/05/20 21:37, Roman Bolshakov wrote:
>> There's no similar field in CPUX86State, but it's needed for MMIO traps.
>>
>> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> ---
>>  target/i386/cpu.h         |  1 +
>>  target/i386/hvf/hvf.c     |  5 +++++
>>  target/i386/hvf/x86.h     |  1 -
>>  target/i386/hvf/x86_emu.c | 12 ++++++------
>>  4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 7e6566565a..be44e19154 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1593,6 +1593,7 @@ typedef struct CPUX86State {
>>  #endif
>>  #if defined(CONFIG_HVF)
>>      hvf_lazy_flags hvf_lflags;
>> +    void *hvf_mmio_buf;
>>      HVFX86EmulatorState *hvf_emul;
>>  #endif
>>  
>> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>> index 4cee496d71..57696c46c7 100644
>> --- a/target/i386/hvf/hvf.c
>> +++ b/target/i386/hvf/hvf.c
>> @@ -533,7 +533,11 @@ void hvf_reset_vcpu(CPUState *cpu) {
>>  
>>  void hvf_vcpu_destroy(CPUState *cpu)
>>  {
>> +    X86CPU *x86_cpu = X86_CPU(cpu);
>> +    CPUX86State *env = &x86_cpu->env;
>> +
>>      hv_return_t ret = hv_vcpu_destroy((hv_vcpuid_t)cpu->hvf_fd);
>> +    g_free(env->hvf_mmio_buf);
>>      assert_hvf_ok(ret);
>>  }
>>  
>> @@ -563,6 +567,7 @@ int hvf_init_vcpu(CPUState *cpu)
>>      init_decoder();
>>  
>>      hvf_state->hvf_caps = g_new0(struct hvf_vcpu_caps, 1);
>> +    env->hvf_mmio_buf = g_new(char, 4096);
>>      env->hvf_emul = g_new0(HVFX86EmulatorState, 1);
>>  
>>      r = hv_vcpu_create((hv_vcpuid_t *)&cpu->hvf_fd, HV_VCPU_DEFAULT);
>> diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
>> index 2363616c07..483fcea762 100644
>> --- a/target/i386/hvf/x86.h
>> +++ b/target/i386/hvf/x86.h
>> @@ -230,7 +230,6 @@ typedef struct x68_segment_selector {
>>  
>>  /* Definition of hvf_x86_state is here */
>>  struct HVFX86EmulatorState {
>> -    uint8_t mmio_buf[4096];
>>  };
>>  
>>  /* useful register access  macros */
>> diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
>> index 1ad2c30e16..d3e289ed87 100644
>> --- a/target/i386/hvf/x86_emu.c
>> +++ b/target/i386/hvf/x86_emu.c
>> @@ -187,8 +187,8 @@ void write_val_ext(struct CPUX86State *env, target_ulong ptr, target_ulong val,
>>  
>>  uint8_t *read_mmio(struct CPUX86State *env, target_ulong ptr, int bytes)
>>  {
>> -    vmx_read_mem(env_cpu(env), env->hvf_emul->mmio_buf, ptr, bytes);
>> -    return env->hvf_emul->mmio_buf;
>> +    vmx_read_mem(env_cpu(env), env->hvf_mmio_buf, ptr, bytes);
>> +    return env->hvf_mmio_buf;
>>  }
>>  
>>  
>> @@ -489,9 +489,9 @@ static void exec_ins_single(struct CPUX86State *env, struct x86_decode *decode)
>>      target_ulong addr = linear_addr_size(env_cpu(env), RDI(env),
>>                                           decode->addressing_size, R_ES);
>>  
>> -    hvf_handle_io(env_cpu(env), DX(env), env->hvf_emul->mmio_buf, 0,
>> +    hvf_handle_io(env_cpu(env), DX(env), env->hvf_mmio_buf, 0,
>>                    decode->operand_size, 1);
>> -    vmx_write_mem(env_cpu(env), addr, env->hvf_emul->mmio_buf,
>> +    vmx_write_mem(env_cpu(env), addr, env->hvf_mmio_buf,
>>                    decode->operand_size);
>>  
>>      string_increment_reg(env, R_EDI, decode);
>> @@ -512,9 +512,9 @@ static void exec_outs_single(struct CPUX86State *env, struct x86_decode *decode)
>>  {
>>      target_ulong addr = decode_linear_addr(env, decode, RSI(env), R_DS);
>>  
>> -    vmx_read_mem(env_cpu(env), env->hvf_emul->mmio_buf, addr,
>> +    vmx_read_mem(env_cpu(env), env->hvf_mmio_buf, addr,
>>                   decode->operand_size);
>> -    hvf_handle_io(env_cpu(env), DX(env), env->hvf_emul->mmio_buf, 1,
>> +    hvf_handle_io(env_cpu(env), DX(env), env->hvf_mmio_buf, 1,
>>                    decode->operand_size, 1);
>>  
>>      string_increment_reg(env, R_ESI, decode);
>>
> 
> It should be possible to get rid of the buffer altogether, but it's ok
> to do it separately.
> 
> I queued the series, thanks.
> 
> Paolo
> 
> 

Thanks Paolo, I am waiting for this (and maybe another series from Roman) to be able to do the cpus refactoring.

Incidentally, would it not be great to have some machinery that automatically tracks which series is already queued where?
Maybe there is already a mechanism I am unaware of?

Ciao,

Claudio