| Message ID | 97577ebf-3871-47be-97c5-8164701be756@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT-d: SATC handling; ATS: tidying | expand |
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.
--- 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; }
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.