[PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Jan Beulich]

This happens after nestedhvm_vcpu_initialise(), so its effects also need
to be undone.

Fixes: 40a4a9d72d16 ("viridian: add init hooks")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1583,7 +1583,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
 
     rc = viridian_vcpu_init(v);
     if ( rc )
-        goto fail5;
+        goto fail6;
 
     rc = ioreq_server_add_vcpu_all(d, v);
     if ( rc != 0 )

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Andrew Cooper]

On 18/10/2021 07:43, Jan Beulich wrote:
> This happens after nestedhvm_vcpu_initialise(), so its effects also need
> to be undone.
>
> Fixes: 40a4a9d72d16 ("viridian: add init hooks")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Ian Jackson]

Jan Beulich writes ("[PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()"):
> This happens after nestedhvm_vcpu_initialise(), so its effects also need
> to be undone.
> 
> Fixes: 40a4a9d72d16 ("viridian: add init hooks")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1583,7 +1583,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>  
>      rc = viridian_vcpu_init(v);
>      if ( rc )
> -        goto fail5;
> +        goto fail6;

Not acomment about the patch; rather about the code in general.

I have not looked at the context.

But OMG, this is horrific.  How can anyone write code in such an idiom
without writing endless bugs ?

Ian.

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Jan Beulich]

On 18.10.2021 12:42, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()"):
>> This happens after nestedhvm_vcpu_initialise(), so its effects also need
>> to be undone.
>>
>> Fixes: 40a4a9d72d16 ("viridian: add init hooks")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1583,7 +1583,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>>  
>>      rc = viridian_vcpu_init(v);
>>      if ( rc )
>> -        goto fail5;
>> +        goto fail6;
> 
> Not acomment about the patch; rather about the code in general.
> 
> I have not looked at the context.
> 
> But OMG, this is horrific.  How can anyone write code in such an idiom
> without writing endless bugs ?

Well, one of the reasons I dislike "goto".

Since you've been looking here - any chance of getting a release ack?
Perhaps also on "x86/shadow: make a local variable in sh_page_fault()
HVM-only" and "x86/PV: address odd UB in I/O emulation"? Aiui that's
going to be needed from today on ...

Jan

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()"):
> Since you've been looking here - any chance of getting a release ack?

I don't think one is needed[1], but FTR

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

> Perhaps also on "x86/shadow: make a local variable in sh_page_fault()
> HVM-only" and "x86/PV: address odd UB in I/O emulation"? Aiui that's
> going to be needed from today on ...

I assume these are bugfixes too ?

[1] I think I will send out a mail clearly stating the current state
of the tree.

Ian.

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Durrant, Paul]

On 18/10/2021 11:42, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()"):
>> This happens after nestedhvm_vcpu_initialise(), so its effects also need
>> to be undone.
>>
>> Fixes: 40a4a9d72d16 ("viridian: add init hooks")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1583,7 +1583,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>>   
>>       rc = viridian_vcpu_init(v);
>>       if ( rc )
>> -        goto fail5;
>> +        goto fail6;
> 
> Not acomment about the patch; rather about the code in general.
> 
> I have not looked at the context.
> 
> But OMG, this is horrific.  How can anyone write code in such an idiom
> without writing endless bugs ?
> 

Fairly easily. I think this is the first one due to an incorrect exit label.
Using such an idiom in the Windows PV drivers had meant many issues 
could be easily debugged without further code modification because you 
get an fairly instant audit trail of the route out of any failure condition.

   Paul

Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()

[Jan Beulich]

On 18.10.2021 13:27, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] x86/HVM: correct cleanup after failed viridian_vcpu_init()"):
>> Since you've been looking here - any chance of getting a release ack?
> 
> I don't think one is needed[1],

Oh, okay - I keep mixing the different forms of "freeze".

> but FTR
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks.

>> Perhaps also on "x86/shadow: make a local variable in sh_page_fault()
>> HVM-only" and "x86/PV: address odd UB in I/O emulation"? Aiui that's
>> going to be needed from today on ...
> 
> I assume these are bugfixes too ?

Yes.

Jan