| Message ID | 20230322102941.959-3-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: fixes around domain_vpl011_init | expand |
> 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>
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;
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
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
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
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,
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
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;
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(-)