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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.01.17 at 23:57, <venu.busireddy@xxxxxxxxxx> wrote:
> Changes in v13:
>  - Implement feedback from Kevin Tian.
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html 
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html 
>    https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html 

Any reason some of the review comments I had given were left
un-addressed? I'll reproduce them in quotes below.

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -859,6 +859,132 @@ 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

So you've kept "extra" in these, but ...

> +struct user_rmrr {

... switched to "user" here and below. Please be consistent.

> +static int __init add_user_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrr, *rmrru;
> +    unsigned int idx, seg, i;
> +    unsigned long base, end;
> +    bool overlap;
> +
> +    for ( i = 0; i < nr_rmrr; i++ )
> +    {
> +        base = user_rmrrs[i].base_pfn;
> +        end = user_rmrrs[i].end_pfn;
> +
> +        if ( base > end )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Invalid RMRR Range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            continue;
> +        }
> +
> +        if ( (end - base) >= MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range "ERMRRU_FMT" exceeds "\
> +                   __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            continue;
> +        }
> +
> +        overlap = false;
> +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> +        {
> +            if ( pfn_to_paddr(base) < rmrru->end_address &&
> +                 rmrru->base_address < pfn_to_paddr(end + 1) )

"Aren't both ranges inclusive? I.e. shouldn't the first one be <= (and
 the second one could be <= too when dropping the +1), matching
 the check acpi_parse_one_rmrr() does?"

> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
> +                       ERMRRU_ARG(user_rmrrs[i]),
> +                       paddr_to_pfn(rmrru->base_address),
> +                       paddr_to_pfn(rmrru->end_address));
> +                overlap = true;
> +                break;
> +            }
> +        }
> +        /* Don't add overlapping RMRR. */
> +        if ( overlap )
> +            continue;
> +
> +        do
> +        {
> +            if ( !mfn_valid(base) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
> +                       ERMRRU_ARG(user_rmrrs[i]));
> +                break;
> +            }
> +        } while ( base++ < end );
> +
> +        /* Invalid pfn in range as the loop ended before end_pfn was 
> reached. */
> +        if ( base <= end )
> +            continue;
> +
> +        rmrr = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrr )
> +            return -ENOMEM;
> +
> +        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
> +        if ( !rmrr->scope.devices )
> +        {
> +            xfree(rmrr);
> +            return -ENOMEM;
> +        }
> +
> +        seg = 0;
> +        for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ )
> +        {
> +            rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx];
> +            seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]);
> +        }
> +        if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            scope_devices_free(&rmrr->scope);
> +            xfree(rmrr);
> +            continue;
> +        }
> +
> +        rmrr->segment = seg;
> +        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
> +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn + 1);

"And this seems wrong too, unless I'm mistaken with the inclusive-ness."

> +        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
> +
> +        if ( register_one_rmrr(rmrr) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Could not register RMMR range "ERMRRU_FMT"\n",
> +                   ERMRRU_ARG(user_rmrrs[i]));
> +            scope_devices_free(&rmrr->scope);
> +            xfree(rmrr);
> +        }
> +    }
> +    return 0;

Blank line please before a function's final return statement.

Jan

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