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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.10.15 at 19:13, <elena.ufimtseva@xxxxxxxxxx> wrote:
> From: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> 
> On some platforms RMRR regions may be not specified in ACPI and thus will not
> be mapped 1:1 in dom0.

I think this may be misleading to readers: It sounds as if there was
the option for RMRRs to not be specified in ACPI tables, while in
fact this is a firmware bug. How about "On some platforms firmware
fails to specify RMRR regions may in ACPI tables, and thus those
regions will not be mapped in dom0 or guests the respective device(s)
get passed through to"?

> +static void __init add_extra_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *acpi_rmrr;
> +    struct acpi_rmrr_unit *rmrru;
> +    unsigned int dev, seg, i;
> +    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 "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(extra_rmrr_units[i]));
> +            continue;
> +        }
> +
> +        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
> +             MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range "ERMRRU_FMT" exceeds 
> "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +                   ERMRRU_ARG(extra_rmrr_units[i]));
> +            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 &&

Stray blank inside the inner parentheses.

> +                 rmrru->base_address < 
> pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx - %lx]\n",

ERMRRU_FMT doesn't have any blanks inside the square brackets,
so I'd suggest the other format to nt have them either.

> +                       ERMRRU_ARG(extra_rmrr_units[i]),
> +                       paddr_to_pfn(rmrru->base_address),
> +                       paddr_to_pfn(rmrru->end_address));
> +                overlap = 1;
> +                break;
> +            }
> +        }
> +        /* Dont add overlapping RMRR */

"Don't" and missing full stop.

> +        if ( overlap )
> +            continue;
> +
> +        pfn = extra_rmrr_units[i].base_pfn;
> +        do
> +        {
> +            if ( !mfn_valid(pfn) || (pfn >> (paddr_bits - PAGE_SHIFT)) )

Actually I think the right side is redundant with the max_pfn check
mfn_valid() does.

> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> +                       ERMRRU_ARG(extra_rmrr_units[i]));
> +            break;

Wrong indentation.

> +            }
> +
> +        } while ( pfn++ < extra_rmrr_units[i].end_pfn );

Stray blank line before the end of the do/while body.

> +
> +        /* Invalid pfn in range as the loop ended before end_pfn was 
> reached. */
> +        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]);

|=

> +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;

The last assignment isn't really needed.

> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            bool_t default_segment = 0;
> +
> +            if ( *s == ';' )
> +                break;

Can this if() ever be true? *s == '=' on the first iteration, and *s == ','
on any subsequent one afaics.

> +            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);

This line for sure is too long.

Jan

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