Hi Marek,
> -----Original Message-----
> From: Marek Marczykowski-Górecki <marma...@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
> <marma...@invisiblethingslab.com>
> > Reviewed-by: Kevin Tian <kevin.t...@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
