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);
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.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
> 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>
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
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
© 2016 - 2026 Red Hat, Inc.