Re: [PATCH v13 3/3] iommu: add rmrr Xen command line option for extra rmrrs

[Elena Ufimtseva <elena.ufimtseva@oracle.com>]

On Thu, Jan 12, 2017 at 04:44:42AM -0700, Jan Beulich wrote:
> >>> On 10.01.17 at 23:57, <venu.busireddy@oracle.com> wrote:
> > Changes in v13:
> >  - Implement feedback from Kevin Tian.
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html 
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html 
> >    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html 
> 
> Any reason some of the review comments I had given were left
> un-addressed? I'll reproduce them in quotes below.
>

Hi Jan

Thanks for reminding!
That was my fault that I did not tell this to Venu when transferring
this patchset to him.
 
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -859,6 +859,132 @@ out:
> >      return ret;
> >  }
> >  
> > +#define MAX_EXTRA_RMRR_PAGES 16
> > +#define MAX_EXTRA_RMRR 10
> > +
> > +/* RMRR units derived from command line rmrr option. */
> > +#define MAX_EXTRA_RMRR_DEV 20
> 
> So you've kept "extra" in these, but ...
> 
> > +struct user_rmrr {
> 
> ... switched to "user" here and below. Please be consistent.
> 
> > +static int __init add_user_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrr, *rmrru;
> > +    unsigned int idx, seg, i;
> > +    unsigned long base, end;
> > +    bool overlap;
> > +
> > +    for ( i = 0; i < nr_rmrr; i++ )
> > +    {
> > +        base = user_rmrrs[i].base_pfn;
> > +        end = user_rmrrs[i].end_pfn;
> > +
> > +        if ( base > end )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Invalid RMRR Range "ERMRRU_FMT"\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            continue;
> > +        }
> > +
> > +        if ( (end - base) >= MAX_EXTRA_RMRR_PAGES )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "RMRR range "ERMRRU_FMT" exceeds "\
> > +                   __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            continue;
> > +        }
> > +
> > +        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 + 1) )
> 
> "Aren't both ranges inclusive? I.e. shouldn't the first one be <= (and
>  the second one could be <= too when dropping the +1), matching
>  the check acpi_parse_one_rmrr() does?"

I agree. The ranges in acpu_rmrr_units and user_rmrrs are inclusive.
If this is fixed, then there is another part where I am not sure what
would be the better way to fix this. If fix is needed.

I am looking at rmrr_identity_mapping where the RMRR paddr get converted
to pfn and then mapped with iommu.
If ( rmrr->end_address & ~PAGE_SHIFT_MASK_4K ) == 0, the while loop
    while ( base_pfn < end_pfn )
 will not map that inclusive end_address of rmrr.
Does it seem wrong?


> 
> > +            {
> > +                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;
> > +
> > +        do
> > +        {
> > +            if ( !mfn_valid(base) )
> > +            {
> > +                printk(XENLOG_ERR VTDPREFIX
> > +                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> > +                       ERMRRU_ARG(user_rmrrs[i]));
> > +                break;
> > +            }
> > +        } while ( base++ < end );
> > +
> > +        /* Invalid pfn in range as the loop ended before end_pfn was reached. */
> > +        if ( base <= end )
> > +            continue;
> > +
> > +        rmrr = xzalloc(struct acpi_rmrr_unit);
> > +        if ( !rmrr )
> > +            return -ENOMEM;
> > +
> > +        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
> > +        if ( !rmrr->scope.devices )
> > +        {
> > +            xfree(rmrr);
> > +            return -ENOMEM;
> > +        }
> > +
> > +        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;
> > +        }
> > +
> > +        rmrr->segment = seg;
> > +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
> > +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);
> 
> "And this seems wrong too, unless I'm mistaken with the inclusive-ness."
> 
This one is the avoidance of the while loop mapping in
rmrr_identity_mapping.

> > +        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
> > +
> > +        if ( register_one_rmrr(rmrr) )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Could not register RMMR range "ERMRRU_FMT"\n",
> > +                   ERMRRU_ARG(user_rmrrs[i]));
> > +            scope_devices_free(&rmrr->scope);
> > +            xfree(rmrr);
> > +        }
> > +    }
> > +    return 0;
> 
> Blank line please before a function's final return statement.
> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel