Re: [PATCH v2 05/12] x86/hvm: Remove duplicate calls caused by tracing

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/2021 13:18, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> 1) vpic_ack_pending_irq() calls vlapic_accept_pic_intr() twice, once in the
>>    TRACE_2D() instantiation and once "for real".  Make the call only once.
>>
>> 2) vlapic_accept_pic_intr() similarly calls __vlapic_accept_pic_intr() twice,
>>    although this is more complicated to disentangle.
>>
>>    v cannot be NULL because it has already been dereferenced in the function,
>>    causing the ternary expression to always call __vlapic_accept_pic_intr().
>>    However, the return expression of the function takes care to skip the call
>>    if this vCPU isn't the PIC target.  As __vlapic_accept_pic_intr() is far
>>    from trivial, make the TRACE_2D() semantics match the return semantics by
>>    only calling __vlapic_accept_pic_intr() when the vCPU is the PIC target.
>>
>> 3) hpet_set_timer() duplicates calls to hpet_tick_to_ns().  Pull the logic 
>> out
>>    which simplifies both the TRACE and create_periodic_time() calls.
>>
>> 4) lapic_rearm() makes multiple calls to vlapic_lvtt_period().  Pull it out
>>    into a local variable.
>>
>> vlapic_accept_pic_intr() is called on every VMEntry, so this is a reduction 
>> in
>> VMEntry complexity across the board.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>
>> --- a/xen/arch/x86/hvm/vpic.c
>> +++ b/xen/arch/x86/hvm/vpic.c
>> @@ -512,14 +512,15 @@ void vpic_irq_negative_edge(struct domain *d, int irq)
>>  
>>  int vpic_ack_pending_irq(struct vcpu *v)
>>  {
>> -    int irq;
>> +    int irq, accept;
> Strictly speaking "accept" wants to be bool, and ...
>
>>      struct hvm_hw_vpic *vpic = &v->domain->arch.hvm.vpic[0];
>>  
>>      ASSERT(has_vpic(v->domain));
>>  
>> -    TRACE_2D(TRC_HVM_EMUL_PIC_PEND_IRQ_CALL, vlapic_accept_pic_intr(v),
>> -             vpic->int_output);
>> -    if ( !vlapic_accept_pic_intr(v) || !vpic->int_output )
>> +    accept = vlapic_accept_pic_intr(v);
> ... vlapic_accept_pic_intr() would eventually also want to be
> converted to return bool.

Yeah, but given the potential for backport here, I specifically avoided
unrelated changed.  We've got loads of functions wanting to change to bool.

~Andrew