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

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
> > 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.

Kevin, on the second thought, I think to support multiple devices 
per one rmrr one need to put on command line same address/range and
specify unique device each time. 

The specified region will me mapped within iommu only once if start end
end are the same (and will just increase the count of total mappings).
Taking into account that it will be rarely used, we may want to skip
adding more code?
 
> 
> > +
> > +        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