Re: [Xen-devel] [PATCH v10 5/5] iommu: add rmrr Xen command line option for extra rmrrs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.07.15 at 20:18, <elena.ufimtseva@xxxxxxxxxx> 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

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