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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Mar 09, 2015 at 10:42:57AM -0400, 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

ThinkCentre M93p

> 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.
> +
> +### rmrr
> +> '= [sbdf]start<:end>,[sbdf]start<:end>

You are missing an ` at the end.

Hm, the syntax is interesting. Usually '[' and ']' denote
an optional field (see 'console'). But in your case you require it.

I am not actually sure how to say in a man page that '[' is
to not be optinal.

Perhaps instead of '[]' you could use '()' ? Or just require it
and change the syntax:

 
[<seg>:]<bus>:<device>.<func>:start-end[,[<seg>:]<bus>:<device>.<func>:start-end,...]

?
(And of course the code too)

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

 * <sbdf> is the PCI location of the device in 
[<segment>:]<bus>:<device>.<function>
   notation. 

A typical setup might be [00:1a.f]
> 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);
> +
>  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 )

How come 10?

Perhaps swithc 'idx' to a different counter - to the amount of 
characters you have touched - so that you can make sure that
you won't go past the 100 you have allocated?



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