It's acpi_parse_one_rmrr() where the allocation is coming from (by way
of invoking acpi_parse_dev_scope()), or in add_one_user_rmrr()'s case
allocation is even open-coded there, so freeing would better also happen
there. Care needs to be taken to preserve acpi_parse_one_rmrr()'s
ultimate return value.
While fiddling with callers also move scope_devices_free() to .init and
have it use XFREE() instead of open-coding it.
In register_one_rmrr() also have the "ignore" path take the main
function return path.
Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -82,14 +82,13 @@ static int __init acpi_register_rmrr_uni
return 0;
}
-static void scope_devices_free(struct dmar_scope *scope)
+static void __init scope_devices_free(struct dmar_scope *scope)
{
if ( !scope )
return;
scope->devices_cnt = 0;
- xfree(scope->devices);
- scope->devices = NULL;
+ XFREE(scope->devices);
}
static void __init disable_all_dmar_units(void)
@@ -595,17 +594,13 @@ static int register_one_rmrr(struct acpi
" Ignore RMRR [%"PRIx64",%"PRIx64"] as no device"
" under its scope is PCI discoverable!\n",
rmrru->base_address, rmrru->end_address);
- scope_devices_free(&rmrru->scope);
- xfree(rmrru);
- return 1;
+ ret = 1;
}
else if ( rmrru->base_address > rmrru->end_address )
{
dprintk(XENLOG_WARNING VTDPREFIX,
" RMRR [%"PRIx64",%"PRIx64"] is incorrect!\n",
rmrru->base_address, rmrru->end_address);
- scope_devices_free(&rmrru->scope);
- xfree(rmrru);
ret = -EFAULT;
}
else
@@ -660,21 +655,20 @@ acpi_parse_one_rmrr(struct acpi_dmar_hea
&rmrru->scope, RMRR_TYPE, rmrr->segment);
if ( !ret && (rmrru->scope.devices_cnt != 0) )
- {
ret = register_one_rmrr(rmrru);
- /*
- * register_one_rmrr() returns greater than 0 when a specified
- * PCIe device cannot be detected. To prevent VT-d from being
- * disabled in such cases, reset the return value to 0 here.
- */
- if ( ret > 0 )
- ret = 0;
- }
- else
+ if ( ret )
+ {
+ scope_devices_free(&rmrru->scope);
xfree(rmrru);
+ }
- return ret;
+ /*
+ * register_one_rmrr() returns greater than 0 when a specified PCIe
+ * device cannot be detected. To prevent VT-d from being disabled in
+ * such cases, make the return value 0 here.
+ */
+ return ret > 0 ? 0 : ret;
}
static int __init
@@ -945,9 +939,13 @@ static int __init add_one_user_rmrr(unsi
rmrr->scope.devices_cnt = dev_count;
if ( register_one_rmrr(rmrr) )
+ {
printk(XENLOG_ERR VTDPREFIX
"Could not register RMMR range "ERMRRU_FMT"\n",
ERMRRU_ARG);
+ scope_devices_free(&rmrr->scope);
+ xfree(rmrr);
+ }
return 1;
}
On Thu, Feb 15, 2024 at 11:14:02AM +0100, Jan Beulich wrote:
> It's acpi_parse_one_rmrr() where the allocation is coming from (by way
> of invoking acpi_parse_dev_scope()), or in add_one_user_rmrr()'s case
> allocation is even open-coded there, so freeing would better also happen
> there. Care needs to be taken to preserve acpi_parse_one_rmrr()'s
> ultimate return value.
>
> While fiddling with callers also move scope_devices_free() to .init and
> have it use XFREE() instead of open-coding it.
>
> In register_one_rmrr() also have the "ignore" path take the main
> function return path.
>
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> v2: New.
>
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -82,14 +82,13 @@ static int __init acpi_register_rmrr_uni
> return 0;
> }
>
> -static void scope_devices_free(struct dmar_scope *scope)
> +static void __init scope_devices_free(struct dmar_scope *scope)
> {
> if ( !scope )
> return;
>
> scope->devices_cnt = 0;
> - xfree(scope->devices);
> - scope->devices = NULL;
> + XFREE(scope->devices);
> }
>
> static void __init disable_all_dmar_units(void)
> @@ -595,17 +594,13 @@ static int register_one_rmrr(struct acpi
register_one_rmrr() could also be made __init AFAICT? (even before
this patch)
Thanks, Roger.
On 06.05.2024 11:12, Roger Pau Monné wrote:
> On Thu, Feb 15, 2024 at 11:14:02AM +0100, Jan Beulich wrote:
>> It's acpi_parse_one_rmrr() where the allocation is coming from (by way
>> of invoking acpi_parse_dev_scope()), or in add_one_user_rmrr()'s case
>> allocation is even open-coded there, so freeing would better also happen
>> there. Care needs to be taken to preserve acpi_parse_one_rmrr()'s
>> ultimate return value.
>>
>> While fiddling with callers also move scope_devices_free() to .init and
>> have it use XFREE() instead of open-coding it.
>>
>> In register_one_rmrr() also have the "ignore" path take the main
>> function return path.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks.
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -82,14 +82,13 @@ static int __init acpi_register_rmrr_uni
>> return 0;
>> }
>>
>> -static void scope_devices_free(struct dmar_scope *scope)
>> +static void __init scope_devices_free(struct dmar_scope *scope)
>> {
>> if ( !scope )
>> return;
>>
>> scope->devices_cnt = 0;
>> - xfree(scope->devices);
>> - scope->devices = NULL;
>> + XFREE(scope->devices);
>> }
>>
>> static void __init disable_all_dmar_units(void)
>> @@ -595,17 +594,13 @@ static int register_one_rmrr(struct acpi
>
> register_one_rmrr() could also be made __init AFAICT? (even before
> this patch)
Indeed, all the more when it calls acpi_register_rmrr_unit(), which is
__init. With scope_devices_free() becoming __init here, it would seem
quite logical to fold that adjustment right into here. I'll do so,
unless you'd indicate that this would then invalidate your R-b.
Jan
On Mon, May 06, 2024 at 11:21:06AM +0200, Jan Beulich wrote:
> On 06.05.2024 11:12, Roger Pau Monné wrote:
> > On Thu, Feb 15, 2024 at 11:14:02AM +0100, Jan Beulich wrote:
> >> It's acpi_parse_one_rmrr() where the allocation is coming from (by way
> >> of invoking acpi_parse_dev_scope()), or in add_one_user_rmrr()'s case
> >> allocation is even open-coded there, so freeing would better also happen
> >> there. Care needs to be taken to preserve acpi_parse_one_rmrr()'s
> >> ultimate return value.
> >>
> >> While fiddling with callers also move scope_devices_free() to .init and
> >> have it use XFREE() instead of open-coding it.
> >>
> >> In register_one_rmrr() also have the "ignore" path take the main
> >> function return path.
> >>
> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Thanks.
>
> >> --- a/xen/drivers/passthrough/vtd/dmar.c
> >> +++ b/xen/drivers/passthrough/vtd/dmar.c
> >> @@ -82,14 +82,13 @@ static int __init acpi_register_rmrr_uni
> >> return 0;
> >> }
> >>
> >> -static void scope_devices_free(struct dmar_scope *scope)
> >> +static void __init scope_devices_free(struct dmar_scope *scope)
> >> {
> >> if ( !scope )
> >> return;
> >>
> >> scope->devices_cnt = 0;
> >> - xfree(scope->devices);
> >> - scope->devices = NULL;
> >> + XFREE(scope->devices);
> >> }
> >>
> >> static void __init disable_all_dmar_units(void)
> >> @@ -595,17 +594,13 @@ static int register_one_rmrr(struct acpi
> >
> > register_one_rmrr() could also be made __init AFAICT? (even before
> > this patch)
>
> Indeed, all the more when it calls acpi_register_rmrr_unit(), which is
> __init. With scope_devices_free() becoming __init here, it would seem
> quite logical to fold that adjustment right into here. I'll do so,
> unless you'd indicate that this would then invalidate your R-b.
Sure, feel free to fold here.
Thanks, Roger.
© 2016 - 2026 Red Hat, Inc.