[PATCH v3 3/9] target/arm: add asserts for code paths not leveraged when using the vGIC

Mohamed Mediouni posted 9 patches 3 months, 3 weeks ago
Maintainers: Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Mads Ynddal <mads@ynddal.dk>, Shannon Zhao <shannon.zhaosl@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Alexander Graf <agraf@csgraf.de>
There is a newer version of this series
[PATCH v3 3/9] target/arm: add asserts for code paths not leveraged when using the vGIC
Posted by Mohamed Mediouni 3 months, 3 weeks ago
When using the vGIC, timers are directly handled by the platform, so no vmexits ought to happen in that case.

Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
---
 target/arm/hvf/hvf.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 3ba74b8daa..7beb47caad 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1382,6 +1382,9 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val)
     case SYSREG_ICC_SGI1R_EL1:
     case SYSREG_ICC_SRE_EL1:
     case SYSREG_ICC_CTLR_EL1:
+        if (hvf_irqchip_in_kernel()) {
+            abort();
+        }
         /* Call the TCG sysreg handler. This is only safe for GICv3 regs. */
         if (hvf_sysreg_read_cp(cpu, reg, val)) {
             return 0;
@@ -1702,6 +1705,9 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
     case SYSREG_ICC_SGI0R_EL1:
     case SYSREG_ICC_SGI1R_EL1:
     case SYSREG_ICC_SRE_EL1:
+        if (hvf_irqchip_in_kernel()) {
+            abort();
+        }
         /* Call the TCG sysreg handler. This is only safe for GICv3 regs. */
         if (hvf_sysreg_write_cp(cpu, reg, val)) {
             return 0;
@@ -1965,6 +1971,10 @@ int hvf_vcpu_exec(CPUState *cpu)
         /* This is the main one, handle below. */
         break;
     case HV_EXIT_REASON_VTIMER_ACTIVATED:
+        /* This is only for when a user-mode irqchip is used. */
+        if (hvf_irqchip_in_kernel()) {
+            assert("vtimer activated vmexit when using platform GIC");
+        }
         qemu_set_irq(arm_cpu->gt_timer_outputs[GTIMER_VIRT], 1);
         cpu->accel->vtimer_masked = true;
         return 0;
-- 
2.39.5 (Apple Git-154)
Re: [PATCH v3 3/9] target/arm: add asserts for code paths not leveraged when using the vGIC
Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago
Hi Mohamed,

On 26/7/25 00:30, Mohamed Mediouni wrote:
> When using the vGIC, timers are directly handled by the platform, so no vmexits ought to happen in that case.
> 
> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
> ---
>   target/arm/hvf/hvf.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)


> @@ -1965,6 +1971,10 @@ int hvf_vcpu_exec(CPUState *cpu)
>           /* This is the main one, handle below. */
>           break;
>       case HV_EXIT_REASON_VTIMER_ACTIVATED:
> +        /* This is only for when a user-mode irqchip is used. */
> +        if (hvf_irqchip_in_kernel()) {
> +            assert("vtimer activated vmexit when using platform GIC");

This line does nothing (not firing), is that what you intended to?

> +        }
>           qemu_set_irq(arm_cpu->gt_timer_outputs[GTIMER_VIRT], 1);
>           cpu->accel->vtimer_masked = true;
>           return 0;
Re: [PATCH v3 3/9] target/arm: add asserts for code paths not leveraged when using the vGIC
Posted by Mohamed Mediouni 3 months, 2 weeks ago
Hi,

> On 28. Jul 2025, at 12:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Hi Mohamed,
> 
> On 26/7/25 00:30, Mohamed Mediouni wrote:
>> When using the vGIC, timers are directly handled by the platform, so no vmexits ought to happen in that case.
>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>> ---
>>  target/arm/hvf/hvf.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
> 
> 
>> @@ -1965,6 +1971,10 @@ int hvf_vcpu_exec(CPUState *cpu)
>>          /* This is the main one, handle below. */
>>          break;
>>      case HV_EXIT_REASON_VTIMER_ACTIVATED:
>> +        /* This is only for when a user-mode irqchip is used. */
>> +        if (hvf_irqchip_in_kernel()) {
>> +            assert("vtimer activated vmexit when using platform GIC");
> 
> This line does nothing (not firing), is that what you intended to?
It’s specifically so that if this trips, I know that I really screwed things up. Helped me a bit in developing this.

However, this whole patch isn’t expected to ever trigger irl, so would be fine to drop from that perspective.

HV_EXIT_REASON_VTIMER_ACTIVATED will never be returned by Hypervisor.framework when the vGIC is enabled.

Thank you,
-Mohamed
>> +        }
>>          qemu_set_irq(arm_cpu->gt_timer_outputs[GTIMER_VIRT], 1);
>>          cpu->accel->vtimer_masked = true;
>>          return 0;
> 
Re: [PATCH v3 3/9] target/arm: add asserts for code paths not leveraged when using the vGIC
Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago
On 28/7/25 12:46, Mohamed Mediouni wrote:
> Hi,
> 
>> On 28. Jul 2025, at 12:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Mohamed,
>>
>> On 26/7/25 00:30, Mohamed Mediouni wrote:
>>> When using the vGIC, timers are directly handled by the platform, so no vmexits ought to happen in that case.
>>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>>> ---
>>>   target/arm/hvf/hvf.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>
>>
>>> @@ -1965,6 +1971,10 @@ int hvf_vcpu_exec(CPUState *cpu)
>>>           /* This is the main one, handle below. */
>>>           break;
>>>       case HV_EXIT_REASON_VTIMER_ACTIVATED:
>>> +        /* This is only for when a user-mode irqchip is used. */
>>> +        if (hvf_irqchip_in_kernel()) {
>>> +            assert("vtimer activated vmexit when using platform GIC");
>>
>> This line does nothing (not firing), is that what you intended to?
> It’s specifically so that if this trips, I know that I really screwed things up. Helped me a bit in developing this.

Sorry I misread.

Better to follow QEMU style to ease reviewers:

   error_report("vtimer activated vmexit when using platform GIC");
   abort();

Or just:

   assert(!hvf_irqchip_in_kernel());

> 
> However, this whole patch isn’t expected to ever trigger irl, so would be fine to drop from that perspective.
> 
> HV_EXIT_REASON_VTIMER_ACTIVATED will never be returned by Hypervisor.framework when the vGIC is enabled.
> 
> Thank you,
> -Mohamed
>>> +        }
>>>           qemu_set_irq(arm_cpu->gt_timer_outputs[GTIMER_VIRT], 1);
>>>           cpu->accel->vtimer_masked = true;
>>>           return 0;
>>
>