Re: [PATCH v5 10/14] cpus: add handle_interrupt to the CpusAccel interface

[Claudio Fontana]

On 8/14/20 11:01 PM, Richard Henderson wrote:
> On 8/12/20 11:32 AM, Claudio Fontana wrote:
>> +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 && cpus_accel->handle_interrupt) {
>> +        cpus_accel->handle_interrupt(cpu, mask);
>> +    } else {
>> +        generic_handle_interrupt(cpu, mask);
>> +    }
>> +}
> 
> First, by this point you have converted all of the accelerators, so I would
> expect cpus_accel to always be non-null.  I would expect a patch immediately
> preceding this one to place an assert to that effect somewhere in the startup
> code, and to remove all of the checks.

Ok.

> 
> Second, I would prefer that all methods be non-null, so that you don't need to
> check that either.  

This contrasts a bit with the previous comments by Paolo to try to express only 
the "interesting" bits in the CpusAccel struct in each module.


> This patch would add generic_handle_interrupt (perhaps
> named cpus_accel_default_handle_interrupt declared in sysemu/cpus.h?) to the
> CpusAccel structure of all except TCG.
> 
> Similarly for all other methods that are checking non-null-ness of the method
> pointer.  Perhaps assert non-null for each method in cpus_register_accel().
> 
> 
> r~
> 

That was my initial approach in very old versions of the series (all fields as 
explicit and mandatory),
I changed direction there due to Paolo's comments. For me both solutions are 
fine, each has its merits and downsides.

Thanks,

Claudio