[PATCH] igvm: populate errp in stub functions

[PATCH] igvm: populate errp in stub functions

[Luigi Leonardi]

Use error_setg() to report that IGVM is not available, matching
the pattern used by other stubs in the tree.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
---
 stubs/igvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/stubs/igvm.c b/stubs/igvm.c
index 9e9f683fc9..dfb85eb548 100644
--- a/stubs/igvm.c
+++ b/stubs/igvm.c
@@ -17,15 +17,18 @@ int qigvm_x86_get_mem_map_entry(int index,
                                 ConfidentialGuestMemoryMapEntry *entry,
                                 Error **errp)
 {
+    error_setg(errp, "IGVM not supported on this platform");
     return -1;
 }
 
 int qigvm_x86_set_vp_context(void *data, int index, Error **errp)
 {
+    error_setg(errp, "IGVM not supported on this platform");
     return -1;
 }
 
 int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data, Error **errp)
 {
+    error_setg(errp, "IGVM not supported on this platform");
     return -1;
 }

---
base-commit: 29c042c6e9d4a09d4a0ac3fa54aeb7ee08ce0bdc
change-id: 20260609-igvm_stubs-d3205f48e5a0

Best regards,
-- 
Luigi Leonardi <leonardi@redhat.com>

Re: [PATCH] igvm: populate errp in stub functions

[Daniel P. Berrangé]

On Tue, Jun 09, 2026 at 03:20:14PM +0200, Luigi Leonardi wrote:
> Use error_setg() to report that IGVM is not available, matching
> the pattern used by other stubs in the tree.
> 
> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
> ---
>  stubs/igvm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/stubs/igvm.c b/stubs/igvm.c
> index 9e9f683fc9..dfb85eb548 100644
> --- a/stubs/igvm.c
> +++ b/stubs/igvm.c
> @@ -17,15 +17,18 @@ int qigvm_x86_get_mem_map_entry(int index,
>                                  ConfidentialGuestMemoryMapEntry *entry,
>                                  Error **errp)
>  {
> +    error_setg(errp, "IGVM not supported on this platform");
>      return -1;
>  }
>  
>  int qigvm_x86_set_vp_context(void *data, int index, Error **errp)
>  {
> +    error_setg(errp, "IGVM not supported on this platform");
>      return -1;
>  }
>  
>  int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data, Error **errp)
>  {
> +    error_setg(errp, "IGVM not supported on this platform");
>      return -1;
>  }

This is not wrong per-se, so on that  basis

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

but are any of these stubs actually reachable when IGVM is not
enabled in the build ? Usually with stubs we find that one
or two methods are the primary entrypoints which must return
an error, at which point everything else becomes unreachable.
The latter cases can just be g_assert_not_reached() as a sanity
check that some unexpected codepath isn't calling in without
checking status earlier. Those would thus would not need
error_setg, nor a 'return' statement.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] igvm: populate errp in stub functions

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 09, 2026 at 03:20:14PM +0200, Luigi Leonardi wrote:
>> Use error_setg() to report that IGVM is not available, matching
>> the pattern used by other stubs in the tree.
>> 
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
>> ---
>>  stubs/igvm.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/stubs/igvm.c b/stubs/igvm.c
>> index 9e9f683fc9..dfb85eb548 100644
>> --- a/stubs/igvm.c
>> +++ b/stubs/igvm.c
>> @@ -17,15 +17,18 @@ int qigvm_x86_get_mem_map_entry(int index,
>>                                  ConfidentialGuestMemoryMapEntry *entry,
>>                                  Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>>  
>>  int qigvm_x86_set_vp_context(void *data, int index, Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>>  
>>  int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data, Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>
> This is not wrong per-se, so on that  basis
>
>   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> but are any of these stubs actually reachable when IGVM is not
> enabled in the build ? Usually with stubs we find that one
> or two methods are the primary entrypoints which must return
> an error, at which point everything else becomes unreachable.
> The latter cases can just be g_assert_not_reached() as a sanity
> check that some unexpected codepath isn't calling in without
> checking status earlier. Those would thus would not need
> error_setg, nor a 'return' statement.

I much prefer g_assert_not_reached() to unreachable (and thus
untestable) errors.

Re: [PATCH] igvm: populate errp in stub functions

[Luigi Leonardi]

Hi Daniel,

On Tue, Jun 09, 2026 at 02:25:01PM +0100, Daniel P. Berrangé wrote:
>On Tue, Jun 09, 2026 at 03:20:14PM +0200, Luigi Leonardi wrote:
>> Use error_setg() to report that IGVM is not available, matching
>> the pattern used by other stubs in the tree.
>>
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
>> ---
>>  stubs/igvm.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/stubs/igvm.c b/stubs/igvm.c
>> index 9e9f683fc9..dfb85eb548 100644
>> --- a/stubs/igvm.c
>> +++ b/stubs/igvm.c
>> @@ -17,15 +17,18 @@ int qigvm_x86_get_mem_map_entry(int index,
>>                                  ConfidentialGuestMemoryMapEntry *entry,
>>                                  Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>>
>>  int qigvm_x86_set_vp_context(void *data, int index, Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>>
>>  int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data, Error **errp)
>>  {
>> +    error_setg(errp, "IGVM not supported on this platform");
>>      return -1;
>>  }
>
>This is not wrong per-se, so on that  basis
>
>  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>but are any of these stubs actually reachable when IGVM is not
>enabled in the build ? Usually with stubs we find that one
>or two methods are the primary entrypoints which must return
>an error, at which point everything else becomes unreachable.
>The latter cases can just be g_assert_not_reached() as a sanity
>check that some unexpected codepath isn't calling in without
>checking status earlier. Those would thus would not need
>error_setg, nor a 'return' statement.

No, when IGVM is not enabled in the build it's not possible to trigger
any of these stubs. On top of that, I don't think it is possible to trigger
these stubs even when igvm is enabled. This is because the only architecture
that implements igvm support in qemu is x86.

To give more context: the igvm code is split between `backends/igvm.c` and
`target/i386/igvm.c`. The former contains arch-independent functions,
while the latter, x86 related functions.

Therefore, stubs will only become relevant when new architectures will be
supported.

I'm not sure about using asserts here, because with this patch [1] we'll
handle optional igvm directives. So, when we support aarch64 in igvm,
we could have some directives (eg: `IGVM_VHT_MADT`) that are
implemented for one architecture but not for a different one, resulting
in the stub being hit. So, if that directive is marked as optional in the
igvm file, we should not crash qemu.

[1] https://lore.kernel.org/all/20260609-igvm_optional-v2-2-b1f1f08dc40e@redhat.com/

HTH,
Luigi

Re: [PATCH] igvm: populate errp in stub functions

[Gerd Hoffmann]

  Hi,

> No, when IGVM is not enabled in the build it's not possible to trigger
> any of these stubs. On top of that, I don't think it is possible to trigger
> these stubs even when igvm is enabled. This is because the only architecture
> that implements igvm support in qemu is x86.

We have these stubs mainly to avoid linker errors when compiling igvm
support on !x86 architectures.  So we do compile igvm support for other
architectures (or, to be exact, we compile the core igvm code once and
link it into all qemu-system-$arch variants).

Nevertheless I think that the stubs should not be reachable.  The igvm
code already has checks like this ...

    } else if (target_arch() == SYS_EMU_TARGET_X86_64) {
        result = qigvm_x86_set_vp_context(data, vp_context->vp_index, errp);

... so it should never try to call x86 support code on
qemu-system-$somethingelse.

On that base I think switching to asserts should be fine.  If we
actually hit any assert the core code lacks a sanity check.

take care,
  Gerd

Re: [PATCH] igvm: populate errp in stub functions

[Luigi Leonardi]

Hi,

On Wed, Jun 10, 2026 at 04:27:18PM +0200, Gerd Hoffmann wrote:
>  Hi,
>
>> No, when IGVM is not enabled in the build it's not possible to trigger
>> any of these stubs. On top of that, I don't think it is possible to trigger
>> these stubs even when igvm is enabled. This is because the only architecture
>> that implements igvm support in qemu is x86.
>
>We have these stubs mainly to avoid linker errors when compiling igvm
>support on !x86 architectures.  So we do compile igvm support for other
>architectures (or, to be exact, we compile the core igvm code once and
>link it into all qemu-system-$arch variants).
>
>Nevertheless I think that the stubs should not be reachable.  The igvm
>code already has checks like this ...
>
>    } else if (target_arch() == SYS_EMU_TARGET_X86_64) {
>        result = qigvm_x86_set_vp_context(data, vp_context->vp_index, errp);
>
>... so it should never try to call x86 support code on
>qemu-system-$somethingelse.
>
>On that base I think switching to asserts should be fine.  If we
>actually hit any assert the core code lacks a sanity check.
>
>take care,
>  Gerd
>

Right, I got confused by madt function not checking the arch.

Ignore this patch, I'll send a new series that improves madt and switches
to asserts.

Sorry for the confusion.

Thanks,
Luigi