| Message ID | ecee2217151efd08b2bae58166efcdd319ec82c8.1664458360.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On Thu, Sep 29, 2022 at 03:33:12PM +0200, Marek Marczykowski-Górecki wrote: > Re-use rmrr= parameter handling code to handle common device reserved > memory. > > Move MAX_USER_RMRR_PAGES limit enforcement to apply only to > user-configured ranges, but not those from internal callers. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Henry, can this be included in 4.17? The AMD counterpart went in earlier, but due to late review on Intel part, this one didn't. > --- > Changes in v8: > - move add_one_user_rmrr() function earlier > - extend commit message > Changes in v3: > - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR > --- > xen/drivers/passthrough/vtd/dmar.c | 196 +++++++++++++++++------------- > 1 file changed, 114 insertions(+), 82 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c > index 367304c8739c..78c8bad1515a 100644 > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -861,113 +861,136 @@ 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_user_rmrr(void) > +/* 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 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; > } > > +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); > +} > + > +static int __init add_extra_rmrr(void) > +{ > + return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL); > +} > + > #include <asm/tboot.h> > /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */ > /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */ > @@ -1010,7 +1033,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 +1131,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 >
Hi Marek, > -----Original Message----- > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Subject: Re: [PATCH v8 1/2] IOMMU/VT-d: wire common device reserved > memory API > > On Thu, Sep 29, 2022 at 03:33:12PM +0200, Marek Marczykowski-Górecki > wrote: > > Re-use rmrr= parameter handling code to handle common device reserved > > memory. > > > > Move MAX_USER_RMRR_PAGES limit enforcement to apply only to > > user-configured ranges, but not those from internal callers. > > > > Signed-off-by: Marek Marczykowski-Górecki > <marmarek@invisiblethingslab.com> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Henry, can this be included in 4.17? The AMD counterpart went in > earlier, but due to late review on Intel part, this one didn't. Thanks for the information. I agree this is a valid reason, but to be safe I would like to hear opinions from the x86 maintainers (added in CC). Andrew/Jan/Roger: May I have your feedback about this? Thanks! Kind regards, Henry > > > --- > > Changes in v8: > > - move add_one_user_rmrr() function earlier > > - extend commit message > > Changes in v3: > > - make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR > > --- > > xen/drivers/passthrough/vtd/dmar.c | 196 +++++++++++++++++------------- > > 1 file changed, 114 insertions(+), 82 deletions(-) > > > > diff --git a/xen/drivers/passthrough/vtd/dmar.c > b/xen/drivers/passthrough/vtd/dmar.c > > index 367304c8739c..78c8bad1515a 100644 > > --- a/xen/drivers/passthrough/vtd/dmar.c > > +++ b/xen/drivers/passthrough/vtd/dmar.c > > @@ -861,113 +861,136 @@ 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_user_rmrr(void) > > +/* 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 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; > > } > > > > +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); > > +} > > + > > +static int __init add_extra_rmrr(void) > > +{ > > + return > iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL); > > +} > > + > > #include <asm/tboot.h> > > /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */ > > /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */ > > @@ -1010,7 +1033,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 +1131,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 > > > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab
On 17.10.2022 03:20, Henry Wang wrote: >> -----Original Message----- >> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> >> On Thu, Sep 29, 2022 at 03:33:12PM +0200, Marek Marczykowski-Górecki >> wrote: >>> Re-use rmrr= parameter handling code to handle common device reserved >>> memory. >>> >>> Move MAX_USER_RMRR_PAGES limit enforcement to apply only to >>> user-configured ranges, but not those from internal callers. >>> >>> Signed-off-by: Marek Marczykowski-Górecki >> <marmarek@invisiblethingslab.com> >>> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >> >> Henry, can this be included in 4.17? The AMD counterpart went in >> earlier, but due to late review on Intel part, this one didn't. > > Thanks for the information. I agree this is a valid reason, but to be > safe I would like to hear opinions from the x86 maintainers (added > in CC). > > Andrew/Jan/Roger: May I have your feedback about this? Thanks! Hmm, not sure what to say here. Yes, it would be nice for things to end up consistent across vendors. And yes, the change here is largely mechanical (afaics) and to code most of which shouldn't typically be in use on systems anyway, and so should not pose an undue risk. But still it is quite a bit of code churn ... Jan
On 17.10.2022 10:55, Jan Beulich wrote: > On 17.10.2022 03:20, Henry Wang wrote: >>> -----Original Message----- >>> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>> >>> On Thu, Sep 29, 2022 at 03:33:12PM +0200, Marek Marczykowski-Górecki >>> wrote: >>>> Re-use rmrr= parameter handling code to handle common device reserved >>>> memory. >>>> >>>> Move MAX_USER_RMRR_PAGES limit enforcement to apply only to >>>> user-configured ranges, but not those from internal callers. >>>> >>>> Signed-off-by: Marek Marczykowski-Górecki >>> <marmarek@invisiblethingslab.com> >>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >>> >>> Henry, can this be included in 4.17? The AMD counterpart went in >>> earlier, but due to late review on Intel part, this one didn't. >> >> Thanks for the information. I agree this is a valid reason, but to be >> safe I would like to hear opinions from the x86 maintainers (added >> in CC). >> >> Andrew/Jan/Roger: May I have your feedback about this? Thanks! > > Hmm, not sure what to say here. Yes, it would be nice for things to end > up consistent across vendors. And yes, the change here is largely > mechanical (afaics) and to code most of which shouldn't typically be in > use on systems anyway, and so should not pose an undue risk. But still > it is quite a bit of code churn ... Was this lost, did you decide against allowing this in, or were you hoping for further responses by others? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Subject: Re: [PATCH v8 1/2] IOMMU/VT-d: wire common device reserved > memory API > > >>>> Signed-off-by: Marek Marczykowski-Górecki > >>> <marmarek@invisiblethingslab.com> > >>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > >>> > >>> Henry, can this be included in 4.17? The AMD counterpart went in > >>> earlier, but due to late review on Intel part, this one didn't. > >> > >> Thanks for the information. I agree this is a valid reason, but to be > >> safe I would like to hear opinions from the x86 maintainers (added > >> in CC). > >> > >> Andrew/Jan/Roger: May I have your feedback about this? Thanks! > > > > Hmm, not sure what to say here. Yes, it would be nice for things to end > > up consistent across vendors. And yes, the change here is largely > > mechanical (afaics) and to code most of which shouldn't typically be in > > use on systems anyway, and so should not pose an undue risk. But still > > it is quite a bit of code churn ... > > Was this lost, did you decide against allowing this in, or were you hoping > for further responses by others? Sorry for the confusion. Yeah I was hoping to see if we can have further responses from others, but it seems no responses so far... I have the exact same opinion as yours so I am also not sure. But if you changed your mind and would like to commit the patch for completeness of the original series, please feel free to add my release-ack. I would not block this patch. Kind regards, Henry > > Jan
On Thu, Nov 03, 2022 at 02:55:31AM +0000, Henry Wang wrote: > Hi Jan, > > > -----Original Message----- > > From: Jan Beulich <jbeulich@suse.com> > > Subject: Re: [PATCH v8 1/2] IOMMU/VT-d: wire common device reserved > > memory API > > > > >>>> Signed-off-by: Marek Marczykowski-Górecki > > >>> <marmarek@invisiblethingslab.com> > > >>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > >>> > > >>> Henry, can this be included in 4.17? The AMD counterpart went in > > >>> earlier, but due to late review on Intel part, this one didn't. > > >> > > >> Thanks for the information. I agree this is a valid reason, but to be > > >> safe I would like to hear opinions from the x86 maintainers (added > > >> in CC). > > >> > > >> Andrew/Jan/Roger: May I have your feedback about this? Thanks! > > > > > > Hmm, not sure what to say here. Yes, it would be nice for things to end > > > up consistent across vendors. And yes, the change here is largely > > > mechanical (afaics) and to code most of which shouldn't typically be in > > > use on systems anyway, and so should not pose an undue risk. But still > > > it is quite a bit of code churn ... > > > > Was this lost, did you decide against allowing this in, or were you hoping > > for further responses by others? > > Sorry for the confusion. Yeah I was hoping to see if we can have further > responses from others, but it seems no responses so far... > > I have the exact same opinion as yours so I am also not sure. But if you > changed your mind and would like to commit the patch for completeness > of the original series, please feel free to add my release-ack. I would not > block this patch. FWIW, most of the diff is just extracting loop body into a function, the only functional difference is a new called for this function, and moving one of the checks (MAX_USER_RMRR_PAGES enforcement) outside of it. So, my (biased) opinion is, it's rather low risk.
Hi Marek, > -----Original Message----- > > FWIW, most of the diff is just extracting loop body into a function, the > only functional difference is a new called for this function, and moving > one of the checks (MAX_USER_RMRR_PAGES enforcement) outside of it. So, > my (biased) opinion is, it's rather low risk. I think yesterday Jan has committed this patch :) Kind regards, Henry > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 367304c8739c..78c8bad1515a 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -861,113 +861,136 @@ 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_user_rmrr(void) +/* 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 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; } +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); +} + +static int __init add_extra_rmrr(void) +{ + return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL); +} + #include <asm/tboot.h> /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */ /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */ @@ -1010,7 +1033,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 +1131,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;