From: "Jan Beulich" <JBeulich@suse.com>
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
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 [thread overview]
Message-ID: <55A504050200007800090947@mail.emea.novell.com> (raw)
In-Reply-To: <1436811482-16113-6-git-send-email-elena.ufimtseva@oracle.com>
>>> On 13.07.15 at 20:18, <elena.ufimtseva@oracle.com> 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
next prev parent reply other threads:[~2015-07-14 10:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-13 18:17 [PATCH v10 0/5] iommu: add rmrr Xen command line option elena.ufimtseva
2015-07-13 18:17 ` [PATCH v10 1/5] dmar: device scope mem leak fix elena.ufimtseva
2015-07-13 18:17 ` [PATCH v10 2/5] iommu VT-d: separate rmrr addition function elena.ufimtseva
2015-07-13 18:18 ` [PATCH v10 3/5] pci: add wrapper for parse_pci elena.ufimtseva
2015-07-13 18:18 ` [PATCH v10 4/5] pci: add PCI_SBDF and PCI_SEG macros elena.ufimtseva
2015-07-13 18:18 ` [PATCH v10 5/5] iommu: add rmrr Xen command line option for extra rmrrs elena.ufimtseva
2015-07-14 10:43 ` Jan Beulich [this message]
2015-07-15 7:25 ` Jan Beulich
2015-07-15 15:27 ` Elena Ufimtseva
2015-07-15 16:08 ` Jan Beulich
2015-07-14 10:18 ` [PATCH v10 0/5] iommu: add rmrr Xen command line option Jan Beulich
-- strict thread matches above, loose matches on Subject: below --
2015-07-15 16:15 [PATCH v10 5/5] iommu: add rmrr Xen command line option for extra rmrrs Elena Ufimtseva
2015-07-16 8:02 ` Jan Beulich
2015-07-16 16:00 Elena Ufimtseva
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A504050200007800090947@mail.emea.novell.com \
--to=jbeulich@suse.com \
--cc=boris.ostrovsky@oracle.com \
--cc=elena.ufimtseva@oracle.com \
--cc=kevin.tian@intel.com \
--cc=tim@xen.org \
--cc=xen-devel@lists.xen.org \
--cc=yang.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.