vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
CONFIG_NEW_VGIC is not set.

enter_hypervisor_from_guest is protecting calls to these functions
with CONFIG_NEW_VGIC but their definitions and declarations are not
protected. This means that we are including them in the image even
though we are not making use of them. Fix that.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/domain.c        | 2 ++
 xen/arch/arm/vtimer.c        | 2 ++
 xen/include/asm-arm/event.h  | 2 ++
 xen/include/asm-arm/vtimer.h | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 96e1b23550..7baa2b7417 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1108,12 +1108,14 @@ void vcpu_mark_events_pending(struct vcpu *v)
     vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, true);
 }
 
+#ifdef CONFIG_NEW_VGIC
 void vcpu_update_evtchn_irq(struct vcpu *v)
 {
     bool pending = vcpu_info(v, evtchn_upcall_pending);
 
     vgic_inject_irq(v->domain, v, v->domain->arch.evtchn_irq, pending);
 }
+#endif
 
 /* The ARM spec declares that even if local irqs are masked in
  * the CPSR register, an irq should wake up a cpu from WFI anyway.
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 0196951af4..63a8374f7d 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -347,6 +347,7 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
     }
 }
 
+#ifdef CONFIG_NEW_VGIC
 static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
                               register_t vtimer_ctl)
 {
@@ -395,6 +396,7 @@ void vtimer_update_irqs(struct vcpu *v)
     /* For the physical timer we rely on our emulated state. */
     vtimer_update_irq(v, &v->arch.phys_timer, v->arch.phys_timer.ctl);
 }
+#endif /* CONFIG_NEW_VGIC */
 
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index b14c166ad6..f4193cb62e 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -5,7 +5,9 @@
 
 void vcpu_kick(struct vcpu *v);
 void vcpu_mark_events_pending(struct vcpu *v);
+#ifdef CONFIG_NEW_VGIC
 void vcpu_update_evtchn_irq(struct vcpu *v);
+#endif
 void vcpu_block_unless_event_pending(struct vcpu *v);
 
 static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h
index 9d4fb4c6e8..1e945ae2c5 100644
--- a/xen/include/asm-arm/vtimer.h
+++ b/xen/include/asm-arm/vtimer.h
@@ -27,7 +27,9 @@ extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
 extern void virt_timer_save(struct vcpu *v);
 extern void virt_timer_restore(struct vcpu *v);
 extern void vcpu_timer_destroy(struct vcpu *v);
+#ifdef CONFIG_NEW_VGIC
 void vtimer_update_irqs(struct vcpu *v);
+#endif
 
 #endif
 
-- 
2.29.0

Hi Michal,

On 06/12/2021 14:19, Michal Orzel wrote:
> vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
> CONFIG_NEW_VGIC is not set.
> 
> enter_hypervisor_from_guest is protecting calls to these functions
> with CONFIG_NEW_VGIC but their definitions and declarations are not > protected. This means that we are including them in the image even
> though we are not making use of them. Fix that.

While I agree, they are only used by the new vGIC, the implementation of 
the functions are not. So I don't think they should be protected by 
CONFIG_NEW_VGIC.

Actually, I am not convinced they should be protected. But I guess you 
did that for a reason. Would you be able to clarify what is your reason?

Cheers,

-- 
Julien Grall

Hi Julien,

On 06.12.2021 15:39, Julien Grall wrote:
> Hi Michal,
> 
> On 06/12/2021 14:19, Michal Orzel wrote:
>> vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
>> CONFIG_NEW_VGIC is not set.
>>
>> enter_hypervisor_from_guest is protecting calls to these functions
>> with CONFIG_NEW_VGIC but their definitions and declarations are not > protected. This means that we are including them in the image even
>> though we are not making use of them. Fix that.
> 
> While I agree, they are only used by the new vGIC, the implementation of the functions are not. So I don't think they should be protected by CONFIG_NEW_VGIC.
> 
> Actually, I am not convinced they should be protected. But I guess you did that for a reason. Would you be able to clarify what is your reason?
> 
From what I know + what the commit introducing these fucntions states (b9db96f71a74), the current vGIC does not handle level-triggered vIRQs.
The functionality of these functions is only related to new VGIC implementation which can handle level triggered vIRQs.
So I do not think that these functions are generic and thus I believe they should be protected.
> Cheers,
> 

Cheers,
Michal

On 06/12/2021 15:00, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 06.12.2021 15:39, Julien Grall wrote:
>> Hi Michal,
>>
>> On 06/12/2021 14:19, Michal Orzel wrote:
>>> vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
>>> CONFIG_NEW_VGIC is not set.
>>>
>>> enter_hypervisor_from_guest is protecting calls to these functions
>>> with CONFIG_NEW_VGIC but their definitions and declarations are not > protected. This means that we are including them in the image even
>>> though we are not making use of them. Fix that.
>>
>> While I agree, they are only used by the new vGIC, the implementation of the functions are not. So I don't think they should be protected by CONFIG_NEW_VGIC.
>>
>> Actually, I am not convinced they should be protected. But I guess you did that for a reason. Would you be able to clarify what is your reason?
>>
>  From what I know + what the commit introducing these fucntions states (b9db96f71a74), the current vGIC does not handle level-triggered vIRQs.
> The functionality of these functions is only related to new VGIC implementation which can handle level triggered vIRQs.

This is a known error in the vGIC implementation which should be 
resolved before this leads to a disaster.

> So I do not think that these functions are generic and thus I believe they should be protected.

None of the functions rely on the internal of the new vGIC. In fact, as 
I wrote above, the current vGIC ought to be able to handle level-trigger 
interrupt properly.

They are not called for the current vGIC because there was concern about 
the performance impact on each trap (see [1]).

So I think those functions ought to stay compiled in for everyone.

Cheers,

[1] 
https://lore.kernel.org/xen-devel/22601816-8235-7891-b634-4af5348a1337@arm.com/



>> Cheers,
>>
> 
> Cheers,
> Michal
> 

-- 
Julien Grall

Hi Julien,

On 06.12.2021 17:40, Julien Grall wrote:
> 
> 
> On 06/12/2021 15:00, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 06.12.2021 15:39, Julien Grall wrote:
>>> Hi Michal,
>>>
>>> On 06/12/2021 14:19, Michal Orzel wrote:
>>>> vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
>>>> CONFIG_NEW_VGIC is not set.
>>>>
>>>> enter_hypervisor_from_guest is protecting calls to these functions
>>>> with CONFIG_NEW_VGIC but their definitions and declarations are not > protected. This means that we are including them in the image even
>>>> though we are not making use of them. Fix that.
>>>
>>> While I agree, they are only used by the new vGIC, the implementation of the functions are not. So I don't think they should be protected by CONFIG_NEW_VGIC.
>>>
>>> Actually, I am not convinced they should be protected. But I guess you did that for a reason. Would you be able to clarify what is your reason?
>>>
>>  From what I know + what the commit introducing these fucntions states (b9db96f71a74), the current vGIC does not handle level-triggered vIRQs.
>> The functionality of these functions is only related to new VGIC implementation which can handle level triggered vIRQs.
> 
> This is a known error in the vGIC implementation which should be resolved before this leads to a disaster.

I just thought that if this error is present for such a long time, there are no plans to make current vgic handle level type irqs.
> 
>> So I do not think that these functions are generic and thus I believe they should be protected.
> 
> None of the functions rely on the internal of the new vGIC. In fact, as I wrote above, the current vGIC ought to be able to handle level-trigger interrupt properly.
> 
> They are not called for the current vGIC because there was concern about the performance impact on each trap (see [1]).
> 
> So I think those functions ought to stay compiled in for everyone.
> 
I'm totally ok with that.

> Cheers,
> 
> [1] https://lore.kernel.org/xen-devel/22601816-8235-7891-b634-4af5348a1337@arm.com/
> 
> 
> 
>>> Cheers,
>>>
>>
>> Cheers,
>> Michal
>>
> 

Cheers,
Michal

Hi,

On 07/12/2021 07:10, Michal Orzel wrote:
> On 06.12.2021 17:40, Julien Grall wrote:
>>
>>
>> On 06/12/2021 15:00, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>> On 06.12.2021 15:39, Julien Grall wrote:
>>>> Hi Michal,
>>>>
>>>> On 06/12/2021 14:19, Michal Orzel wrote:
>>>>> vtimer_update_irqs, vtimer_update_irq and vcpu_update_evtchn_irq if
>>>>> CONFIG_NEW_VGIC is not set.
>>>>>
>>>>> enter_hypervisor_from_guest is protecting calls to these functions
>>>>> with CONFIG_NEW_VGIC but their definitions and declarations are not > protected. This means that we are including them in the image even
>>>>> though we are not making use of them. Fix that.
>>>>
>>>> While I agree, they are only used by the new vGIC, the implementation of the functions are not. So I don't think they should be protected by CONFIG_NEW_VGIC.
>>>>
>>>> Actually, I am not convinced they should be protected. But I guess you did that for a reason. Would you be able to clarify what is your reason?
>>>>
>>>   From what I know + what the commit introducing these fucntions states (b9db96f71a74), the current vGIC does not handle level-triggered vIRQs.
>>> The functionality of these functions is only related to new VGIC implementation which can handle level triggered vIRQs.
>>
>> This is a known error in the vGIC implementation which should be resolved before this leads to a disaster.
> 
> I just thought that if this error is present for such a long time, there are no plans to make current vgic handle level type irqs.

The error has been present for such a long time because no-one yet 
volunteered to properly fix it. That said, the new vGIC has been an 
attempt to resolve it but the effort kind of died.

It would be nice to revive the effort.

Cheers,

-- 
Julien Grall