[PATCH for-11.0 2/2] i386/hyperv: add stubs for synic enablement

Ani Sinha posted 2 patches 2 weeks, 4 days ago
Maintainers: "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Marcelo Tosatti <mtosatti@redhat.com>
There is a newer version of this series
[PATCH for-11.0 2/2] i386/hyperv: add stubs for synic enablement
Posted by Ani Sinha 2 weeks, 4 days ago
Add a new call hyperv_enable_synic() that can be called whether or not
CONFIG_HYPERV is enabled. This way genetic code in i396/kvm.c can call this
function to enable synic for hyperv. For non-hyperv cases, the stub will
be a noop.

Reported-by: Michale Tokarev <mjt@tls.msk.ru>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 target/i386/kvm/hyperv-stub.c |  5 +++++
 target/i386/kvm/hyperv.c      |  9 +++++++++
 target/i386/kvm/hyperv.h      |  1 +
 target/i386/kvm/kvm.c         | 12 +++++-------
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/target/i386/kvm/hyperv-stub.c b/target/i386/kvm/hyperv-stub.c
index 5836f53c23..767a4c7e1a 100644
--- a/target/i386/kvm/hyperv-stub.c
+++ b/target/i386/kvm/hyperv-stub.c
@@ -61,3 +61,8 @@ uint64_t hyperv_syndbg_query_options(void)
 {
     return 0;
 }
+
+int hyperv_enable_synic(X86CPU *cpu)
+{
+    return 0;
+}
diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c
index f7a81bd270..bd3c26d02b 100644
--- a/target/i386/kvm/hyperv.c
+++ b/target/i386/kvm/hyperv.c
@@ -24,6 +24,15 @@ int hyperv_x86_synic_add(X86CPU *cpu)
     return 0;
 }
 
+int hyperv_enable_synic(X86CPU *cpu)
+{
+    int ret = 0;
+    if (!hyperv_is_synic_enabled()) {
+        ret = hyperv_x86_synic_add(cpu);
+    }
+    return ret;
+}
+
 /*
  * All devices possibly using SynIC have to be reset before calling this to let
  * them remove their SINT routes first.
diff --git a/target/i386/kvm/hyperv.h b/target/i386/kvm/hyperv.h
index e45a4512fe..a393a5d428 100644
--- a/target/i386/kvm/hyperv.h
+++ b/target/i386/kvm/hyperv.h
@@ -23,6 +23,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
 #endif
 
 int hyperv_x86_synic_add(X86CPU *cpu);
+int hyperv_enable_synic(X86CPU *cpu);
 void hyperv_x86_synic_reset(X86CPU *cpu);
 void hyperv_x86_synic_update(X86CPU *cpu);
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index a29f757c16..9e352882c8 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1754,13 +1754,11 @@ static int hyperv_init_vcpu(X86CPU *cpu)
             return ret;
         }
 
-        if (!hyperv_is_synic_enabled()) {
-            ret = hyperv_x86_synic_add(cpu);
-            if (ret < 0) {
-                error_report("failed to create HyperV SynIC: %s",
-                             strerror(-ret));
-                return ret;
-            }
+        ret = hyperv_enable_synic(cpu);
+        if (ret < 0) {
+            error_report("failed to create HyperV SynIC: %s",
+                         strerror(-ret));
+            return ret;
         }
     }
 
-- 
2.42.0
Re: [PATCH for-11.0 2/2] i386/hyperv: add stubs for synic enablement
Posted by Philippe Mathieu-Daudé 2 weeks, 3 days ago
Hi Ani,

On 19/3/26 11:11, Ani Sinha wrote:
> Add a new call hyperv_enable_synic() that can be called whether or not
> CONFIG_HYPERV is enabled. This way genetic code in i396/kvm.c can call this
> function to enable synic for hyperv. For non-hyperv cases, the stub will
> be a noop.
> 
> Reported-by: Michale Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>   target/i386/kvm/hyperv-stub.c |  5 +++++
>   target/i386/kvm/hyperv.c      |  9 +++++++++
>   target/i386/kvm/hyperv.h      |  1 +
>   target/i386/kvm/kvm.c         | 12 +++++-------
>   4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/target/i386/kvm/hyperv-stub.c b/target/i386/kvm/hyperv-stub.c
> index 5836f53c23..767a4c7e1a 100644
> --- a/target/i386/kvm/hyperv-stub.c
> +++ b/target/i386/kvm/hyperv-stub.c
> @@ -61,3 +61,8 @@ uint64_t hyperv_syndbg_query_options(void)
>   {
>       return 0;
>   }
> +
> +int hyperv_enable_synic(X86CPU *cpu)
> +{
> +    return 0;
> +}
> diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c
> index f7a81bd270..bd3c26d02b 100644
> --- a/target/i386/kvm/hyperv.c
> +++ b/target/i386/kvm/hyperv.c
> @@ -24,6 +24,15 @@ int hyperv_x86_synic_add(X86CPU *cpu)
>       return 0;
>   }
>   
> +int hyperv_enable_synic(X86CPU *cpu)
> +{
> +    int ret = 0;
> +    if (!hyperv_is_synic_enabled()) {
> +        ret = hyperv_x86_synic_add(cpu);
> +    }
> +    return ret;
> +}
> +
>   /*
>    * All devices possibly using SynIC have to be reset before calling this to let
>    * them remove their SINT routes first.
> diff --git a/target/i386/kvm/hyperv.h b/target/i386/kvm/hyperv.h
> index e45a4512fe..a393a5d428 100644
> --- a/target/i386/kvm/hyperv.h
> +++ b/target/i386/kvm/hyperv.h
> @@ -23,6 +23,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
>   #endif
>   
>   int hyperv_x86_synic_add(X86CPU *cpu);
> +int hyperv_enable_synic(X86CPU *cpu);
>   void hyperv_x86_synic_reset(X86CPU *cpu);
>   void hyperv_x86_synic_update(X86CPU *cpu);
>   
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index a29f757c16..9e352882c8 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1754,13 +1754,11 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>               return ret;
>           }
>   
> -        if (!hyperv_is_synic_enabled()) {
> -            ret = hyperv_x86_synic_add(cpu);
> -            if (ret < 0) {
> -                error_report("failed to create HyperV SynIC: %s",
> -                             strerror(-ret));
> -                return ret;
> -            }
> +        ret = hyperv_enable_synic(cpu);
> +        if (ret < 0) {
> +            error_report("failed to create HyperV SynIC: %s",
> +                         strerror(-ret));
> +            return ret;
>           }
>       }
>   

Why not simply add a hyperv_is_synic_enabled() stub?
Re: [PATCH for-11.0 2/2] i386/hyperv: add stubs for synic enablement
Posted by Ani Sinha 2 weeks, 3 days ago

> On 19 Mar 2026, at 4:23 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Hi Ani,
> 
> On 19/3/26 11:11, Ani Sinha wrote:
>> Add a new call hyperv_enable_synic() that can be called whether or not
>> CONFIG_HYPERV is enabled. This way genetic code in i396/kvm.c can call this
>> function to enable synic for hyperv. For non-hyperv cases, the stub will
>> be a noop.
>> Reported-by: Michale Tokarev <mjt@tls.msk.ru>
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>>  target/i386/kvm/hyperv-stub.c |  5 +++++
>>  target/i386/kvm/hyperv.c      |  9 +++++++++
>>  target/i386/kvm/hyperv.h      |  1 +
>>  target/i386/kvm/kvm.c         | 12 +++++-------
>>  4 files changed, 20 insertions(+), 7 deletions(-)
>> diff --git a/target/i386/kvm/hyperv-stub.c b/target/i386/kvm/hyperv-stub.c
>> index 5836f53c23..767a4c7e1a 100644
>> --- a/target/i386/kvm/hyperv-stub.c
>> +++ b/target/i386/kvm/hyperv-stub.c
>> @@ -61,3 +61,8 @@ uint64_t hyperv_syndbg_query_options(void)
>>  {
>>      return 0;
>>  }
>> +
>> +int hyperv_enable_synic(X86CPU *cpu)
>> +{
>> +    return 0;
>> +}
>> diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c
>> index f7a81bd270..bd3c26d02b 100644
>> --- a/target/i386/kvm/hyperv.c
>> +++ b/target/i386/kvm/hyperv.c
>> @@ -24,6 +24,15 @@ int hyperv_x86_synic_add(X86CPU *cpu)
>>      return 0;
>>  }
>>  +int hyperv_enable_synic(X86CPU *cpu)
>> +{
>> +    int ret = 0;
>> +    if (!hyperv_is_synic_enabled()) {
>> +        ret = hyperv_x86_synic_add(cpu);
>> +    }
>> +    return ret;
>> +}
>> +
>>  /*
>>   * All devices possibly using SynIC have to be reset before calling this to let
>>   * them remove their SINT routes first.
>> diff --git a/target/i386/kvm/hyperv.h b/target/i386/kvm/hyperv.h
>> index e45a4512fe..a393a5d428 100644
>> --- a/target/i386/kvm/hyperv.h
>> +++ b/target/i386/kvm/hyperv.h
>> @@ -23,6 +23,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
>>  #endif
>>    int hyperv_x86_synic_add(X86CPU *cpu);
>> +int hyperv_enable_synic(X86CPU *cpu);
>>  void hyperv_x86_synic_reset(X86CPU *cpu);
>>  void hyperv_x86_synic_update(X86CPU *cpu);
>>  diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index a29f757c16..9e352882c8 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1754,13 +1754,11 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>>              return ret;
>>          }
>>  -        if (!hyperv_is_synic_enabled()) {
>> -            ret = hyperv_x86_synic_add(cpu);
>> -            if (ret < 0) {
>> -                error_report("failed to create HyperV SynIC: %s",
>> -                             strerror(-ret));
>> -                return ret;
>> -            }
>> +        ret = hyperv_enable_synic(cpu);
>> +        if (ret < 0) {
>> +            error_report("failed to create HyperV SynIC: %s",
>> +                         strerror(-ret));
>> +            return ret;
>>          }
>>      }
>>  
> 
> Why not simply add a hyperv_is_synic_enabled() stub?

That would be wrong unless I assert in hyperv_is_synic_enabled() stub function which would break minimal microvms?
The other reason is I do not like hyperv_x86_synic_add() returning -ENOSYS. If hyperv is not enabled we do not need to call those functions at all.