Re: [PATCH] xen/arm: Do not route NS phys timer IRQ to Xen

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 28/10/2022 11:03, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 28/10/2022 08:56, Michal Orzel wrote:
>> At the moment, we route NS phys timer IRQ to Xen even though it does not
>> make use of this timer. Xen uses hypervisor timer for itself and the
>> physical timer is fully emulated, hence there is nothing that can trigger
>> such IRQ. This means that requesting/releasing IRQ ends up as a deadcode
>> as it has no impact on the functional behavior, whereas the code within
>> a handler ends up being unreachable. This is a left over from the early
>> days when the CNTHP IRQ was buggy on the HW model used for testing and we
>> had to use the CNTP instead.
>>
>> Remove the calls to {request/release}_irq for this timer as well as the
>> code within the handler. Since timer_interrupt handler is now only used
>> by the CNTHP, remove the IRQ affiliation condition. Keep the calls to
>> zero the CNTP_CTL_EL0 register on timer init/deinit for sanity and also 
>> remove
>> the corresponding perf counter definition.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> Based on the outcome of the following discussion:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2Fd55938a3-aaca-1d01-b34f-858dbca9830b%40amd.com%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C4df8dc89b3124eb8f51608dab8c35ab3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638025446431622763%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=qeZR%2BBvOwKA9PKjq2xemSXhJ1Xij%2F%2FMWKADD70vrwW0%3D&amp;reserved=0
>> ---
>>   xen/arch/arm/include/asm/perfc_defn.h |  1 -
>>   xen/arch/arm/time.c                   | 16 +---------------
>>   2 files changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/perfc_defn.h 
>> b/xen/arch/arm/include/asm/perfc_defn.h
>> index 31f071222b24..3ab0391175d7 100644
>> --- a/xen/arch/arm/include/asm/perfc_defn.h
>> +++ b/xen/arch/arm/include/asm/perfc_defn.h
>> @@ -70,7 +70,6 @@ PERFCOUNTER(spis,                 "#SPIs")
>>   PERFCOUNTER(guest_irqs,           "#GUEST-IRQS")
>>
>>   PERFCOUNTER(hyp_timer_irqs,   "Hypervisor timer interrupts")
>> -PERFCOUNTER(phys_timer_irqs,  "Physical timer interrupts")
>>   PERFCOUNTER(virt_timer_irqs,  "Virtual timer interrupts")
>>   PERFCOUNTER(maintenance_irqs, "Maintenance interrupts")
>>
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index dec53b5f7d53..3160fcc7b440 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -222,8 +222,7 @@ int reprogram_timer(s_time_t timeout)
>>   /* Handle the firing timer */
>>   static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs 
>> *regs)
>>   {
>> -    if ( irq == (timer_irq[TIMER_HYP_PPI]) &&
>> -         READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
>> +    if ( READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
> 
> AFAICT, this condition is meant to be true most of the times. So as you
> are modifying the code, could you take the opportunity to add a
> "likely()"? Or better invert the condition so the code below is not
> indented.

Sure thing. I can take the opportunity to do the following:
if ( unlikely(!(READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING)) )
    return;

Also, shouldn't we reflect the purpose of this handler by renaming it
from timer_interrupt to htimer_interrupt (or hyp_timer_interrupt) to be 
consistent
with the naming (i.e. vtimer_interrupt -> virtual, timer_interrupt -> quite 
ambiguous given the usage)?

> 
> Cheers,
> 
> --
> Julien Grall

~Michal