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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jun 08, 2018 at 04:30:30PM +0100, Paul Durrant 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 and including 4GB, not marked as unusable in the
>  E820 table, will get a  mapping established."

My version of xen-command-line.markdown says:

"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 not marked as unusable in the E820 table will get a mapping
established."

I think the documentation or the code needs fixing, because the
current code does indeed only map up to 4GB.

> 
> 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.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown   | 4 +++-
>  xen/drivers/passthrough/iommu.c       | 2 +-
>  xen/drivers/passthrough/vtd/iommu.c   | 2 +-
>  xen/drivers/passthrough/vtd/x86/vtd.c | 8 ++++++++
>  xen/include/xen/iommu.h               | 2 +-
>  5 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 6beb28dada..97768f1633 100644
> --- 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 and including 4GB, not marked as unusable in the
> -E820 table, will get a mapping established.
> +E820 table, will get a mapping established. Note that if `dom0-strict`
> +mode is enabled then conventional RAM pages not assigned to dom0 will not
> +be mapped.
>  
>  ### irq\_ratelimit (x86)
>  > `= <integer>`
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 2c44fabf99..a483212356 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -52,7 +52,7 @@ custom_param("iommu", parse_iommu_param);
>  bool_t __initdata iommu_enable = 1;
>  bool_t __read_mostly iommu_enabled;
>  bool_t __read_mostly force_iommu;
> -bool_t __hwdom_initdata iommu_dom0_strict;
> +bool_t __read_mostly iommu_dom0_strict;

I'm not sure why you need to change the attributes of
iommu_dom0_strict, AFAICT it's still only used in hwdom_init (or init)
functions?

>  bool_t __read_mostly iommu_verbose;
>  bool_t __read_mostly iommu_workaround_bios_bug;
>  bool_t __read_mostly iommu_igfx = 1;
> diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> b/xen/drivers/passthrough/vtd/iommu.c
> index 08bce92d40..e3f043288b 100644
> --- 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 )

I think you will have to add an is_pv_domain(d) check here, or else PVH
Dom0 will also get those mappings, which is wrong because in the PVH
case we don't want to identity map PFNs into guest p2m.

PVH Dom0 wasn't calling vtd_set_hwdom_mapping because of the
!need_iommu check.

Thanks, Roger.

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