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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

. snip..
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index a8e1e5d..fa659a9 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -42,6 +42,8 @@
>  #define MIN_SCOPE_LEN (sizeof(struct acpi_dmar_device_scope) + \
>                         sizeof(struct acpi_dmar_pci_path))
>  
> +#define PRI_RMRR(s,e) "[%lx-%lx]"
> +
>  LIST_HEAD_READ_MOSTLY(acpi_drhd_units);
>  LIST_HEAD_READ_MOSTLY(acpi_rmrr_units);
>  static LIST_HEAD_READ_MOSTLY(acpi_atsr_units);
> @@ -425,7 +427,7 @@ static int __init acpi_parse_dev_scope(
>          default:
>              if ( iommu_verbose )
>                  printk(XENLOG_WARNING VTDPREFIX "Unknown scope type %#x\n",
> -                       acpi_scope->entry_type);
> +                acpi_scope->entry_type);

Stray change? It should be in a seperate patch I think?

>              start += acpi_scope->length;
>              continue;
>          }
> @@ -479,8 +481,7 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>      INIT_LIST_HEAD(&dmaru->ioapic_list);
>      INIT_LIST_HEAD(&dmaru->hpet_list);
>      if ( iommu_verbose )
> -        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n",
> -                dmaru->address);
> +        dprintk(VTDPREFIX, "  dmaru->address = %"PRIx64"\n", dmaru->address);

Ditto.
>  
>      ret = iommu_alloc(dmaru);
>      if ( ret )
> @@ -541,8 +542,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>              if ( !pci_device_detect(drhd->segment, b, d, f) )
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                        " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                        " in this DRHD's scope!\n", drhd->segment, b, d, f);
> +                        " Non-existent device (%04x:%02x:%02x.%u) is reported in this DRHD's scope!\n",
> +                        drhd->segment, b, d, f);

As well - an seperate cleanup patch.
>                  invalid_cnt++;
>              }
>          }
> @@ -553,8 +554,8 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>                   invalid_cnt == dmaru->scope.devices_cnt )
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                    "  Workaround BIOS bug: ignore the DRHD due to all "
> -                    "devices under its scope are not PCI discoverable!\n");
> +                        "  Workaround BIOS bug: ignore the DRHD due to all "
> +                        "devices under its scope are not PCI discoverable!\n");

This one too.
>  
>                  scope_devices_free(&dmaru->scope);
>                  iommu_free(dmaru);
> @@ -563,10 +564,10 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
>              else
>              {
>                  dprintk(XENLOG_WARNING VTDPREFIX,
> -                    "  The DRHD is invalid due to there are devices under "
> -                    "its scope are not PCI discoverable! Pls try option "
> -                    "iommu=force or iommu=workaround_bios_bug if you "
> -                    "really want VT-d\n");
> +                        "  The DRHD is invalid due to there are devices under "
> +                        "its scope are not PCI discoverable! Pls try option "
> +                        "iommu=force or iommu=workaround_bios_bug if you "
> +                        "really want VT-d\n");

And that as well.
>                  ret = -EINVAL;
>              }
>          }
> @@ -604,8 +605,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>          if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
>          {
>              dprintk(XENLOG_WARNING VTDPREFIX,
> -                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> +                    " Non-existent device (%04x:%02x:%02x.%u) is reported in RMRR "PRI_RMRR(s,e)"'s scope!\n",
>                      rmrru->segment, b, d, f,
>                      rmrru->base_address, rmrru->end_address);
>              ignore = 1;
> @@ -620,16 +620,16 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>      if ( ignore )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> +                "  Ignore the RMRR "PRI_RMRR(s,e)" due to "
>                  "devices under its scope are not PCI discoverable!\n",
> -            rmrru->base_address, rmrru->end_address);
> +                rmrru->base_address, rmrru->end_address);
>          scope_devices_free(&rmrru->scope);
>          xfree(rmrru);
>      }
>      else if ( rmrru->base_address > rmrru->end_address )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> +                "  The RMRR "PRI_RMRR(s,e)" is incorrect!\n",
>                  rmrru->base_address, rmrru->end_address);
>          scope_devices_free(&rmrru->scope);
>          xfree(rmrru);
> @@ -639,7 +639,7 @@ static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>      {
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX,
> -                    "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
> +                    "  RMRR region: "PRI_RMRR(s,e)"\n",
>                      rmrru->base_address, rmrru->end_address);
>          acpi_register_rmrr_unit(rmrru);
>      }
> @@ -664,7 +664,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>         if ( base_addr <= rmrru->end_address && rmrru->base_address <= end_addr )
>         {
>             printk(XENLOG_ERR VTDPREFIX
> -                  "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n",
> +                  "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
>                    rmrru->base_address, rmrru->end_address,
>                    base_addr, end_addr);
>             return -EEXIST;
> @@ -678,8 +678,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>           (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "  RMRR address range not in reserved memory "
> -                "base = %"PRIx64" end = %"PRIx64"; "
> +                "  RMRR address range "PRI_RMRR(s,e)" not in reserved memory; "
>                  "iommu_inclusive_mapping=1 parameter may be needed.\n",
>                  base_addr, end_addr);
>      }
> @@ -779,8 +778,7 @@ acpi_parse_one_rhsa(struct acpi_dmar_header *header)
>      list_add_tail(&rhsau->list, &acpi_rhsa_units);
>      if ( iommu_verbose )
>          dprintk(VTDPREFIX,
> -                "  rhsau->address: %"PRIx64
> -                " rhsau->proximity_domain: %"PRIx32"\n",
> +                "  rhsau->address: %"PRIx64" rhsau->proximity_domain: %"PRIx32"\n",

All of those should go in a seperate cleanup patch please.

>                  rhsau->address, rhsau->proximity_domain);
>  
>      return ret;
> @@ -869,6 +867,140 @@ out:
>      return ret;
>  }
>  
> +#define MAX_EXTRA_RMRR_PAGES 16
> +#define MAX_EXTRA_RMRR 10
> +
> +/* RMRR units derived from command line rmrr option */

Missing full stop.

> +#define MAX_EXTRA_RMRR_DEV 20
> +struct extra_rmrr_unit {
> +    struct list_head list;
> +    unsigned long base_pfn, end_pfn;
> +    unsigned int dev_count;
> +    u32    sbdf[MAX_EXTRA_RMRR_DEV];
> +};
> +static __initdata unsigned int nr_rmrr;
> +static struct __initdata extra_rmrr_unit extra_rmrr_units[MAX_EXTRA_RMRR];
> +
> +/* Macro for RMRR inclusive range formatting */

Macro? Ah I think you meant the:
#define PRI_RMRR(s,e) "[%lx-%lx]"

which is defined somewhere at the top of the file with your patch. Please
move it here.

Also, missing full stop at the end of the line.
> +
> +static void __init add_extra_rmrr(void)
> +{
> +    struct acpi_rmrr_unit *acpi_rmrr;
> +    struct acpi_rmrr_unit *rmrru;
> +    unsigned int dev, seg, i, j;
> +    unsigned long pfn;
> +
> +    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 "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            continue;
> +        }
> +
> +        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
> +             MAX_EXTRA_RMRR_PAGES )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "RMRR range "PRI_RMRR(s,e)" exceeds "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            continue;
> +        }
> +
> +        for ( j = 0; j < nr_rmrr; j++ )
> +        {
> +            if ( i != j &&
> +                 extra_rmrr_units[i].base_pfn <= extra_rmrr_units[j].end_pfn &&
> +                 extra_rmrr_units[j].base_pfn <= extra_rmrr_units[i].end_pfn )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                      "Overlapping RMRRs "PRI_RMRR(s,e)" and "PRI_RMRR(s,e)"\n",
> +                      extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn,
> +                      extra_rmrr_units[j].base_pfn, extra_rmrr_units[j].end_pfn);
> +                break;
> +            }
> +        }
> +        /* Broke out of the overlap loop check, continue with next rmrr. */
> +        if ( j < nr_rmrr )
> +            continue;
> +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> +        {
> +            if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn <= rmrru->end_address) &&
> +                 rmrru->base_address <= pfn_to_paddr(extra_rmrr_units[i].end_pfn) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping extra RMRRs "PRI_RMRR(s,e)" and ACPI RMRRs "PRI_RMRR(s,e)"\n",
> +                       extra_rmrr_units[i].base_pfn,
> +                       extra_rmrr_units[i].end_pfn,
> +                       paddr_to_pfn(rmrru->base_address),
> +                       paddr_to_pfn(rmrru->end_address));
> +                break;

You break out of the for loop (list_for_each_entry) - did ..
> +            }
> +        }

... you also want to continue with the next rmrr? Right now the code
will continue on with this 'i'.

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

Why the 'iommu_verbose' ? Why not just print it out as all the other errors
are printed?

> +                    printk(XENLOG_ERR VTDPREFIX
> +                           "Invalid mfn in RMRR range "PRI_RMRR(s,e)"\n",
> +                           extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +                break;
> +            }
> +        } while ( pfn++ <= extra_rmrr_units[i].end_pfn );
> +        /* The range had invalid mfn as the loop was broken out before reaching end_pfn. */
> +        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]);
> +        }
> +        if ( seg != PCI_SEG(extra_rmrr_units[i].sbdf[0]) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Segments are not equal for RMRR range "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            scope_devices_free(&acpi_rmrr->scope);
> +            xfree(acpi_rmrr);
> +            continue;
> +        }
> +
> +        acpi_rmrr->segment = seg;
> +        acpi_rmrr->base_address = pfn_to_paddr(extra_rmrr_units[i].base_pfn);
> +        acpi_rmrr->end_address = pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1);
> +        acpi_rmrr->scope.devices_cnt = extra_rmrr_units[i].dev_count;
> +
> +        if ( register_one_rmrr(acpi_rmrr) )
> +        {
> +            printk(XENLOG_ERR VTDPREFIX
> +                   "Could not register RMMR range "PRI_RMRR(s,e)"\n",
> +                   extra_rmrr_units[i].base_pfn, extra_rmrr_units[i].end_pfn);
> +            scope_devices_free(&acpi_rmrr->scope);
> +            xfree(acpi_rmrr);
> +        }
> +    }
> +}
> +
>  #include <asm/tboot.h>
>  /* ACPI tables may not be DMA protected by tboot, so use DMAR copy */
>  /* SINIT saved in SinitMleData in TXT heap (which is DMA protected) */
> @@ -878,6 +1010,7 @@ int __init acpi_dmar_init(void)
>  {
>      acpi_physical_address dmar_addr;
>      acpi_native_uint dmar_len;
> +    int ret;
>  
>      if ( ACPI_SUCCESS(acpi_get_table_phys(ACPI_SIG_DMAR, 0,
>                                            &dmar_addr, &dmar_len)) )
> @@ -888,7 +1021,10 @@ 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;
>  }
>  
>  void acpi_dmar_reinstate(void)
> @@ -919,3 +1055,67 @@ 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
> + * acpi_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 segment of the first device is not specified, segment zero will be used.
> + * If other segments are not specified, first device segment will be used.
> + * If a segment is specified for other than the first device and it does not match
> + * the one specified for the first one, an error will be reported.
> + */
> +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;
> +
> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            bool_t default_segment = 0;
> +
> +            if ( *s == ';' )
> +                break;
> +            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);
> +            extra_rmrr_units[nr_rmrr].dev_count++;
> +            s = stmp;
> +        } while ( *s == ',' &&
> +                  extra_rmrr_units[nr_rmrr].dev_count < MAX_EXTRA_RMRR_DEV );
> +
> +        if ( extra_rmrr_units[nr_rmrr].dev_count )
> +            nr_rmrr++;
> +
> +    } while ( *s++ == ';' && nr_rmrr < MAX_EXTRA_RMRR );
> +}
> +custom_param("rmrr", parse_rmrr_param);


Otherwise looks fine!
> -- 
> 2.1.3
>