On Thu, Jan 12, 2017 at 04:44:42AM -0700, Jan Beulich wrote:
> >>> On 10.01.17 at 23:57, <[email protected]> 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
[email protected]
https://lists.xen.org/xen-devel
