1. Patchew
  2. Xen
  3. View series

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

Michal Orzel posted 1 patch 1 year, 6 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20221028075630.32261-1-michal.orzel@amd.com
There is a newer version of this series
xen/arch/arm/include/asm/perfc_defn.h |  1 -
xen/arch/arm/time.c                   | 16 +---------------
2 files changed, 1 insertion(+), 16 deletions(-)
[PATCH] xen/arm: Do not route NS phys timer IRQ to Xen
Posted by Michal Orzel 1 year, 6 months ago 
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@amd.com>
---
Based on the outcome of the following discussion:
https://lore.kernel.org/xen-devel/d55938a3-aaca-1d01-b34f-858dbca9830b@amd.com/
---
 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 )
     {
         perfc_incr(hyp_timer_irqs);
         /* Signal the generic timer code to do its work */
@@ -231,16 +230,6 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
         /* Disable the timer to avoid more interrupts */
         WRITE_SYSREG(0, CNTHP_CTL_EL2);
     }
-
-    if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) &&
-         READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
-    {
-        perfc_incr(phys_timer_irqs);
-        /* Signal the generic timer code to do its work */
-        raise_softirq(TIMER_SOFTIRQ);
-        /* Disable the timer to avoid more interrupts */
-        WRITE_SYSREG(0, CNTP_CTL_EL0);
-    }
 }
 
 static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
@@ -306,8 +295,6 @@ void init_timer_interrupt(void)
                 "hyptimer", NULL);
     request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
                    "virtimer", NULL);
-    request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
-                "phytimer", NULL);
 
     check_timer_irq_cfg(timer_irq[TIMER_HYP_PPI], "hypervisor");
     check_timer_irq_cfg(timer_irq[TIMER_VIRT_PPI], "virtual");
@@ -326,7 +313,6 @@ static void deinit_timer_interrupt(void)
 
     release_irq(timer_irq[TIMER_HYP_PPI], NULL);
     release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
-    release_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], NULL);
 }
 
 /* Wait a set number of microseconds */
-- 
2.25.1
Re: [PATCH] xen/arm: Do not route NS phys timer IRQ to Xen
Posted by Julien Grall 1 year, 6 months ago 
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@amd.com>
> ---
> Based on the outcome of the following discussion:
> https://lore.kernel.org/xen-devel/d55938a3-aaca-1d01-b34f-858dbca9830b@amd.com/
> ---
>   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.

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm: Do not route NS phys timer IRQ to Xen
Posted by Michal Orzel 1 year, 6 months ago 
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@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&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
Re: [PATCH] xen/arm: Do not route NS phys timer IRQ to Xen
Posted by Julien Grall 1 year, 6 months ago 

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@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&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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.