* [PATCH v2 0/2] xen/arm: fixes around domain_vpl011_init
@ 2023-03-23 13:56 Michal Orzel
2023-03-23 13:56 ` [PATCH v2 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init Michal Orzel
2023-03-23 13:56 ` [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path Michal Orzel
0 siblings, 2 replies; 5+ messages in thread
From: Michal Orzel @ 2023-03-23 13:56 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
This series contains 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 | 47 +++++++++++++++++++++++--------------
2 files changed, 34 insertions(+), 17 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init
2023-03-23 13:56 [PATCH v2 0/2] xen/arm: fixes around domain_vpl011_init Michal Orzel
@ 2023-03-23 13:56 ` Michal Orzel
2023-03-23 13:56 ` [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path Michal Orzel
1 sibling, 0 replies; 5+ messages in thread
From: Michal Orzel @ 2023-03-23 13:56 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Luca Fancellu, Julien Grall
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>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in v2:
- none
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path
2023-03-23 13:56 [PATCH v2 0/2] xen/arm: fixes around domain_vpl011_init Michal Orzel
2023-03-23 13:56 ` [PATCH v2 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init Michal Orzel
@ 2023-03-23 13:56 ` Michal Orzel
2023-03-24 9:35 ` Luca Fancellu
1 sibling, 1 reply; 5+ messages in thread
From: Michal Orzel @ 2023-03-23 13:56 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
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(). Moreover, we should be calling XFREE() to
prevent an extra free in domain_vpl011_deinit().
In order not to repeat the same steps twice, call domain_vpl011_deinit()
on an error path whenever there is more work to do than returning rc.
Since this function can now be called from different places and more
than once, add proper guards, use XFREE() instead of xfree() and move
vgic_free_virq() to it.
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>
---
Changes in v2:
- make use of domain_vpl011_deinit() to prevent code duplication
- use XFREE() instead of xfree()
---
xen/arch/arm/vpl011.c | 47 +++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 541ec962f189..2fa80bc15ac4 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;
}
}
@@ -705,7 +705,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
if ( !rc )
{
rc = -EINVAL;
- goto out2;
+ goto out1;
}
vpl011->uartfr = TXFE | RXFE;
@@ -717,15 +717,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
return 0;
-out2:
- 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;
@@ -735,17 +728,37 @@ void domain_vpl011_deinit(struct domain *d)
{
struct vpl011 *vpl011 = &d->arch.vpl011;
+ if ( vpl011->virq )
+ {
+ vgic_free_virq(d, vpl011->virq);
+
+ /*
+ * Set to invalid irq (we use SPI) to prevent extra free and to avoid
+ * freeing irq that could have already been reserved by someone else.
+ */
+ vpl011->virq = 0;
+ }
+
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);
+
+ if ( vpl011->evtchn )
+ {
+ free_xen_event_channel(d, vpl011->evtchn);
- free_xen_event_channel(d, vpl011->evtchn);
- destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
- vpl011->backend.dom.ring_page);
+ /*
+ * Set to invalid event channel port to prevent extra free and to
+ * avoid freeing port that could have already been allocated for
+ * other purposes.
+ */
+ vpl011->evtchn = 0;
+ }
}
else
- xfree(vpl011->backend.xen);
+ XFREE(vpl011->backend.xen);
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path
2023-03-23 13:56 ` [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path Michal Orzel
@ 2023-03-24 9:35 ` Luca Fancellu
2023-03-29 21:20 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Luca Fancellu @ 2023-03-24 9:35 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> On 23 Mar 2023, at 13:56, 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(). Moreover, we should be calling XFREE() to
> prevent an extra free in domain_vpl011_deinit().
>
> In order not to repeat the same steps twice, call domain_vpl011_deinit()
> on an error path whenever there is more work to do than returning rc.
> Since this function can now be called from different places and more
> than once, add proper guards, use XFREE() instead of xfree() and move
> vgic_free_virq() to it.
>
> 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>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path
2023-03-24 9:35 ` Luca Fancellu
@ 2023-03-29 21:20 ` Julien Grall
0 siblings, 0 replies; 5+ messages in thread
From: Julien Grall @ 2023-03-29 21:20 UTC (permalink / raw
To: Luca Fancellu, Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
Hi,
On 24/03/2023 09:35, Luca Fancellu wrote:
>
>
>> On 23 Mar 2023, at 13:56, 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(). Moreover, we should be calling XFREE() to
>> prevent an extra free in domain_vpl011_deinit().
>>
>> In order not to repeat the same steps twice, call domain_vpl011_deinit()
>> on an error path whenever there is more work to do than returning rc.
>> Since this function can now be called from different places and more
>> than once, add proper guards, use XFREE() instead of xfree() and move
>> vgic_free_virq() to it.
>>
>> 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>
>
>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
And committed the series.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-29 21:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-23 13:56 [PATCH v2 0/2] xen/arm: fixes around domain_vpl011_init Michal Orzel
2023-03-23 13:56 ` [PATCH v2 1/2] xen/arm: domain_build: Check return code of domain_vpl011_init Michal Orzel
2023-03-23 13:56 ` [PATCH v2 2/2] xen/arm: vpl011: Fix domain_vpl011_init error path Michal Orzel
2023-03-24 9:35 ` Luca Fancellu
2023-03-29 21:20 ` Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).