Re: [Xen-devel] [PATCH v2 2/2] iommu: add rmrr Xen command line option for misc rmrrs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.15 at 17:08, <elena.ufimtseva@xxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1170,6 +1170,14 @@ 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>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]

> '= start<-end>=sbdf1[,bdf2[,...]];start<-end>=sbdf1[,bdf2[,...]]

Segments can't vary between multiple devices associated with a
single RMRR.

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -49,6 +49,7 @@ static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
>  static struct acpi_table_header *__read_mostly dmar_table;
>  static int __read_mostly dmar_flags;
>  static u64 __read_mostly igd_drhd_address;
> +static void __init add_misc_rmrr(void);

Please simply put the function definition not at the end of the file,
thus avoiding the need for a separate declaration.

> @@ -900,3 +902,115 @@ 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>=sbdf1[,sbdf2[,...]]#start<-end>=sbdf1[,sbdf2[,...]]
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +
> +#define MAX_MISC_RMRR 10
> +__initdata LIST_HEAD(misc_rmrr_units);
> +__initdata unsigned int nr_rmrr = 0;
> +struct __initdata misc_rmrr_unit rmrru[MAX_MISC_RMRR];

All three static. The one zero initializer then also becomes pointless.

> +static void __init parse_rmrr_param(const char *str)
> +{
> +    unsigned int seg, bus, dev, func;
> +    const char *s = str, *cur, *stmp;
> +    unsigned int i = 0, rmrrs = 0;

Afaict you could easily do with just one of these two variables.

> +    u64 start, end;
> +
> +    do {
> +        start = simple_strtoull(cur = s, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == '-' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +        if ( end >= start && rmrrs < MAX_MISC_RMRR )
> +        {
> +            rmrru[i].base_address = start << PAGE_SHIFT;
> +            rmrru[i].end_address = (end + 1) << PAGE_SHIFT;
> +            rmrru[i].dev_count = 0;
> +        }
> +        else
> +        {
> +            printk(XENLOG_WARNING "Bad rmrr: start > end, %"PRIx64" > 
> %"PRIx64"\n",
> +                   start, end);

This is misleading (as we may end up here due to the other half of the
if(). But a message logged from a command line parsing function isn't
really that useful anyway, so perhaps just drop it? Maybe instead
make add_misc_rmrr() more verbose, at least in iommu_verbose mode?
Also if you already validate the range, then please also make sure both
ends are valid physical addresses (may again need to be done in the
other function).

> +            break;
> +        }
> +
> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            if ( rmrru[i].dev_count >= MAX_MISC_RMRR_DEV )
> +                break;
> +            if ( *s == '#' )
> +                break;
> +            seg = bus = dev = func = 0;

Pointless initializers.

> +            stmp = parse_pci(s + 1, &seg, &bus, &dev, &func);
> +            if ( !stmp )
> +                break;
> +            rmrru[i].devices[rmrru[i].dev_count++] = PCI_BDF(bus, dev, func);
> +            rmrru[i].segment = seg;

As said above, this must be parsed only on the first device, or (to
keep the code here simple) verified to match what was specified on
the first one (which may require a small adjustment to parse_pci()
so that you can tell a default segment [currently zero] from one
explicitly specified as such).

> +            s = stmp;
> +        } while ( *s == ',' );
> +
> +        if ( rmrru[i].dev_count ) {

Coding style.

> +static void __init add_misc_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    struct misc_rmrr_unit *rmrru, *r;
> +
> +    list_for_each_entry_safe( rmrru, r, &misc_rmrr_units, list )
> +    {
> +        rmrrn = xzalloc(struct acpi_rmrr_unit);
> +        if ( !rmrrn )
> +            goto del;
> +
> +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));

Why xzalloc() rather than xmalloc()? And why just a single array
element?

> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            goto del;
> +        }
> +        rmrrn->segment = rmrru->segment;
> +        rmrrn->base_address = rmrru->base_address;
> +        rmrrn->end_address = rmrru->end_address;
> +
> +        for (int dev = 0; dev < rmrru->dev_count; dev++)

Coding style again. Also no initializers inside statements please (and
the variable should be of unsigned type).

> +            rmrrn->scope.devices[dev] = rmrru->devices[dev];

Actually - memcpy() for the whole array?

> +
> +        rmrrn->scope.devices_cnt = rmrru->dev_count;
> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +        }
> + del:
> +        list_del(&rmrru->list);

I don't think you need this, and then you also don't need the _safe
iterator.

> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -132,4 +132,15 @@ void disable_pmr(struct iommu *iommu);
>  int is_usb_device(u16 seg, u8 bus, u8 devfn);
>  int is_igd_drhd(struct acpi_drhd_unit *drhd);
>  
> +/*RMRR units derived from command line rmrr option */
> +#define MAX_MISC_RMRR_DEV 20
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    segment;
> +    u16    devices[MAX_MISC_RMRR_DEV];
> +    u16    dev_count;
> +};

This type isn't used outside of dmar.c, so I don't see why it gets
defined in a header.

Jan

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