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

[Roman Bolshakov]

On Thu, Jun 11, 2020 at 03:24:31PM +0200, Claudio Fontana wrote:
> 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

Hi Claudio,

I also had the same question earlier today on IRC but I've just recalled
that PULL requests typically have a reference to the queue repo/branch:

https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06825.html

I'll rebase on it and prepare the series.

Regards,
Roman