Re: [PATCH v6 12/16] cpus: add handle_interrupt to the CpusAccel interface

[Claudio Fontana]

On 9/1/20 11:38 AM, Roman Bolshakov wrote:
> On Tue, Sep 01, 2020 at 09:21:57AM +0200, Claudio Fontana wrote:
>> kvm: uses the generic handler
>> qtest: uses the generic handler
>> whpx: changed to use the generic handler (identical implementation)
>> hax: changed to use the generic handler (identical implementation)
>> hvf: changed to use the generic handler (identical implementation)
>> tcg: adapt tcg-cpus to point to the tcg-specific handler
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  accel/tcg/tcg-all.c    | 26 --------------------------
>>  accel/tcg/tcg-cpus.c   | 28 ++++++++++++++++++++++++++++
>>  hw/core/cpu.c          | 13 -------------
>>  include/hw/core/cpu.h  | 14 --------------
>>  include/sysemu/cpus.h  |  2 ++
>>  softmmu/cpus.c         | 18 ++++++++++++++++++
>>  target/i386/hax-all.c  | 10 ----------
>>  target/i386/hvf/hvf.c  |  9 ---------
>>  target/i386/whpx-all.c | 10 ----------
>>  9 files changed, 48 insertions(+), 82 deletions(-)
>>
>> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
>> index 01957b130d..af9bf5c5bb 100644
>> --- a/accel/tcg/tcg-all.c
>> +++ b/accel/tcg/tcg-all.c
>> @@ -47,31 +47,6 @@ typedef struct TCGState {
>>  #define TCG_STATE(obj) \
>>          OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
>>  
>> -/* mask must never be zero, except for A20 change call */
>> -static void tcg_handle_interrupt(CPUState *cpu, int mask)
>> -{
>> -    int old_mask;
>> -    g_assert(qemu_mutex_iothread_locked());
>> -
>> -    old_mask = cpu->interrupt_request;
>> -    cpu->interrupt_request |= mask;
>> -
>> -    /*
>> -     * If called from iothread context, wake the target cpu in
>> -     * case its halted.
>> -     */
>> -    if (!qemu_cpu_is_self(cpu)) {
>> -        qemu_cpu_kick(cpu);
>> -    } else {
>> -        atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
>> -        if (icount_enabled() &&
>> -            !cpu->can_do_io
>> -            && (mask & ~old_mask) != 0) {
>> -            cpu_abort(cpu, "Raised interrupt while not in I/O function");
>> -        }
>> -    }
>> -}
>> -
>>  /*
>>   * We default to false if we know other options have been enabled
>>   * which are currently incompatible with MTTCG. Otherwise when each
>> @@ -128,7 +103,6 @@ static int tcg_init(MachineState *ms)
>>      TCGState *s = TCG_STATE(current_accel());
>>  
>>      tcg_exec_init(s->tb_size * 1024 * 1024);
>> -    cpu_interrupt_handler = tcg_handle_interrupt;
>>      mttcg_enabled = s->mttcg_enabled;
>>      cpus_register_accel(&tcg_cpus);
>>  
>> diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c
>> index 72696f6d86..2bb209e2c6 100644
>> --- a/accel/tcg/tcg-cpus.c
>> +++ b/accel/tcg/tcg-cpus.c
>> @@ -533,9 +533,37 @@ static int64_t tcg_get_elapsed_ticks(void)
>>      return cpu_get_ticks();
>>  }
>>  
>> +/* mask must never be zero, except for A20 change call */
>> +static void tcg_handle_interrupt(CPUState *cpu, int mask)
>> +{
>> +    int old_mask;
>> +    g_assert(qemu_mutex_iothread_locked());
>> +
>> +    old_mask = cpu->interrupt_request;
>> +    cpu->interrupt_request |= mask;
>> +
>> +    /*
>> +     * If called from iothread context, wake the target cpu in
>> +     * case its halted.
>> +     */
>> +    if (!qemu_cpu_is_self(cpu)) {
>> +        qemu_cpu_kick(cpu);
>> +    } else {
>> +        atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
>> +        if (icount_enabled() &&
>> +            !cpu->can_do_io
>> +            && (mask & ~old_mask) != 0) {
>> +            cpu_abort(cpu, "Raised interrupt while not in I/O function");
>> +        }
>> +    }
>> +}
>> +
>>  const CpusAccel tcg_cpus = {
>>      .create_vcpu_thread = tcg_start_vcpu_thread,
>>      .kick_vcpu_thread = tcg_kick_vcpu_thread,
>> +
>> +    .handle_interrupt = tcg_handle_interrupt,
>> +
>>      .get_virtual_clock = tcg_get_virtual_clock,
>>      .get_elapsed_ticks = tcg_get_elapsed_ticks,
>>  };
>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
>> index fa8602493b..451b3d5ee7 100644
>> --- a/hw/core/cpu.c
>> +++ b/hw/core/cpu.c
>> @@ -35,8 +35,6 @@
>>  #include "qemu/plugin.h"
>>  #include "sysemu/hw_accel.h"
>>  
>> -CPUInterruptHandler cpu_interrupt_handler;
>> -
>>  CPUState *cpu_by_arch_id(int64_t id)
>>  {
>>      CPUState *cpu;
>> @@ -394,17 +392,6 @@ static vaddr cpu_adjust_watchpoint_address(CPUState 
>> *cpu, vaddr addr, int len)
>>      return addr;
>>  }
>>  
>> -static void generic_handle_interrupt(CPUState *cpu, int mask)
>> -{
>> -    cpu->interrupt_request |= mask;
>> -
>> -    if (!qemu_cpu_is_self(cpu)) {
>> -        qemu_cpu_kick(cpu);
>> -    }
>> -}
>> -
>> -CPUInterruptHandler cpu_interrupt_handler = generic_handle_interrupt;
>> -
>>  static void cpu_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 8f145733ce..efd33d87fd 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -838,12 +838,6 @@ bool cpu_exists(int64_t id);
>>   */
>>  CPUState *cpu_by_arch_id(int64_t id);
>>  
>> -#ifndef CONFIG_USER_ONLY
>> -
>> -typedef void (*CPUInterruptHandler)(CPUState *, int);
>> -
>> -extern CPUInterruptHandler cpu_interrupt_handler;
>> -
>>  /**
>>   * cpu_interrupt:
>>   * @cpu: The CPU to set an interrupt on.
>> @@ -851,17 +845,9 @@ extern CPUInterruptHandler cpu_interrupt_handler;
>>   *
>>   * Invokes the interrupt handler.
>>   */
>> -static inline void cpu_interrupt(CPUState *cpu, int mask)
>> -{
>> -    cpu_interrupt_handler(cpu, mask);
>> -}
>> -
>> -#else /* USER_ONLY */
>>  
>>  void cpu_interrupt(CPUState *cpu, int mask);
>>  
>> -#endif /* USER_ONLY */
>> -
>>  #ifdef NEED_CPU_H
>>  
>>  #ifdef CONFIG_SOFTMMU
>> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
>> index 26171697f5..231685955d 100644
>> --- a/include/sysemu/cpus.h
>> +++ b/include/sysemu/cpus.h
>> @@ -16,6 +16,8 @@ typedef struct CpusAccel {
>>      void (*synchronize_state)(CPUState *cpu);
>>      void (*synchronize_pre_loadvm)(CPUState *cpu);
>>  
>> +    void (*handle_interrupt)(CPUState *cpu, int mask);
>> +
>>      int64_t (*get_virtual_clock)(void);
>>      int64_t (*get_elapsed_ticks)(void);
>>  } CpusAccel;
>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>> index f32ecb4bb9..7068666579 100644
>> --- a/softmmu/cpus.c
>> +++ b/softmmu/cpus.c
>> @@ -225,6 +225,24 @@ int64_t cpus_get_elapsed_ticks(void)
>>      return cpu_get_ticks();
>>  }
>>  
>> +static void generic_handle_interrupt(CPUState *cpu, int mask)
>> +{
>> +    cpu->interrupt_request |= mask;
>> +
>> +    if (!qemu_cpu_is_self(cpu)) {
>> +        qemu_cpu_kick(cpu);
>> +    }
>> +}
>> +
>> +void cpu_interrupt(CPUState *cpu, int mask)
>> +{
>> +    if (cpus_accel->handle_interrupt) {
>> +        cpus_accel->handle_interrupt(cpu, mask);
>> +    } else {
>> +        generic_handle_interrupt(cpu, mask);
>> +    }
>> +}
>> +
> 
> The handlers is not something that dynamically changes, the functions
> can be assigned once during accel registration.

Data struct is now const though, they are never assigned, only initialized..

>  Accel registraton is
> also the place where the checks can take place.
> 
> Regards,
> Roman
> 
>>  static int do_vm_stop(RunState state, bool send_stop)
>>  {
>>      int ret = 0;
>> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
>> index b66ddeb8bf..fd1ab673d7 100644
>> --- a/target/i386/hax-all.c
>> +++ b/target/i386/hax-all.c
>> @@ -297,15 +297,6 @@ int hax_vm_destroy(struct hax_vm *vm)
>>      return 0;
>>  }
>>  
>> -static void hax_handle_interrupt(CPUState *cpu, int mask)
>> -{
>> -    cpu->interrupt_request |= mask;
>> -
>> -    if (!qemu_cpu_is_self(cpu)) {
>> -        qemu_cpu_kick(cpu);
>> -    }
>> -}
>> -
>>  static int hax_init(ram_addr_t ram_size, int max_cpus)
>>  {
>>      struct hax_state *hax = NULL;
>> @@ -350,7 +341,6 @@ static int hax_init(ram_addr_t ram_size, int max_cpus)
>>      qversion.cur_version = hax_cur_version;
>>      qversion.min_version = hax_min_version;
>>      hax_notify_qemu_version(hax->vm->fd, &qversion);
>> -    cpu_interrupt_handler = hax_handle_interrupt;
>>  
>>      return ret;
>>    error:
>> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>> index 7ac6987c1b..ed9356565c 100644
>> --- a/target/i386/hvf/hvf.c
>> +++ b/target/i386/hvf/hvf.c
>> @@ -262,14 +262,6 @@ static void update_apic_tpr(CPUState *cpu)
>>  
>>  #define VECTORING_INFO_VECTOR_MASK     0xff
>>  
>> -static void hvf_handle_interrupt(CPUState * cpu, int mask)
>> -{
>> -    cpu->interrupt_request |= mask;
>> -    if (!qemu_cpu_is_self(cpu)) {
>> -        qemu_cpu_kick(cpu);
>> -    }
>> -}
>> -
>>  void hvf_handle_io(CPUArchState *env, uint16_t port, void *buffer,
>>                    int direction, int size, int count)
>>  {
>> @@ -894,7 +886,6 @@ static int hvf_accel_init(MachineState *ms)
>>      }
>>    
>>      hvf_state = s;
>> -    cpu_interrupt_handler = hvf_handle_interrupt;
>>      memory_listener_register(&hvf_memory_listener, &address_space_memory);
>>      cpus_register_accel(&hvf_cpus);
>>      return 0;
>> diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
>> index 8b6986c864..b3d17fbe04 100644
>> --- a/target/i386/whpx-all.c
>> +++ b/target/i386/whpx-all.c
>> @@ -1413,15 +1413,6 @@ static void whpx_memory_init(void)
>>      memory_listener_register(&whpx_memory_listener, &address_space_memory);
>>  }
>>  
>> -static void whpx_handle_interrupt(CPUState *cpu, int mask)
>> -{
>> -    cpu->interrupt_request |= mask;
>> -
>> -    if (!qemu_cpu_is_self(cpu)) {
>> -        qemu_cpu_kick(cpu);
>> -    }
>> -}
>> -
>>  /*
>>   * Load the functions from the given library, using the given handle. If a
>>   * handle is provided, it is used, otherwise the library is opened. The
>> @@ -1576,7 +1567,6 @@ static int whpx_accel_init(MachineState *ms)
>>  
>>      whpx_memory_init();
>>  
>> -    cpu_interrupt_handler = whpx_handle_interrupt;
>>      cpus_register_accel(&whpx_cpus);
>>  
>>      printf("Windows Hypervisor Platform accelerator is operational\n");
>> -- 
>> 2.26.2
>>