RE: [PATCH v8 1/2] IOMMU/VT-d: wire common device reserved memory API

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Marek,

> -----Original Message-----
> From: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> 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