From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v10 5/5] iommu: add rmrr Xen command line option for extra rmrrs Date: Tue, 14 Jul 2015 11:43:49 +0100 Message-ID: <55A504050200007800090947@mail.emea.novell.com> References: <1436811482-16113-1-git-send-email-elena.ufimtseva@oracle.com> <1436811482-16113-6-git-send-email-elena.ufimtseva@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436811482-16113-6-git-send-email-elena.ufimtseva@oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: elena.ufimtseva@oracle.com Cc: kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org, yang.z.zhang@intel.com, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org >>> On 13.07.15 at 20:18, wrote: > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -867,6 +867,145 @@ 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 > +struct extra_rmrr_unit { > + struct list_head list; > + unsigned long base_pfn, end_pfn; > + unsigned int dev_count; > + u32 sbdf[MAX_EXTRA_RMRR_DEV]; unless you want to align _all_ fields' names, there should be just a single space between type and name. > +}; > +static __initdata unsigned int nr_rmrr; > +static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR]; > + > +/* Macro for RMRR inclusive range formatting. */ > +#define PRI_RMRR(s,e) "[%lx-%lx]" Just PRI_RMRR (i.e. no parens or parameters) please. And I'm still missing a macro to pair the respective arguments - as said before, as single format specifier should be accompanied by a single argument (as visible to the reader at the use sites). > +static void __init add_extra_rmrr(void) > +{ > + struct acpi_rmrr_unit *acpi_rmrr; > + struct acpi_rmrr_unit *rmrru; > + unsigned int dev, seg, i, j; > + unsigned long pfn; > + bool_t overlap; > + > + for ( i = 0; i < nr_rmrr; i++ ) > + { > + if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Invalid RMRR Range "PRI_RMRR(s,e)"\n", > + extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn); > + continue; > + } > + > + if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >= > + MAX_EXTRA_RMRR_PAGES ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "RMRR range "PRI_RMRR(s,e)" exceeds "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n", > + extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn); > + continue; > + } > + > + for ( j = 0; j < nr_rmrr; j++ ) > + { > + if ( i != j && > + extra_rmrr_units[i].base_pfn <= extra_rmrr_units[j].end_pfn && > + extra_rmrr_units[j].base_pfn <= extra_rmrr_units[i].end_pfn ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n", No "extra" here ... > + extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn, > + extra_rmrr_units[j].base_pfn, extra_rmrr_units[j].end_pfn); > + break; > + } > + } > + /* Broke out of the overlap loop check, continue with next rmrr. */ > + if ( j < nr_rmrr ) > + continue; > + overlap = 0; > + list_for_each_entry(rmrru, &acpi_rmrr_units, list) > + { > + if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn <= rmrru->end_address) && > + rmrru->base_address <= pfn_to_paddr(extra_rmrr_units[i].end_pfn) ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Overlapping extra RMRRs "PRI_RMRR(s,e)" and ACPI RMRRs "PRI_RMRR(s,e)"\n", ... but here? This may end up being confusing... Also s/RMRRs/RMRR/g here please. Also note the misplaced closing parenthesis on the first line of the enclosing if(). > + extra_rmrr_units[i].base_pfn, > + extra_rmrr_units[i].end_pfn, > + paddr_to_pfn(rmrru->base_address), > + paddr_to_pfn(rmrru->end_address)); > + overlap = 1; > + break; > + } > + } > + /* Continue to next RMRR is this one overlaps with one from ACPI. */ > + if ( overlap ) > + continue; > + > + pfn = extra_rmrr_units[i].base_pfn; > + do > + { > + if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) ) > + { > + printk(XENLOG_ERR VTDPREFIX > + "Invalid pfn in RMRR range "PRI_RMRR(s,e)"\n", > + extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn); > + break; > + } > + } while ( pfn++ <= extra_rmrr_units[i].end_pfn ); Off by one afaict. > +static void __init parse_rmrr_param(const char *str) > +{ > + const char *s = str, *cur, *stmp; > + unsigned int seg, bus, dev, func; > + unsigned long start, end; > + > + do { > + start = simple_strtoul(cur = s, &s, 0); > + if ( cur == s ) > + break; > + > + if ( *s == '-' ) > + { > + end = simple_strtoul(cur = s + 1, &s, 0); > + if ( cur == s ) > + break; > + } > + else > + end = start; > + > + extra_rmrr_units[nr_rmrr].base_pfn = start; > + extra_rmrr_units[nr_rmrr].end_pfn = end; > + extra_rmrr_units[nr_rmrr].dev_count = 0; > + > + if ( *s != '=' ) > + continue; > + > + do { > + bool_t default_segment = 0; > + > + if ( *s == ';' ) > + break; > + stmp = __parse_pci(s + 1, &seg, &bus, &dev, &func, &default_segment); > + if ( !stmp ) > + break; > + > + /* Not specified segment will be replaced with one from first device. */ > + if ( extra_rmrr_units[nr_rmrr].dev_count && default_segment ) > + seg = PCI_SEG(extra_rmrr_units[nr_rmrr].sbdf[0]); > + > + /* Keep sbdf's even if they differ and later report an error. */ > + extra_rmrr_units[nr_rmrr].sbdf[extra_rmrr_units[nr_rmrr].dev_count] = PCI_SBDF(seg, bus, dev, func); Long line. > + extra_rmrr_units[nr_rmrr].dev_count++; > + s = stmp; > + } while ( (*s == ',' || *s ) && > + extra_rmrr_units[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV ); This condition is _still_ bogus: If *s == ',' then obviously also *s, i.e. the former condition is redundant with the latter. But then I don't think you really mean just *s here, i.e. I'd rather see the latter part dropped. Jan