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 )

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.

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

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.

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

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

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>