| Message ID | 902f7007a679c5850bee43b1347b159e1f5eeb16.1663383053.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On Sat, Sep 17, 2022 at 04:51:27AM +0200, Marek Marczykowski-Górecki wrote: > Re-use rmrr= parameter handling code to handle common device reserved > memory. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Kevin, can you please review this patch? It's unchanged since v3, and pending review for some moths already. > --- > Changes in v3: > - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR > --- > xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------- > 1 file changed, 119 insertions(+), 82 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c > index 367304c8739c..3df5f6b69719 100644 > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -861,111 +861,139 @@ static struct user_rmrr __initdata user_rmrrs[MAX_USER_RMRR]; > > /* Macro for RMRR inclusive range formatting. */ > #define ERMRRU_FMT "[%lx-%lx]" > -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn > +#define ERMRRU_ARG base_pfn, end_pfn > + > +static int __init add_one_user_rmrr(unsigned long base_pfn, > + unsigned long end_pfn, > + unsigned int dev_count, > + uint32_t *sbdf); > > static int __init add_user_rmrr(void) > { > + unsigned int i; > + int ret; > + > + for ( i = 0; i < nr_rmrr; i++ ) > + { > + ret = add_one_user_rmrr(user_rmrrs[i].base_pfn, > + user_rmrrs[i].end_pfn, > + user_rmrrs[i].dev_count, > + user_rmrrs[i].sbdf); > + if ( ret < 0 ) > + return ret; > + } > + return 0; > +} > + > +/* Returns 1 on success, 0 when ignoring and < 0 on error. */ > +static int __init add_one_user_rmrr(unsigned long base_pfn, > + unsigned long end_pfn, > + unsigned int dev_count, > + uint32_t *sbdf) > +{ > struct acpi_rmrr_unit *rmrr, *rmrru; > - unsigned int idx, seg, i; > - unsigned long base, end; > + unsigned int idx, seg; > + unsigned long base_iter; > bool overlap; > > - for ( i = 0; i < nr_rmrr; i++ ) > + if ( iommu_verbose ) > + printk(XENLOG_DEBUG VTDPREFIX > + "Adding RMRR for %d device ([0]: %#x) range "ERMRRU_FMT"\n", > + dev_count, sbdf[0], ERMRRU_ARG); > + > + if ( base_pfn > end_pfn ) > { > - base = user_rmrrs[i].base_pfn; > - end = user_rmrrs[i].end_pfn; > + printk(XENLOG_ERR VTDPREFIX > + "Invalid RMRR Range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + return 0; > + } > > - if ( base > end ) > + overlap = false; > + list_for_each_entry(rmrru, &acpi_rmrr_units, list) > + { > + if ( pfn_to_paddr(base_pfn) <= rmrru->end_address && > + rmrru->base_address <= pfn_to_paddr(end_pfn) ) > { > printk(XENLOG_ERR VTDPREFIX > - "Invalid RMRR Range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - continue; > + "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", > + ERMRRU_ARG, > + paddr_to_pfn(rmrru->base_address), > + paddr_to_pfn(rmrru->end_address)); > + overlap = true; > + break; > } > + } > + /* Don't add overlapping RMRR. */ > + if ( overlap ) > + return 0; > > - if ( (end - base) >= MAX_USER_RMRR_PAGES ) > + base_iter = base_pfn; > + do > + { > + if ( !mfn_valid(_mfn(base_iter)) ) > { > printk(XENLOG_ERR VTDPREFIX > - "RMRR range "ERMRRU_FMT" exceeds "\ > - __stringify(MAX_USER_RMRR_PAGES)" pages\n", > - ERMRRU_ARG(user_rmrrs[i])); > - continue; > + "Invalid pfn in RMRR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + break; > } > + } while ( base_iter++ < end_pfn ); > > - overlap = false; > - list_for_each_entry(rmrru, &acpi_rmrr_units, list) > - { > - if ( pfn_to_paddr(base) <= rmrru->end_address && > - rmrru->base_address <= pfn_to_paddr(end) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", > - ERMRRU_ARG(user_rmrrs[i]), > - paddr_to_pfn(rmrru->base_address), > - paddr_to_pfn(rmrru->end_address)); > - overlap = true; > - break; > - } > - } > - /* Don't add overlapping RMRR. */ > - if ( overlap ) > - continue; > + /* Invalid pfn in range as the loop ended before end_pfn was reached. */ > + if ( base_iter <= end_pfn ) > + return 0; > > - do > - { > - if ( !mfn_valid(_mfn(base)) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Invalid pfn in RMRR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - break; > - } > - } while ( base++ < end ); > + rmrr = xzalloc(struct acpi_rmrr_unit); > + if ( !rmrr ) > + return -ENOMEM; > > - /* Invalid pfn in range as the loop ended before end_pfn was reached. */ > - if ( base <= end ) > - continue; > + rmrr->scope.devices = xmalloc_array(u16, dev_count); > + if ( !rmrr->scope.devices ) > + { > + xfree(rmrr); > + return -ENOMEM; > + } > > - rmrr = xzalloc(struct acpi_rmrr_unit); > - if ( !rmrr ) > - return -ENOMEM; > + seg = 0; > + for ( idx = 0; idx < dev_count; idx++ ) > + { > + rmrr->scope.devices[idx] = sbdf[idx]; > + seg |= PCI_SEG(sbdf[idx]); > + } > + if ( seg != PCI_SEG(sbdf[0]) ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Segments are not equal for RMRR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + scope_devices_free(&rmrr->scope); > + xfree(rmrr); > + return 0; > + } > > - rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count); > - if ( !rmrr->scope.devices ) > - { > - xfree(rmrr); > - return -ENOMEM; > - } > + rmrr->segment = seg; > + rmrr->base_address = pfn_to_paddr(base_pfn); > + /* Align the end_address to the end of the page */ > + rmrr->end_address = pfn_to_paddr(end_pfn) | ~PAGE_MASK; > + rmrr->scope.devices_cnt = dev_count; > > - seg = 0; > - for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ ) > - { > - rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx]; > - seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]); > - } > - if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Segments are not equal for RMRR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - scope_devices_free(&rmrr->scope); > - xfree(rmrr); > - continue; > - } > + if ( register_one_rmrr(rmrr) ) > + printk(XENLOG_ERR VTDPREFIX > + "Could not register RMMR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > > - rmrr->segment = seg; > - rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn); > - /* Align the end_address to the end of the page */ > - rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | ~PAGE_MASK; > - rmrr->scope.devices_cnt = user_rmrrs[i].dev_count; > + return 1; > +} > > - if ( register_one_rmrr(rmrr) ) > - printk(XENLOG_ERR VTDPREFIX > - "Could not register RMMR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - } > +static int __init cf_check add_one_extra_rmrr(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt) > +{ > + u32 sbdf_array[] = { id }; > + return add_one_user_rmrr(start, start+nr, 1, sbdf_array); > +} > > - return 0; > +static int __init add_extra_rmrr(void) > +{ > + return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL); > } > > #include <asm/tboot.h> > @@ -1010,7 +1038,7 @@ int __init acpi_dmar_init(void) > { > iommu_init_ops = &intel_iommu_init_ops; > > - return add_user_rmrr(); > + return add_user_rmrr() || add_extra_rmrr(); > } > > return ret; > @@ -1108,6 +1136,15 @@ static int __init cf_check parse_rmrr_param(const char *str) > else > end = start; > > + if ( (end - start) >= MAX_USER_RMRR_PAGES ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "RMRR range "ERMRRU_FMT" exceeds "\ > + __stringify(MAX_USER_RMRR_PAGES)" pages\n", > + start, end); > + return -E2BIG; > + } > + > user_rmrrs[nr_rmrr].base_pfn = start; > user_rmrrs[nr_rmrr].end_pfn = end; > > -- > git-series 0.9.1
> From: Marek Marczykowski-Górecki > Sent: Saturday, September 17, 2022 10:51 AM > > Re-use rmrr= parameter handling code to handle common device reserved > memory. > > Signed-off-by: Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> > --- > Changes in v3: > - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR > --- > xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------- > 1 file changed, 119 insertions(+), 82 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/dmar.c > b/xen/drivers/passthrough/vtd/dmar.c > index 367304c8739c..3df5f6b69719 100644 > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -861,111 +861,139 @@ static struct user_rmrr __initdata > user_rmrrs[MAX_USER_RMRR]; > > /* Macro for RMRR inclusive range formatting. */ > #define ERMRRU_FMT "[%lx-%lx]" > -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn > +#define ERMRRU_ARG base_pfn, end_pfn > + > +static int __init add_one_user_rmrr(unsigned long base_pfn, > + unsigned long end_pfn, > + unsigned int dev_count, > + uint32_t *sbdf); Just move the function here then no need of a declaration. > > static int __init add_user_rmrr(void) > { > + unsigned int i; > + int ret; > + > + for ( i = 0; i < nr_rmrr; i++ ) > + { > + ret = add_one_user_rmrr(user_rmrrs[i].base_pfn, > + user_rmrrs[i].end_pfn, > + user_rmrrs[i].dev_count, > + user_rmrrs[i].sbdf); > + if ( ret < 0 ) > + return ret; > + } > + return 0; > +} > + > +/* Returns 1 on success, 0 when ignoring and < 0 on error. */ I'm not sure the point of 'ignoring'. If user specifies a wrong RMRR range (overlap, mfn invalid, etc.) just return an error similar to -ENOMEM. Ignoring a user-specified range implies that something would potentially get broken hence better fail it early. > +static int __init add_one_user_rmrr(unsigned long base_pfn, > + unsigned long end_pfn, > + unsigned int dev_count, > + uint32_t *sbdf) > +{ > struct acpi_rmrr_unit *rmrr, *rmrru; > - unsigned int idx, seg, i; > - unsigned long base, end; > + unsigned int idx, seg; > + unsigned long base_iter; > bool overlap; > > - for ( i = 0; i < nr_rmrr; i++ ) > + if ( iommu_verbose ) > + printk(XENLOG_DEBUG VTDPREFIX > + "Adding RMRR for %d device ([0]: %#x) range "ERMRRU_FMT"\n", > + dev_count, sbdf[0], ERMRRU_ARG); > + > + if ( base_pfn > end_pfn ) > { > - base = user_rmrrs[i].base_pfn; > - end = user_rmrrs[i].end_pfn; > + printk(XENLOG_ERR VTDPREFIX > + "Invalid RMRR Range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + return 0; > + } > > - if ( base > end ) > + overlap = false; > + list_for_each_entry(rmrru, &acpi_rmrr_units, list) > + { > + if ( pfn_to_paddr(base_pfn) <= rmrru->end_address && > + rmrru->base_address <= pfn_to_paddr(end_pfn) ) > { > printk(XENLOG_ERR VTDPREFIX > - "Invalid RMRR Range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - continue; > + "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", > + ERMRRU_ARG, > + paddr_to_pfn(rmrru->base_address), > + paddr_to_pfn(rmrru->end_address)); > + overlap = true; > + break; > } > + } > + /* Don't add overlapping RMRR. */ > + if ( overlap ) > + return 0; > > - if ( (end - base) >= MAX_USER_RMRR_PAGES ) > + base_iter = base_pfn; > + do > + { > + if ( !mfn_valid(_mfn(base_iter)) ) > { > printk(XENLOG_ERR VTDPREFIX > - "RMRR range "ERMRRU_FMT" exceeds "\ > - __stringify(MAX_USER_RMRR_PAGES)" pages\n", > - ERMRRU_ARG(user_rmrrs[i])); > - continue; > + "Invalid pfn in RMRR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + break; > } > + } while ( base_iter++ < end_pfn ); > > - overlap = false; > - list_for_each_entry(rmrru, &acpi_rmrr_units, list) > - { > - if ( pfn_to_paddr(base) <= rmrru->end_address && > - rmrru->base_address <= pfn_to_paddr(end) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", > - ERMRRU_ARG(user_rmrrs[i]), > - paddr_to_pfn(rmrru->base_address), > - paddr_to_pfn(rmrru->end_address)); > - overlap = true; > - break; > - } > - } > - /* Don't add overlapping RMRR. */ > - if ( overlap ) > - continue; > + /* Invalid pfn in range as the loop ended before end_pfn was reached. */ > + if ( base_iter <= end_pfn ) > + return 0; > > - do > - { > - if ( !mfn_valid(_mfn(base)) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Invalid pfn in RMRR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - break; > - } > - } while ( base++ < end ); > + rmrr = xzalloc(struct acpi_rmrr_unit); > + if ( !rmrr ) > + return -ENOMEM; > > - /* Invalid pfn in range as the loop ended before end_pfn was reached. > */ > - if ( base <= end ) > - continue; > + rmrr->scope.devices = xmalloc_array(u16, dev_count); > + if ( !rmrr->scope.devices ) > + { > + xfree(rmrr); > + return -ENOMEM; > + } > > - rmrr = xzalloc(struct acpi_rmrr_unit); > - if ( !rmrr ) > - return -ENOMEM; > + seg = 0; > + for ( idx = 0; idx < dev_count; idx++ ) > + { > + rmrr->scope.devices[idx] = sbdf[idx]; > + seg |= PCI_SEG(sbdf[idx]); > + } > + if ( seg != PCI_SEG(sbdf[0]) ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Segments are not equal for RMRR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > + scope_devices_free(&rmrr->scope); > + xfree(rmrr); > + return 0; > + } > > - rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count); > - if ( !rmrr->scope.devices ) > - { > - xfree(rmrr); > - return -ENOMEM; > - } > + rmrr->segment = seg; > + rmrr->base_address = pfn_to_paddr(base_pfn); > + /* Align the end_address to the end of the page */ > + rmrr->end_address = pfn_to_paddr(end_pfn) | ~PAGE_MASK; > + rmrr->scope.devices_cnt = dev_count; > > - seg = 0; > - for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ ) > - { > - rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx]; > - seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]); > - } > - if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) ) > - { > - printk(XENLOG_ERR VTDPREFIX > - "Segments are not equal for RMRR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - scope_devices_free(&rmrr->scope); > - xfree(rmrr); > - continue; > - } > + if ( register_one_rmrr(rmrr) ) > + printk(XENLOG_ERR VTDPREFIX > + "Could not register RMMR range "ERMRRU_FMT"\n", > + ERMRRU_ARG); > > - rmrr->segment = seg; > - rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn); > - /* Align the end_address to the end of the page */ > - rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | > ~PAGE_MASK; > - rmrr->scope.devices_cnt = user_rmrrs[i].dev_count; > + return 1; > +} > > - if ( register_one_rmrr(rmrr) ) > - printk(XENLOG_ERR VTDPREFIX > - "Could not register RMMR range "ERMRRU_FMT"\n", > - ERMRRU_ARG(user_rmrrs[i])); > - } > +static int __init cf_check add_one_extra_rmrr(xen_pfn_t start, xen_ulong_t > nr, u32 id, void *ctxt) > +{ > + u32 sbdf_array[] = { id }; > + return add_one_user_rmrr(start, start+nr, 1, sbdf_array); > +} > > - return 0; > +static int __init add_extra_rmrr(void) > +{ > + return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, > NULL); > } > > #include <asm/tboot.h> > @@ -1010,7 +1038,7 @@ int __init acpi_dmar_init(void) > { > iommu_init_ops = &intel_iommu_init_ops; > > - return add_user_rmrr(); > + return add_user_rmrr() || add_extra_rmrr(); > } > > return ret; > @@ -1108,6 +1136,15 @@ static int __init cf_check parse_rmrr_param(const > char *str) > else > end = start; > > + if ( (end - start) >= MAX_USER_RMRR_PAGES ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "RMRR range "ERMRRU_FMT" exceeds "\ > + __stringify(MAX_USER_RMRR_PAGES)" pages\n", > + start, end); > + return -E2BIG; > + } > + why moving this error check out of add_one_user_rmrr()? I didn't get why it's special from other checks there, e.g. having base>end... > user_rmrrs[nr_rmrr].base_pfn = start; > user_rmrrs[nr_rmrr].end_pfn = end; > > -- > git-series 0.9.1
On Fri, Sep 23, 2022 at 07:21:04AM +0000, Tian, Kevin wrote: > > From: Marek Marczykowski-Górecki > > Sent: Saturday, September 17, 2022 10:51 AM > > > > Re-use rmrr= parameter handling code to handle common device reserved > > memory. > > > > Signed-off-by: Marek Marczykowski-Górecki > > <marmarek@invisiblethingslab.com> > > --- > > Changes in v3: > > - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR > > --- > > xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------- > > 1 file changed, 119 insertions(+), 82 deletions(-) > > > > diff --git a/xen/drivers/passthrough/vtd/dmar.c > > b/xen/drivers/passthrough/vtd/dmar.c > > index 367304c8739c..3df5f6b69719 100644 > > --- a/xen/drivers/passthrough/vtd/dmar.c > > +++ b/xen/drivers/passthrough/vtd/dmar.c > > @@ -861,111 +861,139 @@ static struct user_rmrr __initdata > > user_rmrrs[MAX_USER_RMRR]; > > > > /* Macro for RMRR inclusive range formatting. */ > > #define ERMRRU_FMT "[%lx-%lx]" > > -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn > > +#define ERMRRU_ARG base_pfn, end_pfn > > + > > +static int __init add_one_user_rmrr(unsigned long base_pfn, > > + unsigned long end_pfn, > > + unsigned int dev_count, > > + uint32_t *sbdf); > > Just move the function here then no need of a declaration. Ok. > > > > static int __init add_user_rmrr(void) > > { > > + unsigned int i; > > + int ret; > > + > > + for ( i = 0; i < nr_rmrr; i++ ) > > + { > > + ret = add_one_user_rmrr(user_rmrrs[i].base_pfn, > > + user_rmrrs[i].end_pfn, > > + user_rmrrs[i].dev_count, > > + user_rmrrs[i].sbdf); > > + if ( ret < 0 ) > > + return ret; > > + } > > + return 0; > > +} > > + > > +/* Returns 1 on success, 0 when ignoring and < 0 on error. */ > > I'm not sure the point of 'ignoring'. If user specifies a wrong RMRR > range (overlap, mfn invalid, etc.) just return an error similar to > -ENOMEM. Ignoring a user-specified range implies that something > would potentially get broken hence better fail it early. That's the behaviour that was here before, I simply added a comment about it explicitly (previously it used 'continue' heavily, now it's a separate function so it's a return value). While I agree in principle, I don't think such change should be part of this patch. (...) > > @@ -1108,6 +1136,15 @@ static int __init cf_check parse_rmrr_param(const > > char *str) > > else > > end = start; > > > > + if ( (end - start) >= MAX_USER_RMRR_PAGES ) > > + { > > + printk(XENLOG_ERR VTDPREFIX > > + "RMRR range "ERMRRU_FMT" exceeds "\ > > + __stringify(MAX_USER_RMRR_PAGES)" pages\n", > > + start, end); > > + return -E2BIG; > > + } > > + > > why moving this error check out of add_one_user_rmrr()? I didn't > get why it's special from other checks there, e.g. having base>end... To allow internal callers bypass MAX_USER_RMRR_PAGES, and make it apply really only to user-provided ranges.
> From: Marczykowski, Marek <marmarek@invisiblethingslab.com> > Sent: Tuesday, September 27, 2022 7:54 AM > > On Fri, Sep 23, 2022 at 07:21:04AM +0000, Tian, Kevin wrote: > > > From: Marek Marczykowski-Górecki > > > Sent: Saturday, September 17, 2022 10:51 AM > > > > > > Re-use rmrr= parameter handling code to handle common device > reserved > > > memory. > > > > > > Signed-off-by: Marek Marczykowski-Górecki > > > <marmarek@invisiblethingslab.com> > > > --- > > > Changes in v3: > > > - make MAX_USER_RMRR_PAGES applicable only to user-configured > RMRR > > > --- > > > xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------ > - > > > 1 file changed, 119 insertions(+), 82 deletions(-) > > > > > > diff --git a/xen/drivers/passthrough/vtd/dmar.c > > > b/xen/drivers/passthrough/vtd/dmar.c > > > index 367304c8739c..3df5f6b69719 100644 > > > --- a/xen/drivers/passthrough/vtd/dmar.c > > > +++ b/xen/drivers/passthrough/vtd/dmar.c > > > @@ -861,111 +861,139 @@ static struct user_rmrr __initdata > > > user_rmrrs[MAX_USER_RMRR]; > > > > > > /* Macro for RMRR inclusive range formatting. */ > > > #define ERMRRU_FMT "[%lx-%lx]" > > > -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn > > > +#define ERMRRU_ARG base_pfn, end_pfn > > > + > > > +static int __init add_one_user_rmrr(unsigned long base_pfn, > > > + unsigned long end_pfn, > > > + unsigned int dev_count, > > > + uint32_t *sbdf); > > > > Just move the function here then no need of a declaration. > > Ok. > > > > > > > static int __init add_user_rmrr(void) > > > { > > > + unsigned int i; > > > + int ret; > > > + > > > + for ( i = 0; i < nr_rmrr; i++ ) > > > + { > > > + ret = add_one_user_rmrr(user_rmrrs[i].base_pfn, > > > + user_rmrrs[i].end_pfn, > > > + user_rmrrs[i].dev_count, > > > + user_rmrrs[i].sbdf); > > > + if ( ret < 0 ) > > > + return ret; > > > + } > > > + return 0; > > > +} > > > + > > > +/* Returns 1 on success, 0 when ignoring and < 0 on error. */ > > > > I'm not sure the point of 'ignoring'. If user specifies a wrong RMRR > > range (overlap, mfn invalid, etc.) just return an error similar to > > -ENOMEM. Ignoring a user-specified range implies that something > > would potentially get broken hence better fail it early. > > That's the behaviour that was here before, I simply added a comment > about it explicitly (previously it used 'continue' heavily, now it's a > separate function so it's a return value). > While I agree in principle, I don't think such change should be part of > this patch. > > (...) > > > > @@ -1108,6 +1136,15 @@ static int __init cf_check > parse_rmrr_param(const > > > char *str) > > > else > > > end = start; > > > > > > + if ( (end - start) >= MAX_USER_RMRR_PAGES ) > > > + { > > > + printk(XENLOG_ERR VTDPREFIX > > > + "RMRR range "ERMRRU_FMT" exceeds "\ > > > + __stringify(MAX_USER_RMRR_PAGES)" pages\n", > > > + start, end); > > > + return -E2BIG; > > > + } > > > + > > > > why moving this error check out of add_one_user_rmrr()? I didn't > > get why it's special from other checks there, e.g. having base>end... > > To allow internal callers bypass MAX_USER_RMRR_PAGES, and make it apply > really only to user-provided ranges. > With above clarification and order adjustment, Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 367304c8739c..3df5f6b69719 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -861,111 +861,139 @@ static struct user_rmrr __initdata user_rmrrs[MAX_USER_RMRR]; /* Macro for RMRR inclusive range formatting. */ #define ERMRRU_FMT "[%lx-%lx]" -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn +#define ERMRRU_ARG base_pfn, end_pfn + +static int __init add_one_user_rmrr(unsigned long base_pfn, + unsigned long end_pfn, + unsigned int dev_count, + uint32_t *sbdf); static int __init add_user_rmrr(void) { + unsigned int i; + int ret; + + for ( i = 0; i < nr_rmrr; i++ ) + { + ret = add_one_user_rmrr(user_rmrrs[i].base_pfn, + user_rmrrs[i].end_pfn, + user_rmrrs[i].dev_count, + user_rmrrs[i].sbdf); + if ( ret < 0 ) + return ret; + } + return 0; +} + +/* Returns 1 on success, 0 when ignoring and < 0 on error. */ +static int __init add_one_user_rmrr(unsigned long base_pfn, + unsigned long end_pfn, + unsigned int dev_count, + uint32_t *sbdf) +{ struct acpi_rmrr_unit *rmrr, *rmrru; - unsigned int idx, seg, i; - unsigned long base, end; + unsigned int idx, seg; + unsigned long base_iter; bool overlap; - for ( i = 0; i < nr_rmrr; i++ ) + if ( iommu_verbose ) + printk(XENLOG_DEBUG VTDPREFIX + "Adding RMRR for %d device ([0]: %#x) range "ERMRRU_FMT"\n", + dev_count, sbdf[0], ERMRRU_ARG); + + if ( base_pfn > end_pfn ) { - base = user_rmrrs[i].base_pfn; - end = user_rmrrs[i].end_pfn; + printk(XENLOG_ERR VTDPREFIX + "Invalid RMRR Range "ERMRRU_FMT"\n", + ERMRRU_ARG); + return 0; + } - if ( base > end ) + overlap = false; + list_for_each_entry(rmrru, &acpi_rmrr_units, list) + { + if ( pfn_to_paddr(base_pfn) <= rmrru->end_address && + rmrru->base_address <= pfn_to_paddr(end_pfn) ) { printk(XENLOG_ERR VTDPREFIX - "Invalid RMRR Range "ERMRRU_FMT"\n", - ERMRRU_ARG(user_rmrrs[i])); - continue; + "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", + ERMRRU_ARG, + paddr_to_pfn(rmrru->base_address), + paddr_to_pfn(rmrru->end_address)); + overlap = true; + break; } + } + /* Don't add overlapping RMRR. */ + if ( overlap ) + return 0; - if ( (end - base) >= MAX_USER_RMRR_PAGES ) + base_iter = base_pfn; + do + { + if ( !mfn_valid(_mfn(base_iter)) ) { printk(XENLOG_ERR VTDPREFIX - "RMRR range "ERMRRU_FMT" exceeds "\ - __stringify(MAX_USER_RMRR_PAGES)" pages\n", - ERMRRU_ARG(user_rmrrs[i])); - continue; + "Invalid pfn in RMRR range "ERMRRU_FMT"\n", + ERMRRU_ARG); + break; } + } while ( base_iter++ < end_pfn ); - overlap = false; - list_for_each_entry(rmrru, &acpi_rmrr_units, list) - { - if ( pfn_to_paddr(base) <= rmrru->end_address && - rmrru->base_address <= pfn_to_paddr(end) ) - { - printk(XENLOG_ERR VTDPREFIX - "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n", - ERMRRU_ARG(user_rmrrs[i]), - paddr_to_pfn(rmrru->base_address), - paddr_to_pfn(rmrru->end_address)); - overlap = true; - break; - } - } - /* Don't add overlapping RMRR. */ - if ( overlap ) - continue; + /* Invalid pfn in range as the loop ended before end_pfn was reached. */ + if ( base_iter <= end_pfn ) + return 0; - do - { - if ( !mfn_valid(_mfn(base)) ) - { - printk(XENLOG_ERR VTDPREFIX - "Invalid pfn in RMRR range "ERMRRU_FMT"\n", - ERMRRU_ARG(user_rmrrs[i])); - break; - } - } while ( base++ < end ); + rmrr = xzalloc(struct acpi_rmrr_unit); + if ( !rmrr ) + return -ENOMEM; - /* Invalid pfn in range as the loop ended before end_pfn was reached. */ - if ( base <= end ) - continue; + rmrr->scope.devices = xmalloc_array(u16, dev_count); + if ( !rmrr->scope.devices ) + { + xfree(rmrr); + return -ENOMEM; + } - rmrr = xzalloc(struct acpi_rmrr_unit); - if ( !rmrr ) - return -ENOMEM; + seg = 0; + for ( idx = 0; idx < dev_count; idx++ ) + { + rmrr->scope.devices[idx] = sbdf[idx]; + seg |= PCI_SEG(sbdf[idx]); + } + if ( seg != PCI_SEG(sbdf[0]) ) + { + printk(XENLOG_ERR VTDPREFIX + "Segments are not equal for RMRR range "ERMRRU_FMT"\n", + ERMRRU_ARG); + scope_devices_free(&rmrr->scope); + xfree(rmrr); + return 0; + } - rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count); - if ( !rmrr->scope.devices ) - { - xfree(rmrr); - return -ENOMEM; - } + rmrr->segment = seg; + rmrr->base_address = pfn_to_paddr(base_pfn); + /* Align the end_address to the end of the page */ + rmrr->end_address = pfn_to_paddr(end_pfn) | ~PAGE_MASK; + rmrr->scope.devices_cnt = dev_count; - seg = 0; - for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ ) - { - rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx]; - seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]); - } - if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) ) - { - printk(XENLOG_ERR VTDPREFIX - "Segments are not equal for RMRR range "ERMRRU_FMT"\n", - ERMRRU_ARG(user_rmrrs[i])); - scope_devices_free(&rmrr->scope); - xfree(rmrr); - continue; - } + if ( register_one_rmrr(rmrr) ) + printk(XENLOG_ERR VTDPREFIX + "Could not register RMMR range "ERMRRU_FMT"\n", + ERMRRU_ARG); - rmrr->segment = seg; - rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn); - /* Align the end_address to the end of the page */ - rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | ~PAGE_MASK; - rmrr->scope.devices_cnt = user_rmrrs[i].dev_count; + return 1; +} - if ( register_one_rmrr(rmrr) ) - printk(XENLOG_ERR VTDPREFIX - "Could not register RMMR range "ERMRRU_FMT"\n", - ERMRRU_ARG(user_rmrrs[i])); - } +static int __init cf_check add_one_extra_rmrr(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt) +{ + u32 sbdf_array[] = { id }; + return add_one_user_rmrr(start, start+nr, 1, sbdf_array); +} - return 0; +static int __init add_extra_rmrr(void) +{ + return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL); } #include <asm/tboot.h> @@ -1010,7 +1038,7 @@ int __init acpi_dmar_init(void) { iommu_init_ops = &intel_iommu_init_ops; - return add_user_rmrr(); + return add_user_rmrr() || add_extra_rmrr(); } return ret; @@ -1108,6 +1136,15 @@ static int __init cf_check parse_rmrr_param(const char *str) else end = start; + if ( (end - start) >= MAX_USER_RMRR_PAGES ) + { + printk(XENLOG_ERR VTDPREFIX + "RMRR range "ERMRRU_FMT" exceeds "\ + __stringify(MAX_USER_RMRR_PAGES)" pages\n", + start, end); + return -E2BIG; + } + user_rmrrs[nr_rmrr].base_pfn = start; user_rmrrs[nr_rmrr].end_pfn = end;
Re-use rmrr= parameter handling code to handle common device reserved memory. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v3: - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR --- xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------- 1 file changed, 119 insertions(+), 82 deletions(-)