Re: [Xen-devel] [PATCH v4 2/4] iommu: make iommu_inclusive_mapping a suboption of dom0-iommu

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.18 at 12:07, <roger.pau@xxxxxxxxxx> wrote:
> Introduce a new dom0-iommu=inclusive generic option that supersedes
> iommu_inclusive_mapping. The previous behaviour is preserved and the
> option should only be enabled by default on Intel hardware.

Why "should" instead of "is"?

> @@ -1221,6 +1221,18 @@ PV Dom0:
>  Note that all the above options are mutually exclusive. Specifying more than
>  one on the `dom0-iommu` command line will result in undefined behavior.
>  
> +The following options control whether non-RAM regions are added to the Dom0
> +iommu tables. Note that they can be prefixed with `no-` to effect the inverse
> +meaning:

I'm not particularly happy about the mentioning of "no-" here: Why is
this better than the also permitted "=0" etc suffixes? Keep it generic,
just like other options do.

> +* `inclusive`: sets up DMA remapping for all the non-RAM memory below 4GB
> +  except for unusable ranges. Use this to work around firmware issues 
> providing
> +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
> +  accesses for Dom0, with this option all pages up to 4GB, not marked as
> +  unusable in the E820 table, will get a mapping established. Note that this
> +  option is only applicable to a PV Dom0 and is enabled by default on Intel
> +  hardware.

No word at all about the interaction with none/strict/relaxed? I think,
as mentioned for patch 1, "none" renders this option meaningless as
well. But for "relaxed" it's pretty unclear, because from E820 alone
you can't judge whether e.g. a reserved region is RAM or MMIO. (As
an implication, the mentioning of RAM in patch 1's doc for "relaxed"
then looks symmetrically wrong, just like I've already asked to replace
"memory" by "RAM" for "strict".)

> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,7 @@ int arch_iommu_populate_page_table(struct domain *d)
>      /* The IOMMU shares the p2m with the CPU */
>      return -ENOSYS;
>  }
> +
> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +}

The option being in common code, I think you also need to set it for
ARM, so it won't remain at its default of -1.

> @@ -144,16 +145,23 @@ static int __init parse_dom0_iommu_param(const char *s)
>      int rc = 0;
>  
>      do {
> +        bool val = !!strncmp(s, "no-", 3);

Oh, you do a literal comparison against "no-". Please don't, that's what
we have parse_boolean() for.

> @@ -202,6 +210,13 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      if ( !iommu_enabled )
>          return;
>  
> +    if ( iommu_dom0_inclusive == true && !is_pv_domain(d) )

Why the "== true"? It shouldn't have its initializer value of -1 anymore
at this point.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel