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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.15 at 23:14, <elena.ufimtseva@xxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1185,6 +1185,18 @@ Specify the host reboot method.
>  'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by
>   default it will use that method first).
>  
> +### rmrr
> +> '= 
> start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
> +
> +Define RMRRs units that are missing from ACPI table along with device they

"RMRR units" or "RMRRs"

> +belong to and use them for 1:1 mapping. End addresses can be omitted and one
> +page will be mapped. The ranges are inclusive when start and end are 
> specified.
> +If segment of the first device is not specified, segment zero will be used.
> +If other segments are not specified, first device segment will be used.
> +If segments are specified for every device and not equal, an error will be 
> reported.

This isn't precise enough: "If a segment is specified for other than
the first device and it doesn't match that specified for the first one,
..."

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -869,6 +869,120 @@ 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;
> +    u16    dev_count;

Pointless (afaict) use of fixed width type - use "unsigned int" instead.

> +    u32    sbdf[MAX_EXTRA_RMRR_DEV];
> +};
> +static __initdata unsigned int nr_rmrr;
> +static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR];
> +
> +#define PRI_RMRRL "[%lx - %lx]"

If you do this, then you also want a macro for the paired arguments.
Otherwise the number of format specifiers and the number of value
to be formatted will appear to disagree to the reader.

Further please use usual mathematical range representation, i.e.
[x,y] for inclusive ranges and [x,y) for ranges inclusive at the
beginning, but not inclusive at their end. And avoid spaces here,
they're not really helpful but eat serial line bandwidth.

Also, what's the L standing for?

> +static void __init add_extra_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *acpi_rmrr;
> +    unsigned int dev, seg, i, j;
> +    unsigned long pfn;
> +
> +    for ( i = 0; i < nr_rmrr; i++ )
> +    {
> +        if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Start pfn > end pfn for RMRR range "PRI_RMRRL"\n",
> +                   extra_rmrr_units[i].base_pfn, 
> extra_rmrr_units[i].end_pfn);

How about just "Invalid RMRR range ..."?

> +            continue;
> +        }
> +
> +        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >= 
> MAX_EXTRA_RMRR_PAGES )

Long line.

> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range exceeds %s pages 
> "PRI_RMRRL"\n",__stringify(MAX_EXTRA_RMRR_PAGES),

You don't need %s for a string literal - simply embed the literal just
like you also embed PRI_RMRRL. And please print the range after the
word "range", not at the end of the message.

> +                   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_RMRRL" and "PRI_RMRRL"\n",
> +                      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;

Now this only checks for overlaps amongst the extra RMRRs, while
you should also check for overlaps with ACPI provided ones.

> +        for ( pfn = extra_rmrr_units[i].base_pfn; pfn <= 
> extra_rmrr_units[i].end_pfn; pfn++ )
> +        {
> +            if ( !mfn_valid(pfn) )
> +            {
> +                if ( iommu_verbose )
> +                    printk(XENLOG_ERR VTDPREFIX
> +                           "Invalid mfn in RMRR range "PRI_RMRRL"\n",
> +                           extra_rmrr_units[i].base_pfn, 
> extra_rmrr_units[i].end_pfn);
> +                break;
> +            }
> +        }

Even if an admin error, could you please avoid making this an infinite
loop when .end_pfn is ~0UL? I.e.

        pfn = ...;
        do {
                ...
        } while ( pfn++ < ... );

Or, perhaps even better, reject PFNs out of range for PADDR_BITS.

> +        /* The range had invalid mfn as the loop was broken out before 
> reaching end_pfn. */
> +        if ( pfn <= extra_rmrr_units[i].end_pfn )
> +            continue;
> +
> +        acpi_rmrr = xzalloc(struct acpi_rmrr_unit);
> +        if ( !acpi_rmrr )
> +            return;
> +
> +        acpi_rmrr->scope.devices = xmalloc_array(u16,
> +                                                 
> extra_rmrr_units[i].dev_count);
> +        if ( !acpi_rmrr->scope.devices )
> +        {
> +            xfree(acpi_rmrr);
> +            return;
> +        }
> +
> +        seg = 0;
> +        for ( dev = 0; dev < extra_rmrr_units[i].dev_count; dev++ )
> +        {
> +            acpi_rmrr->scope.devices[dev] = extra_rmrr_units[i].sbdf[dev];
> +            seg = seg | PCI_SEG(extra_rmrr_units[i].sbdf[dev]);
> +        }
> +        if ( seg != PCI_SEG(extra_rmrr_units[i].sbdf[0]) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Segments are not equal for RMRR range "PRI_RMRRL"\n",
> +                   extra_rmrr_units[i].base_pfn, 
> extra_rmrr_units[i].end_pfn);
> +            scope_devices_free(&acpi_rmrr->scope);
> +            xfree(acpi_rmrr);
> +            continue;
> +        }
> +
> +        acpi_rmrr->segment = seg;
> +        acpi_rmrr->base_address = extra_rmrr_units[i].base_pfn << PAGE_SHIFT;
> +        acpi_rmrr->end_address = (extra_rmrr_units[i].end_pfn + 1) << 
> PAGE_SHIFT;

pfn_to_paddr()

> +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 > 0 && default_segment )

Using > 0 here and ...

> +                seg = PCI_SEG(extra_rmrr_units[nr_rmrr].sbdf[0]);
> +
> +            /* Keep sbdf's even if they differ and later report error. */
> +            
> extra_rmrr_units[nr_rmrr].sbdf[extra_rmrr_units[nr_rmrr].dev_count] = 
> PCI_SBDF(seg, bus, dev, func);
> +            extra_rmrr_units[nr_rmrr].dev_count++;
> +            s = stmp;
> +        } while ( (*s == ',' || *s || !s) &&

This looks broken: The first condition is redundant with the second
one, and !s implies that you'll crash on the next iteration.

> +                  extra_rmrr_units[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV );
> +
> +        if ( extra_rmrr_units[nr_rmrr].dev_count )

no comparison operator here - please be consistent (personally I
prefer the latter).

Jan

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