On Thu, Jan 12, 2017 at 04:44:42AM -0700, Jan Beulich wrote: > >>> On 10.01.17 at 23:57, <venu.busire...@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