[v1] target/arm/hvf: Consolidate

[PATCH 08/24] target/arm/hvf: Mention hvf_wfi() must run on vCPU thread

Posted by Philippe Mathieu-Daudé 5 months, 1 week ago

Since hvf_wfi() calls hv_vcpu_get_sys_reg(), which
must run on a vCPU, it also must. Mention it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/hvf/hvf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index d87a41bcc53..05fc591b523 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1836,6 +1836,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
     bql_lock();
 }
 
+/* Must be called by the owning thread */
 static void hvf_wfi(CPUState *cpu)
 {
     ARMCPU *arm_cpu = ARM_CPU(cpu);
-- 
2.51.0

Re: [PATCH 08/24] target/arm/hvf: Mention hvf_wfi() must run on vCPU thread

Posted by Mads Ynddal 5 months ago

> On 3 Sep 2025, at 12.06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Since hvf_wfi() calls hv_vcpu_get_sys_reg(), which
> must run on a vCPU, it also must. Mention it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> target/arm/hvf/hvf.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index d87a41bcc53..05fc591b523 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1836,6 +1836,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
>     bql_lock();
> }
> 
> +/* Must be called by the owning thread */
> static void hvf_wfi(CPUState *cpu)
> {
>     ARMCPU *arm_cpu = ARM_CPU(cpu);
> -- 
> 2.51.0

Reviewed-by: Mads Ynddal <mads@ynddal.dk>

Re: [PATCH 08/24] target/arm/hvf: Mention hvf_wfi() must run on vCPU thread

Posted by Richard Henderson 5 months, 1 week ago

On 9/3/25 12:06, Philippe Mathieu-Daudé wrote:
> Since hvf_wfi() calls hv_vcpu_get_sys_reg(), which
> must run on a vCPU, it also must. Mention it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/hvf/hvf.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index d87a41bcc53..05fc591b523 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1836,6 +1836,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
>       bql_lock();
>   }
>   
> +/* Must be called by the owning thread */
>   static void hvf_wfi(CPUState *cpu)
>   {
>       ARMCPU *arm_cpu = ARM_CPU(cpu);

How can it not?  Are all these separate patches and annotations helpful?

r~

Re: [PATCH 08/24] target/arm/hvf: Mention hvf_wfi() must run on vCPU thread

Posted by Mads Ynddal 5 months ago

> On 3 Sep 2025, at 14.34, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 9/3/25 12:06, Philippe Mathieu-Daudé wrote:
>> Since hvf_wfi() calls hv_vcpu_get_sys_reg(), which
>> must run on a vCPU, it also must. Mention it.
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>  target/arm/hvf/hvf.c | 1 +
>>  1 file changed, 1 insertion(+)
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index d87a41bcc53..05fc591b523 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -1836,6 +1836,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
>>      bql_lock();
>>  }
>>  +/* Must be called by the owning thread */
>>  static void hvf_wfi(CPUState *cpu)
>>  {
>>      ARMCPU *arm_cpu = ARM_CPU(cpu);
> 
> How can it not?  Are all these separate patches and annotations helpful?
> 
> r~

It's just part of Apple's API. It's documented here:
https://developer.apple.com/documentation/hypervisor/hv_vcpu_get_sys_reg(_:_:_:)?language=objc

If they are not called in the right CPU context, they'll return an
error. I find it helpful to have it annotated here.

Re: [PATCH 08/24] target/arm/hvf: Mention hvf_wfi() must run on vCPU thread

Posted by Philippe Mathieu-Daudé 5 months, 1 week ago

On 3/9/25 14:34, Richard Henderson wrote:
> On 9/3/25 12:06, Philippe Mathieu-Daudé wrote:
>> Since hvf_wfi() calls hv_vcpu_get_sys_reg(), which
>> must run on a vCPU, it also must. Mention it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/hvf/hvf.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index d87a41bcc53..05fc591b523 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -1836,6 +1836,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, 
>> struct timespec *ts)
>>       bql_lock();
>>   }
>> +/* Must be called by the owning thread */
>>   static void hvf_wfi(CPUState *cpu)
>>   {
>>       ARMCPU *arm_cpu = ARM_CPU(cpu);
> 
> How can it not?  Are all these separate patches and annotations helpful?

Well they helped me understand the locking issue in patch 15 in
hvf_arm_get_host_cpu_features().