Re: [Xen-devel] [PATCH v2 2/2] VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.18 at 15:47, <paul.durrant@xxxxxxxxxx> wrote:
> The documentation for the iommu_inclusive_mapping Xen command line option
> states:
> 
> "Use this to work around firmware issues providing incorrect RMRR entries"
> 
> Unfortunately this workaround does not function correctly if the dom0-strict
> iommu option is also specified.
> 
> The documentation goes on to say:
> 
> "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."
> 
> This patch modifies the VT-d hardware domain initialization code such that
> the workaround will continue to function in dom0-strict mode, by mapping
> all pages not marked as unusable *unless* they are RAM pages not assigned
> to dom0.

Hmm, so far I was under the impression that the two options aren't meant
to work together anyway.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1213,7 +1213,9 @@ wait descriptor timed out', try increasing this value.
>  Use this to work around firmware issues providing incorrect RMRR 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.
> +get a mapping established. Note that this option is only applicable to a
> +PV dom0 and if `dom0-strict` mode is enabled then conventional RAM pages
> +not assigned to dom0 will not be mapped.

I think for readability you want to either split the sentence into two (dropping
the "and") or at least insert a comma ahead of the "and".

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1304,7 +1304,7 @@ static void __hwdom_init intel_iommu_hwdom_init(struct 
> domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
>  
> -    if ( !iommu_passthrough && !need_iommu(d) )
> +    if ( !iommu_passthrough && is_pv_domain(d) )

I don't understand this change: Where is the !need_iommu() condition
getting enforced now? Oh, wait ...

> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -143,6 +143,14 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
>          if ( xen_in_range(pfn) )
>              continue;
>  
> +        /*
> +         * If dom0-strict mode is enabled then exclude conventional RAM
> +         * and let the common code map dom0's pages.
> +         */
> +        if ( iommu_dom0_strict &&

This is equivalent to need_iommu(d), and if you wrote it that way the
patch would be easier to review and ...

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -29,7 +29,7 @@
>  #include <asm/iommu.h>
>  
>  extern bool_t iommu_enable, iommu_enabled;
> -extern bool_t force_iommu, iommu_verbose;
> +extern bool_t force_iommu, iommu_dom0_strict, iommu_verbose;

... this wouldn't be needed.

Jan



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