[PATCH] i386/sev: Nitpick at the error message's output

[PATCH] i386/sev: Nitpick at the error message's output

[Hyman Huang]

The incorrect error message was produced as a result of
the return number being disregarded on the sev_kvm_init
failure path.

For instance, when a user's failure to launch a SEV guest
is caused by an incorrect IOCTL, the following message is
reported:

kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
kvm: failed to initialize kvm: Operation not permitted

While the error message's accurate output should be:

kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
kvm: failed to initialize kvm: Inappropriate ioctl for device

Fix this by returning the return number directly on the
failure path.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 target/i386/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 9a71246682..4a69ca457c 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1019,7 +1019,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 err:
     sev_guest = NULL;
     ram_block_discard_disable(false);
-    return -1;
+    return ret;
 }
 
 int
-- 
2.39.1

Re: [PATCH] i386/sev: Nitpick at the error message's output

[Daniel P. Berrangé]

On Sat, Jan 06, 2024 at 12:09:55AM +0800, Hyman Huang wrote:
> The incorrect error message was produced as a result of
> the return number being disregarded on the sev_kvm_init
> failure path.
> 
> For instance, when a user's failure to launch a SEV guest
> is caused by an incorrect IOCTL, the following message is
> reported:
> 
> kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> kvm: failed to initialize kvm: Operation not permitted
> 
> While the error message's accurate output should be:
> 
> kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> kvm: failed to initialize kvm: Inappropriate ioctl for device
> 
> Fix this by returning the return number directly on the
> failure path.
> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  target/i386/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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


> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 9a71246682..4a69ca457c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1019,7 +1019,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>  err:
>      sev_guest = NULL;
>      ram_block_discard_disable(false);
> -    return -1;
> +    return ret;
>  }

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] i386/sev: Nitpick at the error message's output

[Alex Bennée]

Hyman Huang <yong.huang@smartx.com> writes:

> The incorrect error message was produced as a result of
> the return number being disregarded on the sev_kvm_init
> failure path.
>
> For instance, when a user's failure to launch a SEV guest
> is caused by an incorrect IOCTL, the following message is
> reported:
>
> kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> kvm: failed to initialize kvm: Operation not permitted
>
> While the error message's accurate output should be:
>
> kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> kvm: failed to initialize kvm: Inappropriate ioctl for device
>
> Fix this by returning the return number directly on the
> failure path.
>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  target/i386/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 9a71246682..4a69ca457c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1019,7 +1019,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>  err:
>      sev_guest = NULL;
>      ram_block_discard_disable(false);
> -    return -1;
> +    return ret;

I don't think this will be correct in all cases because there are other
legs (e.g. if (host_cbitpos != sev->cbitpos)) where ret may be the
successful setting of ram_block_discard_disable(true).

You might want to explore if the function can be re-written with
explicit return's and utilise autofree to do the clean-up of dynamic
objects.

I think this entails setting sev_guest at the end of the function just
before the return 0.

I'm not sure if there is a clean way to handle
ram_block_discard_disable(false); cleanly for all the failure legs
though. Maybe someone with more familiarity with the code has some ideas?


>  }
>  
>  int

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] i386/sev: Nitpick at the error message's output

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On Sat, Jan 6, 2024 at 12:43 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> Hyman Huang <yong.huang@smartx.com> writes:
>
> > The incorrect error message was produced as a result of
> > the return number being disregarded on the sev_kvm_init
> > failure path.
> >
> > For instance, when a user's failure to launch a SEV guest
> > is caused by an incorrect IOCTL, the following message is
> > reported:
> >
> > kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> > kvm: failed to initialize kvm: Operation not permitted
> >
> > While the error message's accurate output should be:
> >
> > kvm: sev_kvm_init: failed to initialize ret=-25 fw_error=0
> > kvm: failed to initialize kvm: Inappropriate ioctl for device
> >
> > Fix this by returning the return number directly on the
> > failure path.
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > ---
> >  target/i386/sev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 9a71246682..4a69ca457c 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -1019,7 +1019,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs,
> Error **errp)
> >  err:
> >      sev_guest = NULL;
> >      ram_block_discard_disable(false);
> > -    return -1;
> > +    return ret;
>
> I don't think this will be correct in all cases because there are other
> legs (e.g. if (host_cbitpos != sev->cbitpos)) where ret may be the
> successful setting of ram_block_discard_disable(true).
>

Indeed, the other failed paths are overlooked by me. I'll try a
commit aiming to sort the error message on all failure paths in
the next version.


> You might want to explore if the function can be re-written with
> explicit return's and utilise autofree to do the clean-up of dynamic
> objects.
>
> I think this entails setting sev_guest at the end of the function just
> before the return 0.
>
> I'm not sure if there is a clean way to handle
> ram_block_discard_disable(false); cleanly for all the failure legs
> though. Maybe someone with more familiarity with the code has some ideas?
>
>
> >  }
> >
> >  int
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>


-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 4000 bytes --]