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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.05.15 at 23:38, <elena.ufimtseva@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -866,6 +866,106 @@ 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;
> +    u32    sbdf[MAX_EXTRA_RMRR_DEV];
> +};
> +static __initdata unsigned int nr_rmrr;
> +static struct __initdata extra_rmrr_unit rmrru[MAX_EXTRA_RMRR];
> +
> +static void __init add_extra_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    unsigned int dev, seg, i, j;
> +    unsigned long pfn;
> +
> +    for ( i = 0; i < nr_rmrr; i++ )
> +    {
> +        if ( rmrru[i].base_pfn > rmrru[i].end_pfn )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Start pfn > end pfn for RMRR range [%"PRIx64" - 
> %"PRIx64"]\n",
> +                   rmrru[i].base_pfn, rmrru[i].end_pfn);

PRIx64 doesn't match "unsigned long" (more below).

> +            return;
> +        }
> +
> +        if ( rmrru[i].end_pfn - rmrru[i].base_pfn >= MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range exceeds 16 pages [%"PRIx64" - %"PRIx64"]\n",

ITYM __stringify(MAX_EXTRA_RMRR_PAGES) instead of 16.

> +                   rmrru[i].base_pfn, rmrru[i].end_pfn);
> +            return;
> +        }
> +
> +        for ( j = 0; j < nr_rmrr; j++ )
> +        {
> +            if ( i != j && rmrru[i].base_pfn <= rmrru[j].end_pfn &&
> +                 rmrru[j].base_pfn <= rmrru[i].end_pfn )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                      "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and 
> [%"PRIx64",%"PRIx64"]\n",
> +                      rmrru[i].base_pfn, rmrru[i].end_pfn,
> +                      rmrru[j].base_pfn, rmrru[j].end_pfn);
> +               return;
> +            }
> +        }
> +
> +        for ( pfn = rmrru[i].base_pfn; pfn <= rmrru[i].end_pfn; pfn++ )
> +        {
> +            if ( !mfn_valid(pfn) )
> +                if ( iommu_verbose )
> +                    printk(XENLOG_ERR VTDPREFIX
> +                           "Invalid mfn in RMRR range [%"PRIx64" - 
> %"PRIx64"]\n",
> +                           rmrru[i].base_pfn, rmrru[i].end_pfn);
> +            return;
> +        }

The lack of braces around the outer if()'s body, with the return
needing to be inside, suggests that this version of the patch
wasn't tested at all.

And as Kevin also already suggested on an earlier code block -
please consider whether "return" is really the right thing if any
of these checks fails.

> @@ -885,7 +986,9 @@ int __init acpi_dmar_init(void)
>          dmar_table = __va(dmar_addr);
>      }
>  
> -    return parse_dmar_table(acpi_parse_dmar);
> +    ret = parse_dmar_table(acpi_parse_dmar);
> +    add_extra_rmrr();
> +    return ret;
>  }

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

> @@ -916,3 +1019,62 @@ int platform_supports_x2apic(void)
>      unsigned int mask = ACPI_DMAR_INTR_REMAP | ACPI_DMAR_X2APIC_OPT_OUT;
>      return cpu_has_x2apic && ((dmar_flags & mask) == ACPI_DMAR_INTR_REMAP);
>  }
> +
> +/*
> + * Parse rmrr Xen command line options and add parsed
> + * device and region into apci_rmrr_unit list to mapped
> + * as RMRRs parsed from ACPI.
> + * Format 
> rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
> + * If the segement of the first device is not specified, the segment zero 
> will be used.

The typo is still there (and also in the command line doc). Please be
sure to address comments on earlier versions before sending out new
ones.

> +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 {
> +        bool_t default_segment = 0;
> +        start = simple_strtoul(cur = s, &s, 0);

Blank line between declarations and statements please. And this is,
afaict, still not correct - it needs to go in the inner loop, as it needs
to be initialized in each of that one's iterations.

Jan

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