| Message ID | 20200528193758.51454-13-r.bolshakov@yadro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386: hvf: Remove HVFX86EmulatorState | expand |
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
On Thu, Jun 04, 2020 at 08:27:37PM +0200, 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. > > > > It should be possible to get rid of the buffer altogether, but it's ok > to do it separately. > Hi Paolo, I will look into that. Thanks, Roman
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
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
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);
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(-)