From mboxrd@z Thu Jan  1 00:00:00 1970
From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Subject: Re: [PATCH 2/2] iommu: add rmrr Xen command line option
 for misc rmrrs
Date: Tue, 10 Mar 2015 02:24:43 -0400
Message-ID: <20150310062443.GA1638@elena.ufimtseva>
References: <1425912177-2890-1-git-send-email-elena.ufimtseva@oracle.com>
	<1425912177-2890-3-git-send-email-elena.ufimtseva@oracle.com>
	<AADFC41AFE54684AB9EE6CBC0274A5D1261BCE1B@SHSMSX101.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D1261BCE1B@SHSMSX101.ccr.corp.intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: andrew.cooper3@citrix.com, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, "jbeulich@suse.com" <jbeulich@suse.com>, "Zhang,
	Yang Z" <yang.z.zhang@intel.com>, "boris.ostrovsky@oracle.com" <boris.ostrovsky@oracle.com>
List-Id: xen-devel@lists.xenproject.org

On Tue, Mar 10, 2015 at 02:47:24AM +0000, Tian, Kevin wrote:
> > From: elena.ufimtseva@oracle.com [mailto:elena.ufimtseva@oracle.com]
> > Sent: Monday, March 09, 2015 10:43 PM
> > 
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > 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, ...], ...

Looks like there are few options here :)

Konrad proposed 
rmrr=[<seg>:]<bus>:<device>.<func>:start-end[,[<seg>:]<bus>:<device>.<func>:start-end,...] 
Andrew also offered similar format to what Konard proposed.
After getting rid of square brackets its pretty much the same format.

Do you think this will work for everyone?
rmrr=start<-end>:[<seg:>]<bus>:<device>.<func>,...
> 
> > 
> > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > ---
> >  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.

It is actually a good point and I think I should support one rmrr with
multiple devices, as ACPI RMRR structure does.

> 
> > +
> > +        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
>