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

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Mon, Mar 09, 2015 at 05:16:18PM +0000, Andrew Cooper wrote:
> On 09/03/15 14:42, elena.ufimtseva@xxxxxxxxxx wrote:
> > 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>
> >
> > 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.
> 
> This file is sorted in alphabetic order.  Please keep it in order.
> 
> > +
> > +### 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];
> > +string_param("rmrr", misc_rmrr);
> 
> This wants to be a custom_param, not a string_param.

Andrew, can you please explain why its preferred?
Also, I wont be able to use xen dynamic allocator at this early stage, correct?
> 
> Furthermore, I think it would be better to match the existing ivrs_hpet[
> and ivrs_ioapic[ parameters as closely as possible, seeing as it is the
> same kind of overrides being added to ACPI tables.

Thanks Andrew, looks like preferred way looks like this:
rmrr=start<:end>[sbdf1, sbdf2, ...], ...
 
> 
> ~Andrew
> 
> > +
> >  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;
> > +
> > +        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;
> > +
> > +        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;
> > +
> > +        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_ */
> 
> 

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