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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: elena.ufimtseva@xxxxxxxxxx [mailto:elena.ufimtseva@xxxxxxxxxx]
> Sent: Monday, March 09, 2015 10:43 PM
> 
> 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. This
> causes IO Page Faults and prevents dom0 from booting
> in PVH mode.
> New Xen command line option rmrr allows to specify
> such devices and memory regions. These regions are added
> to the list of RMRR defined in ACPI if the device
> is present in system. As a result, additional RMRRs will
> be mapped 1:1 in dom0 with correct permissions.
> 
> Mentioned above problems were discovered during PVH work with
> ThinkCentre M and Dell 5600T. No official documentation
> was found so far in regards to what devices and why cause this.
> Experiments show that ThinkCentre M USB devices with enabled
> debug port generate DMA read transactions to the regions of
> memory marked reserved in host e820 map.
> For Dell 5600T the device and faulting addresses are not found yet.
> 
> For detailed history of the discussion please check following threads:
> http://lists.Xen.org/archives/html/xen-devel/2015-02/msg01724.html
> http://lists.Xen.org/archives/html/xen-devel/2015-01/msg02513.html
> 
> Format for rmrr Xen command line option:
> rmrr=[sbdf]start<:end>,[sbdf]start:<end>

how about sticking to rmrr structure, i.e. 

rmrr=start<:end>[sbdf1, sbdf2, ...], ...

> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown |    7 +++
>  xen/drivers/passthrough/iommu.c     |   86
> +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.c |   33 ++++++++++++++
>  xen/drivers/passthrough/vtd/iommu.h |    1 +
>  xen/include/xen/iommu.h             |    9 ++++
>  5 files changed, 136 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-command-line.markdown
> index bc316be..2e1210f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1392,3 +1392,10 @@ mode.
>  > Default: `true`
> 
>  Permit use of the `xsave/xrstor` instructions.
> +
> +### rmrr
> +> '= [sbdf]start<:end>,[sbdf]start<:end>
> +
> +Define RMRRs units that are missing from ACPI table along with device
> +they belong to and use them for 1:1 mapping. End addresses can be omitted
> +and one page will be mapped.
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index cc12735..b64916e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -55,6 +55,9 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> 
> +static char __initdata misc_rmrr[100];

define a macro.

> +string_param("rmrr", misc_rmrr);
> +
>  DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> 
>  DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
> @@ -67,6 +70,87 @@ static struct keyhandler iommu_p2m_table = {
>      .desc = "dump iommu p2m table"
>  };
> 
> +/*
> + * List of command line defined rmrr units.
> + */
> +__initdata LIST_HEAD(misc_rmrr_units);
> +
> +/*
> + * 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=[sbdf]start<:end>,[sbdf]start:<end>
> + * end address can be ommited and one page will be used
> + * for mapping with start pfn.
> + */
> +static void __init parse_iommu_extra_rmrr(const char *s)
> +{
> +    unsigned int idx = 0, found = 0;
> +    struct misc_rmrr_unit *rmrru;
> +    unsigned int seg, bus, dev, func;
> +    const char *str, *cur;
> +    u64 start, end;
> +
> +    do {
> +        if ( idx >= 10 )
> +            break;

as Konrad pointed out, using 10 and earlier 100 are not readable
and error prone.

> +
> +        if ( *s != '[' )
> +            break;
> +
> +        str = s;
> +        seg = bus = dev = func = 0;
> +        str = parse_pci(str + 1, &seg, &bus, &dev, &func);
> +        if ( !str )
> +        {
> +            str = strchr(s, ']');
> +            if ( !str )
> +                return;
> +        }
> +        else
> +            found = 1;
> +
> +        s = str;
> +        if ( *s != ']' )
> +            return;

better to have some warn message for malformat.

> +
> +        start = simple_strtoull(cur = s + 1, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == ':' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +
> +        if ( !found )
> +            continue;
> +
> +        if ( end >= start )
> +        {
> +            rmrru = xzalloc(struct misc_rmrr_unit);
> +            if ( !rmrru )
> +                return;
> +            rmrru->base_address = start << PAGE_SHIFT;
> +            rmrru->end_address = (end << PAGE_SHIFT) + 1;
> +            rmrru->segment = seg;
> +            rmrru->device = PCI_BDF(bus, dev, func);
> +            list_add(&rmrru->list, &misc_rmrr_units);
> +        }
> +        else
> +        {
> +            printk(XENLOG_WARNING "Bad rmrr: start >
> end, %"PRIx64" > %"PRIx64"\n",
> +                   start, end);
> +            break;
> +        }
> +        idx++;
> +    } while ( *s++ == ',' );
> +}
> +
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> @@ -153,6 +237,8 @@ void __hwdom_init iommu_hwdom_init(struct domain
> *d)
>      if ( !iommu_enabled )
>          return;
> 
> +    parse_iommu_extra_rmrr(misc_rmrr);
> +
>      register_keyhandler('o', &iommu_p2m_table);
>      d->need_iommu = !!iommu_dom0_strict;
>      if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 2e113d7..9cb6e73 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1232,6 +1232,38 @@ static int intel_iommu_domain_init(struct domain
> *d)
>      return 0;
>  }
> 
> +static void 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 free;
> +
> +        rmrrn->scope.devices = xzalloc(typeof(*rmrrn->scope.devices));
> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            goto free;
> +        }
> +        rmrrn->scope.devices_cnt = 1;
> +        rmrrn->segment = rmrru->segment;
> +        rmrrn->scope.devices[0] = rmrru->device;

need handle one-rmrr-multiple-deviecs. even if you don't want
to support it, need capture user attempts at least.

> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +        }
> + free:
> +        list_del(&rmrru->list);
> +        xfree(rmrru);
> +    }
> +}
> +
>  static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
> @@ -1243,6 +1275,7 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
>      }
> 
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
> +    add_misc_rmrr();
>      setup_hwdom_rmrr(d);
> 
>      iommu_flush_all();
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index c3e5181..b3c1b59 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -484,6 +484,7 @@ struct qinval_entry {
>  extern struct list_head acpi_drhd_units;
>  extern struct list_head acpi_rmrr_units;
>  extern struct list_head acpi_ioapic_units;
> +extern struct list_head misc_rmrr_units;
> 
>  struct qi_ctrl {
>      u64 qinval_maddr;  /* queue invalidation page machine address */
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 8eb764a..2c43956 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -191,4 +191,13 @@ DECLARE_PER_CPU(bool_t,
> iommu_dont_flush_iotlb);
>  extern struct spinlock iommu_pt_cleanup_lock;
>  extern struct page_list_head iommu_pt_cleanup_list;
> 
> +/*RMRR units derived from command line rmrr option */
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    segment;
> +    u16    device;
> +};
> +
>  #endif /* _IOMMU_H_ */
> --
> 1.7.10.4


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