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

[Julien Grall <julien@xxxxxxx>]

On 28/10/2022 10:36, Michal Orzel wrote:
> 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)?
I am fine with that.

Cheers,

--
Julien Grall