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

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Mon, Oct 26, 2015 at 07:38:06AM -0600, Jan Beulich wrote:
> >>> 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.
>

Thanks Jan for review.

> 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"?
>
Agree, makes more sense.

> > +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.
Right, thats from the old format parsing code I think.
> 
> > +            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.

Ok, will send with fixes!
Thank you.

Elena
> 
> Jan

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