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.or...@amd.com>
>> ---
>> 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&data=05%7C01%7Cmichal.orzel%40amd.com%7C4df8dc89b3124eb8f51608dab8c35ab3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638025446431622763%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=qeZR%2BBvOwKA9PKjq2xemSXhJ1Xij%2F%2FMWKADD70vrwW0%3D&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
