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

> Hyman Huang <yong.hu...@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.hu...@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

Reply via email to