[PATCH 0/2] xen/arm: fixes around domain_vpl011_init

[PATCH 0/2] xen/arm: fixes around domain_vpl011_init

[Michal Orzel]

This series contains two trivial fixes around domain_vpl011_init().

Michal Orzel (2):
  xen/arm: domain_build: Check return code of domain_vpl011_init
  xen/arm: vpl011: Fix domain_vpl011_init error path

 xen/arch/arm/domain_build.c |  4 ++++
 xen/arch/arm/vpl011.c       | 11 +++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init

[Michal Orzel]

We are assigning return code of domain_vpl011_init() to a variable
without checking it for an error. Fix it.

Fixes: 3580c8b2dfc3 ("xen/arm: if direct-map domain use native UART address and IRQ number for vPL011")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/domain_build.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9707eb7b1bb1..3195c5b6d6ac 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3826,7 +3826,11 @@ static int __init construct_domU(struct domain *d,
      * shall be done first.
      */
     if ( kinfo.vpl011 )
+    {
         rc = domain_vpl011_init(d, NULL);
+        if ( rc < 0 )
+            return rc;
+    }
 
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
-- 
2.25.1

[PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Michal Orzel]

When vgic_reserve_virq() fails and backend is in domain, we should also
free the allocated event channel.

When backend is in Xen and call to xzalloc() returns NULL, there is no
need to call xfree(). This should be done instead on an error path
from vgic_reserve_virq(). Also, take the opportunity to return -ENOMEM
instead of -EINVAL when memory allocation fails.

Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/vpl011.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 541ec962f189..df29a65ad365 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
         vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
         if ( vpl011->backend.xen == NULL )
         {
-            rc = -EINVAL;
-            goto out1;
+            rc = -ENOMEM;
+            goto out;
         }
     }
 
@@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
 out2:
     vgic_free_virq(d, vpl011->virq);
 
+    if ( vpl011->backend_in_domain )
+        free_xen_event_channel(d, vpl011->evtchn);
+    else
+        xfree(vpl011->backend.xen);
+
 out1:
     if ( vpl011->backend_in_domain )
         destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
                                 vpl011->backend.dom.ring_page);
-    else
-        xfree(vpl011->backend.xen);
 
 out:
     return rc;
-- 
2.25.1

Re: [PATCH 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init

[Luca Fancellu]

> On 22 Mar 2023, at 10:29, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> We are assigning return code of domain_vpl011_init() to a variable
> without checking it for an error. Fix it.
> 
> Fixes: 3580c8b2dfc3 ("xen/arm: if direct-map domain use native UART address and IRQ number for vPL011")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Hi Michal,

Looks good to me,

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Luca Fancellu]

> On 22 Mar 2023, at 10:29, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> When vgic_reserve_virq() fails and backend is in domain, we should also
> free the allocated event channel.
> 
> When backend is in Xen and call to xzalloc() returns NULL, there is no
> need to call xfree(). This should be done instead on an error path
> from vgic_reserve_virq(). Also, take the opportunity to return -ENOMEM
> instead of -EINVAL when memory allocation fails.
> 
> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Also this one looks good to me:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init

[Julien Grall]

Hi,

On 22/03/2023 10:29, Michal Orzel wrote:
> We are assigning return code of domain_vpl011_init() to a variable
> without checking it for an error. Fix it.
> 
> Fixes: 3580c8b2dfc3 ("xen/arm: if direct-map domain use native UART address and IRQ number for vPL011")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Julien Grall]

Hi Michal,

On 22/03/2023 10:29, Michal Orzel wrote:
> When vgic_reserve_virq() fails and backend is in domain, we should also
> free the allocated event channel.
> 
> When backend is in Xen and call to xzalloc() returns NULL, there is no
> need to call xfree(). This should be done instead on an error path
> from vgic_reserve_virq().

Most likely this was implemented this way to avoid a double "if ( 
vpl011->backend_in_domain)". TBH, I am not very thrilled with this 
approach. Could we instead consider to use domain_pl011_deinit()? (A 
couple of tweak would be necessary to use it)

> Also, take the opportunity to return -ENOMEM
> instead of -EINVAL when memory allocation fails.
> 
> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   xen/arch/arm/vpl011.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 541ec962f189..df29a65ad365 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>           vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>           if ( vpl011->backend.xen == NULL )
>           {
> -            rc = -EINVAL;
> -            goto out1;
> +            rc = -ENOMEM;
> +            goto out;
>           }
>       }
>   
> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>   out2:
>       vgic_free_virq(d, vpl011->virq);
>   
> +    if ( vpl011->backend_in_domain )
> +        free_xen_event_channel(d, vpl011->evtchn);
> +    else
> +        xfree(vpl011->backend.xen);

There is another bug here (unrelated to your change). You want to use 
XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?

> +
>   out1:
>       if ( vpl011->backend_in_domain )
>           destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>                                   vpl011->backend.dom.ring_page);
> -    else
> -        xfree(vpl011->backend.xen);
>   
>   out:
>       return rc;

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Michal Orzel]

Hi Julien,

On 22/03/2023 17:19, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 22/03/2023 10:29, Michal Orzel wrote:
>> When vgic_reserve_virq() fails and backend is in domain, we should also
>> free the allocated event channel.
>>
>> When backend is in Xen and call to xzalloc() returns NULL, there is no
>> need to call xfree(). This should be done instead on an error path
>> from vgic_reserve_virq().
> 
> Most likely this was implemented this way to avoid a double "if (
> vpl011->backend_in_domain)". TBH, I am not very thrilled with this
> approach. Could we instead consider to use domain_pl011_deinit()? (A
> couple of tweak would be necessary to use it)
I think we could. More about it later.

> 
>> Also, take the opportunity to return -ENOMEM
>> instead of -EINVAL when memory allocation fails.
>>
>> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   xen/arch/arm/vpl011.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 541ec962f189..df29a65ad365 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>           vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>>           if ( vpl011->backend.xen == NULL )
>>           {
>> -            rc = -EINVAL;
>> -            goto out1;
>> +            rc = -ENOMEM;
>> +            goto out;
>>           }
>>       }
>>
>> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>   out2:
>>       vgic_free_virq(d, vpl011->virq);
>>
>> +    if ( vpl011->backend_in_domain )
>> +        free_xen_event_channel(d, vpl011->evtchn);
>> +    else
>> +        xfree(vpl011->backend.xen);
> 
> There is another bug here (unrelated to your change). You want to use
> XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
Strictly speaking this is not a bug. Memory allocation can only happen if backend is in Xen.
This means, that if vpl011 init fails, we will call free only once (domain_vpl011_deinit
will not be called on this path i.e. we will invoke panic after construct_domU).
Of course, we could switch to XFREE just for sanity.

> 
>> +
>>   out1:
>>       if ( vpl011->backend_in_domain )
>>           destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>>                                   vpl011->backend.dom.ring_page);
>> -    else
>> -        xfree(vpl011->backend.xen);
>>
>>   out:
>>       return rc;
> 
Solution to reuse domain_pl011_deinit would be as follows:

     vgic_free_virq(d, vpl011->virq);
 
 out1:
-    if ( vpl011->backend_in_domain )
-        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
-                                vpl011->backend.dom.ring_page);
-    else
-        xfree(vpl011->backend.xen);
+    domain_vpl011_deinit(d);
 
 out:
     return rc;
@@ -737,12 +733,15 @@ void domain_vpl011_deinit(struct domain *d)
 
     if ( vpl011->backend_in_domain )
     {
-        if ( !vpl011->backend.dom.ring_buf )
-            return;
+        if ( vpl011->backend.dom.ring_buf )
+            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
+                                    vpl011->backend.dom.ring_page);
 
-        free_xen_event_channel(d, vpl011->evtchn);
-        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
-                                vpl011->backend.dom.ring_page);
+        if ( vpl011->evtchn )
+        {
+            free_xen_event_channel(d, vpl011->evtchn);
+            vpl011->evtchn = 0;
+        }
     }
     else
         xfree(vpl011->backend.xen);

However there is one problem with guarding free_xen_event_channel to be called only once.
Even without allocating event channel, vpl011->evtchn is 0 by default. So doing:
if ( is_port_valid(vpl011->evtchn) )
    free_xen_event_channel(d, vpl011->evtchn);
would not help us as evtchn 0 is always there. So in my proposal I'm assuming (I think correctly)
that vpl011->evtchn cannot be 0 after successful evtchn allocation because 0 is "reserved".
But I might be wrong in which case I'm clueless how to do the proper check.

~Michal

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Julien Grall]

On 23/03/2023 11:10, Michal Orzel wrote:
> Hi Julien,
> 
> On 22/03/2023 17:19, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 22/03/2023 10:29, Michal Orzel wrote:
>>> When vgic_reserve_virq() fails and backend is in domain, we should also
>>> free the allocated event channel.
>>>
>>> When backend is in Xen and call to xzalloc() returns NULL, there is no
>>> need to call xfree(). This should be done instead on an error path
>>> from vgic_reserve_virq().
>>
>> Most likely this was implemented this way to avoid a double "if (
>> vpl011->backend_in_domain)". TBH, I am not very thrilled with this
>> approach. Could we instead consider to use domain_pl011_deinit()? (A
>> couple of tweak would be necessary to use it)
> I think we could. More about it later.
> 
>>
>>> Also, take the opportunity to return -ENOMEM
>>> instead of -EINVAL when memory allocation fails.
>>>
>>> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>>    xen/arch/arm/vpl011.c | 11 +++++++----
>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index 541ec962f189..df29a65ad365 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>            vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>>>            if ( vpl011->backend.xen == NULL )
>>>            {
>>> -            rc = -EINVAL;
>>> -            goto out1;
>>> +            rc = -ENOMEM;
>>> +            goto out;
>>>            }
>>>        }
>>>
>>> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>    out2:
>>>        vgic_free_virq(d, vpl011->virq);
>>>
>>> +    if ( vpl011->backend_in_domain )
>>> +        free_xen_event_channel(d, vpl011->evtchn);
>>> +    else
>>> +        xfree(vpl011->backend.xen);
>>
>> There is another bug here (unrelated to your change). You want to use
>> XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
> Strictly speaking this is not a bug. Memory allocation can only happen if backend is in Xen.
> This means, that if vpl011 init fails, we will call free only once (domain_vpl011_deinit
> will not be called on this path i.e. we will invoke panic after construct_domU).

Well yes, in the current use this is not a real bug (it is only latent). 
But the same reasoning is also true for adding the call to 
free_xen_event_channel() because we would not continue to run the domain 
if domain_vpl011_init() is failing (even when the backend is in the 
domain). And even if we were going to continue this is just a channel 
that cannot be used. It will get free when the domain is destroyed 
(either explicitly in deinit() or by evtchn_destroy()).

> Of course, we could switch to XFREE just for sanity.
This is just not about sanity here. You are relying on how the caller is 
behaving. And we have no guarantee this is going to be the same forever. 
For instance, one may decide that it would fine to continue even if 
construct_domU() is failing (e.g. because the domain is not critical). 
At this point, this would become a real bug.

>>> +
>>>    out1:
>>>        if ( vpl011->backend_in_domain )
>>>            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>>>                                    vpl011->backend.dom.ring_page);
>>> -    else
>>> -        xfree(vpl011->backend.xen);
>>>
>>>    out:
>>>        return rc;
>>
> Solution to reuse domain_pl011_deinit would be as follows:
> 
>       vgic_free_virq(d, vpl011->virq);

We should move this call in domain_vpl011_deinit();

>   
>   out1:
> -    if ( vpl011->backend_in_domain )
> -        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> -                                vpl011->backend.dom.ring_page);
> -    else
> -        xfree(vpl011->backend.xen);
> +    domain_vpl011_deinit(d);
>   
>   out:
>       return rc;
> @@ -737,12 +733,15 @@ void domain_vpl011_deinit(struct domain *d)
>   
>       if ( vpl011->backend_in_domain )
>       {
> -        if ( !vpl011->backend.dom.ring_buf )
> -            return;
> +        if ( vpl011->backend.dom.ring_buf )
> +            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> +                                    vpl011->backend.dom.ring_page);
>   
> -        free_xen_event_channel(d, vpl011->evtchn);
> -        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> -                                vpl011->backend.dom.ring_page);
> +        if ( vpl011->evtchn )
> +        {
> +            free_xen_event_channel(d, vpl011->evtchn);
> +            vpl011->evtchn = 0;
> +        }
>       }
>       else
>           xfree(vpl011->backend.xen);

Now, it is more clearer that this will need to become an XFREE().

> 
> However there is one problem with guarding free_xen_event_channel to be called only once.
> Even without allocating event channel, vpl011->evtchn is 0 by default. So doing:
> if ( is_port_valid(vpl011->evtchn) )
>      free_xen_event_channel(d, vpl011->evtchn);
> would not help us as evtchn 0 is always there. So in my proposal I'm assuming (I think correctly)
> that vpl011->evtchn cannot be 0 after successful evtchn allocation because 0 is "reserved".
0 was reserved because it is used as the "invalid event channel". So 
your check above is correct.

> But I might be wrong in which case I'm clueless how to do the proper check.
> 
> ~Michal

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Michal Orzel]

Hi Julien,

On 23/03/2023 12:33, Julien Grall wrote:
> 
> 
> On 23/03/2023 11:10, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 22/03/2023 17:19, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 22/03/2023 10:29, Michal Orzel wrote:
>>>> When vgic_reserve_virq() fails and backend is in domain, we should also
>>>> free the allocated event channel.
>>>>
>>>> When backend is in Xen and call to xzalloc() returns NULL, there is no
>>>> need to call xfree(). This should be done instead on an error path
>>>> from vgic_reserve_virq().
>>>
>>> Most likely this was implemented this way to avoid a double "if (
>>> vpl011->backend_in_domain)". TBH, I am not very thrilled with this
>>> approach. Could we instead consider to use domain_pl011_deinit()? (A
>>> couple of tweak would be necessary to use it)
>> I think we could. More about it later.
>>
>>>
>>>> Also, take the opportunity to return -ENOMEM
>>>> instead of -EINVAL when memory allocation fails.
>>>>
>>>> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>>    xen/arch/arm/vpl011.c | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>>> index 541ec962f189..df29a65ad365 100644
>>>> --- a/xen/arch/arm/vpl011.c
>>>> +++ b/xen/arch/arm/vpl011.c
>>>> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>            vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>>>>            if ( vpl011->backend.xen == NULL )
>>>>            {
>>>> -            rc = -EINVAL;
>>>> -            goto out1;
>>>> +            rc = -ENOMEM;
>>>> +            goto out;
>>>>            }
>>>>        }
>>>>
>>>> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>    out2:
>>>>        vgic_free_virq(d, vpl011->virq);
>>>>
>>>> +    if ( vpl011->backend_in_domain )
>>>> +        free_xen_event_channel(d, vpl011->evtchn);
>>>> +    else
>>>> +        xfree(vpl011->backend.xen);
>>>
>>> There is another bug here (unrelated to your change). You want to use
>>> XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
>> Strictly speaking this is not a bug. Memory allocation can only happen if backend is in Xen.
>> This means, that if vpl011 init fails, we will call free only once (domain_vpl011_deinit
>> will not be called on this path i.e. we will invoke panic after construct_domU).
> 
> Well yes, in the current use this is not a real bug (it is only latent).
> But the same reasoning is also true for adding the call to
> free_xen_event_channel() because we would not continue to run the domain
> if domain_vpl011_init() is failing (even when the backend is in the
> domain). And even if we were going to continue this is just a channel
> that cannot be used. It will get free when the domain is destroyed
> (either explicitly in deinit() or by evtchn_destroy()).
> 
>> Of course, we could switch to XFREE just for sanity.
> This is just not about sanity here. You are relying on how the caller is
> behaving. And we have no guarantee this is going to be the same forever.
> For instance, one may decide that it would fine to continue even if
> construct_domU() is failing (e.g. because the domain is not critical).
> At this point, this would become a real bug.
ok, makes sense.

> 
>>>> +
>>>>    out1:
>>>>        if ( vpl011->backend_in_domain )
>>>>            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>>>>                                    vpl011->backend.dom.ring_page);
>>>> -    else
>>>> -        xfree(vpl011->backend.xen);
>>>>
>>>>    out:
>>>>        return rc;
>>>
>> Solution to reuse domain_pl011_deinit would be as follows:
>>
>>       vgic_free_virq(d, vpl011->virq);
> 
> We should move this call in domain_vpl011_deinit();
True and I think it does not need any guard as in case of a not registered virq it will
just clear the already cleared bit.

> 
>>
>>   out1:
>> -    if ( vpl011->backend_in_domain )
>> -        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>> -                                vpl011->backend.dom.ring_page);
>> -    else
>> -        xfree(vpl011->backend.xen);
>> +    domain_vpl011_deinit(d);
>>
>>   out:
>>       return rc;
>> @@ -737,12 +733,15 @@ void domain_vpl011_deinit(struct domain *d)
>>
>>       if ( vpl011->backend_in_domain )
>>       {
>> -        if ( !vpl011->backend.dom.ring_buf )
>> -            return;
>> +        if ( vpl011->backend.dom.ring_buf )
>> +            destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>> +                                    vpl011->backend.dom.ring_page);
>>
>> -        free_xen_event_channel(d, vpl011->evtchn);
>> -        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>> -                                vpl011->backend.dom.ring_page);
>> +        if ( vpl011->evtchn )
>> +        {
>> +            free_xen_event_channel(d, vpl011->evtchn);
>> +            vpl011->evtchn = 0;
>> +        }
>>       }
>>       else
>>           xfree(vpl011->backend.xen);
> 
> Now, it is more clearer that this will need to become an XFREE().
> 
>>
>> However there is one problem with guarding free_xen_event_channel to be called only once.
>> Even without allocating event channel, vpl011->evtchn is 0 by default. So doing:
>> if ( is_port_valid(vpl011->evtchn) )
>>      free_xen_event_channel(d, vpl011->evtchn);
>> would not help us as evtchn 0 is always there. So in my proposal I'm assuming (I think correctly)
>> that vpl011->evtchn cannot be 0 after successful evtchn allocation because 0 is "reserved".
> 0 was reserved because it is used as the "invalid event channel". So
> your check above is correct.
ok, let me send a v2 then.

~Michal

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Julien Grall]

On 23/03/2023 12:13, Michal Orzel wrote:
> Hi Julien,
> 
> On 23/03/2023 12:33, Julien Grall wrote:
>>
>>
>> On 23/03/2023 11:10, Michal Orzel wrote:
>>> Hi Julien,
>>>
>>> On 22/03/2023 17:19, Julien Grall wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On 22/03/2023 10:29, Michal Orzel wrote:
>>>>> When vgic_reserve_virq() fails and backend is in domain, we should also
>>>>> free the allocated event channel.
>>>>>
>>>>> When backend is in Xen and call to xzalloc() returns NULL, there is no
>>>>> need to call xfree(). This should be done instead on an error path
>>>>> from vgic_reserve_virq().
>>>>
>>>> Most likely this was implemented this way to avoid a double "if (
>>>> vpl011->backend_in_domain)". TBH, I am not very thrilled with this
>>>> approach. Could we instead consider to use domain_pl011_deinit()? (A
>>>> couple of tweak would be necessary to use it)
>>> I think we could. More about it later.
>>>
>>>>
>>>>> Also, take the opportunity to return -ENOMEM
>>>>> instead of -EINVAL when memory allocation fails.
>>>>>
>>>>> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>> ---
>>>>>     xen/arch/arm/vpl011.c | 11 +++++++----
>>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>>>> index 541ec962f189..df29a65ad365 100644
>>>>> --- a/xen/arch/arm/vpl011.c
>>>>> +++ b/xen/arch/arm/vpl011.c
>>>>> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>>             vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>>>>>             if ( vpl011->backend.xen == NULL )
>>>>>             {
>>>>> -            rc = -EINVAL;
>>>>> -            goto out1;
>>>>> +            rc = -ENOMEM;
>>>>> +            goto out;
>>>>>             }
>>>>>         }
>>>>>
>>>>> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>>     out2:
>>>>>         vgic_free_virq(d, vpl011->virq);
>>>>>
>>>>> +    if ( vpl011->backend_in_domain )
>>>>> +        free_xen_event_channel(d, vpl011->evtchn);
>>>>> +    else
>>>>> +        xfree(vpl011->backend.xen);
>>>>
>>>> There is another bug here (unrelated to your change). You want to use
>>>> XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
>>> Strictly speaking this is not a bug. Memory allocation can only happen if backend is in Xen.
>>> This means, that if vpl011 init fails, we will call free only once (domain_vpl011_deinit
>>> will not be called on this path i.e. we will invoke panic after construct_domU).
>>
>> Well yes, in the current use this is not a real bug (it is only latent).
>> But the same reasoning is also true for adding the call to
>> free_xen_event_channel() because we would not continue to run the domain
>> if domain_vpl011_init() is failing (even when the backend is in the
>> domain). And even if we were going to continue this is just a channel
>> that cannot be used. It will get free when the domain is destroyed
>> (either explicitly in deinit() or by evtchn_destroy()).
>>
>>> Of course, we could switch to XFREE just for sanity.
>> This is just not about sanity here. You are relying on how the caller is
>> behaving. And we have no guarantee this is going to be the same forever.
>> For instance, one may decide that it would fine to continue even if
>> construct_domU() is failing (e.g. because the domain is not critical).
>> At this point, this would become a real bug.
> ok, makes sense.
> 
>>
>>>>> +
>>>>>     out1:
>>>>>         if ( vpl011->backend_in_domain )
>>>>>             destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>>>>>                                     vpl011->backend.dom.ring_page);
>>>>> -    else
>>>>> -        xfree(vpl011->backend.xen);
>>>>>
>>>>>     out:
>>>>>         return rc;
>>>>
>>> Solution to reuse domain_pl011_deinit would be as follows:
>>>
>>>        vgic_free_virq(d, vpl011->virq);
>>
>> We should move this call in domain_vpl011_deinit();
> True and I think it does not need any guard as in case of a not registered virq it will
> just clear the already cleared bit.

Technically it could have been reserved by someone else afterwards. So 
it would be best to 0 it (we allocate a SPI so we could use 0 as invalid).

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path

[Michal Orzel]

On 23/03/2023 13:57, Julien Grall wrote:
> 
> 
> On 23/03/2023 12:13, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 23/03/2023 12:33, Julien Grall wrote:
>>>
>>>
>>> On 23/03/2023 11:10, Michal Orzel wrote:
>>>> Hi Julien,
>>>>
>>>> On 22/03/2023 17:19, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On 22/03/2023 10:29, Michal Orzel wrote:
>>>>>> When vgic_reserve_virq() fails and backend is in domain, we should also
>>>>>> free the allocated event channel.
>>>>>>
>>>>>> When backend is in Xen and call to xzalloc() returns NULL, there is no
>>>>>> need to call xfree(). This should be done instead on an error path
>>>>>> from vgic_reserve_virq().
>>>>>
>>>>> Most likely this was implemented this way to avoid a double "if (
>>>>> vpl011->backend_in_domain)". TBH, I am not very thrilled with this
>>>>> approach. Could we instead consider to use domain_pl011_deinit()? (A
>>>>> couple of tweak would be necessary to use it)
>>>> I think we could. More about it later.
>>>>
>>>>>
>>>>>> Also, take the opportunity to return -ENOMEM
>>>>>> instead of -EINVAL when memory allocation fails.
>>>>>>
>>>>>> Fixes: 1ee1e4b0d1ff ("xen/arm: Allow vpl011 to be used by DomU")
>>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>>> ---
>>>>>>     xen/arch/arm/vpl011.c | 11 +++++++----
>>>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>>>>> index 541ec962f189..df29a65ad365 100644
>>>>>> --- a/xen/arch/arm/vpl011.c
>>>>>> +++ b/xen/arch/arm/vpl011.c
>>>>>> @@ -696,8 +696,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>>>             vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
>>>>>>             if ( vpl011->backend.xen == NULL )
>>>>>>             {
>>>>>> -            rc = -EINVAL;
>>>>>> -            goto out1;
>>>>>> +            rc = -ENOMEM;
>>>>>> +            goto out;
>>>>>>             }
>>>>>>         }
>>>>>>
>>>>>> @@ -720,12 +720,15 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>>>>>>     out2:
>>>>>>         vgic_free_virq(d, vpl011->virq);
>>>>>>
>>>>>> +    if ( vpl011->backend_in_domain )
>>>>>> +        free_xen_event_channel(d, vpl011->evtchn);
>>>>>> +    else
>>>>>> +        xfree(vpl011->backend.xen);
>>>>>
>>>>> There is another bug here (unrelated to your change). You want to use
>>>>> XFREE() to avoid an extra free in domain_pl011_deinit(). Can you look at it?
>>>> Strictly speaking this is not a bug. Memory allocation can only happen if backend is in Xen.
>>>> This means, that if vpl011 init fails, we will call free only once (domain_vpl011_deinit
>>>> will not be called on this path i.e. we will invoke panic after construct_domU).
>>>
>>> Well yes, in the current use this is not a real bug (it is only latent).
>>> But the same reasoning is also true for adding the call to
>>> free_xen_event_channel() because we would not continue to run the domain
>>> if domain_vpl011_init() is failing (even when the backend is in the
>>> domain). And even if we were going to continue this is just a channel
>>> that cannot be used. It will get free when the domain is destroyed
>>> (either explicitly in deinit() or by evtchn_destroy()).
>>>
>>>> Of course, we could switch to XFREE just for sanity.
>>> This is just not about sanity here. You are relying on how the caller is
>>> behaving. And we have no guarantee this is going to be the same forever.
>>> For instance, one may decide that it would fine to continue even if
>>> construct_domU() is failing (e.g. because the domain is not critical).
>>> At this point, this would become a real bug.
>> ok, makes sense.
>>
>>>
>>>>>> +
>>>>>>     out1:
>>>>>>         if ( vpl011->backend_in_domain )
>>>>>>             destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
>>>>>>                                     vpl011->backend.dom.ring_page);
>>>>>> -    else
>>>>>> -        xfree(vpl011->backend.xen);
>>>>>>
>>>>>>     out:
>>>>>>         return rc;
>>>>>
>>>> Solution to reuse domain_pl011_deinit would be as follows:
>>>>
>>>>        vgic_free_virq(d, vpl011->virq);
>>>
>>> We should move this call in domain_vpl011_deinit();
>> True and I think it does not need any guard as in case of a not registered virq it will
>> just clear the already cleared bit.
> 
> Technically it could have been reserved by someone else afterwards. So
> it would be best to 0 it (we allocate a SPI so we could use 0 as invalid).
Hmm, ok so the same handling as for event channel:
if ( vpl011->virq )
{
    vgic_free_virq(d, vpl011->virq);
    vpl011->virq = 0;
}

~Michal