[PATCH] amd/iommu: remove hidden AMD inclusive mappings

[PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Roger Pau Monne]

And just rely on arch_iommu_hwdom_init to setup the correct inclusive
mappings as it's done for Intel.

AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
max_pdx, remove this since it's now a duplication of
arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
below 4GB, so this is a functional change for AMD.

Move the default setting of iommu_hwdom_{inclusive/reserved} to
arch_iommu_hwdom_init since the defaults are now the same for both
Intel and AMD.

Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
 xen/drivers/passthrough/vtd/iommu.c         |  7 ----
 xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
 3 files changed, 7 insertions(+), 47 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4a633ca940..821fe03df5 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -250,50 +250,11 @@ static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev);
 
 static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
 {
-    unsigned long i; 
     const struct amd_iommu *iommu;
 
-    /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
-    if ( iommu_hwdom_inclusive == -1 )
-        iommu_hwdom_inclusive = 0;
-    /* Reserved IOMMU mappings are disabled by default on AMD hardware. */
-    if ( iommu_hwdom_reserved == -1 )
-        iommu_hwdom_reserved = 0;
-
     if ( allocate_domain_resources(dom_iommu(d)) )
         BUG();
 
-    if ( !iommu_hwdom_passthrough && !need_iommu(d) )
-    {
-        int rc = 0;
-
-        /* Set up 1:1 page table for dom0 */
-        for ( i = 0; i < max_pdx; i++ )
-        {
-            unsigned long pfn = pdx_to_pfn(i);
-
-            /*
-             * XXX Should we really map all non-RAM (above 4G)? Minimally
-             * a pfn_valid() check would seem desirable here.
-             */
-            if ( mfn_valid(_mfn(pfn)) )
-            {
-                int ret = amd_iommu_map_page(d, pfn, pfn,
-                                             IOMMUF_readable|IOMMUF_writable);
-
-                if ( !rc )
-                    rc = ret;
-            }
-
-            if ( !(i & 0xfffff) )
-                process_pending_softirqs();
-        }
-
-        if ( rc )
-            AMD_IOMMU_DEBUG("d%d: IOMMU mapping failed: %d\n",
-                            d->domain_id, rc);
-    }
-
     for_each_amd_iommu ( iommu )
         if ( iomem_deny_access(d, PFN_DOWN(iommu->mmio_base_phys),
                                PFN_DOWN(iommu->mmio_base_phys +
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index bb422ec58c..cf8a80d7a1 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1304,13 +1304,6 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
 {
     struct acpi_drhd_unit *drhd;
 
-    /* Inclusive mappings are enabled by default on Intel hardware for PV. */
-    if ( iommu_hwdom_inclusive == -1 )
-        iommu_hwdom_inclusive = is_pv_domain(d);
-    /* Reserved IOMMU mappings are enabled by default on Intel hardware. */
-    if ( iommu_hwdom_reserved == -1 )
-        iommu_hwdom_reserved = 1;
-
     setup_hwdom_pci_devices(d, setup_hwdom_device);
     setup_hwdom_rmrr(d);
     /* Make sure workarounds are applied before enabling the IOMMU(s). */
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index b7c8b5be41..2de8822c59 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 
     BUG_ON(!is_hardware_domain(d));
 
-    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
+    /* Inclusive mappings are enabled by default for PV. */
+    if ( iommu_hwdom_inclusive == -1 )
+        iommu_hwdom_inclusive = is_pv_domain(d);
+    /* Reserved IOMMU mappings are enabled by default. */
+    if ( iommu_hwdom_reserved == -1 )
+        iommu_hwdom_reserved = 1;
+
     if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
     {
         printk(XENLOG_WARNING
-- 
2.19.0


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

Re: [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: 21 September 2018 16:21
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>; Kevin
> Tian <kevin.tian@intel.com>; Jan Beulich <jbeulich@suse.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH] amd/iommu: remove hidden AMD inclusive mappings
> 
> And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> mappings as it's done for Intel.
> 
> AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> max_pdx, remove this since it's now a duplication of
> arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> below 4GB, so this is a functional change for AMD.
> 
> Move the default setting of iommu_hwdom_{inclusive/reserved} to
> arch_iommu_hwdom_init since the defaults are now the same for both
> Intel and AMD.
> 

IMO the functional change and consequent alignment of the defaults is a good thing.

> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
>  xen/drivers/passthrough/vtd/iommu.c         |  7 ----
>  xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
>  3 files changed, 7 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 4a633ca940..821fe03df5 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -250,50 +250,11 @@ static int amd_iommu_add_device(u8 devfn, struct
> pci_dev *pdev);
> 
>  static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
>  {
> -    unsigned long i;
>      const struct amd_iommu *iommu;
> 
> -    /* Inclusive IOMMU mappings are disabled by default on AMD hardware.
> */
> -    if ( iommu_hwdom_inclusive == -1 )
> -        iommu_hwdom_inclusive = 0;
> -    /* Reserved IOMMU mappings are disabled by default on AMD hardware.
> */
> -    if ( iommu_hwdom_reserved == -1 )
> -        iommu_hwdom_reserved = 0;
> -
>      if ( allocate_domain_resources(dom_iommu(d)) )
>          BUG();
> 
> -    if ( !iommu_hwdom_passthrough && !need_iommu(d) )
> -    {
> -        int rc = 0;
> -
> -        /* Set up 1:1 page table for dom0 */
> -        for ( i = 0; i < max_pdx; i++ )
> -        {
> -            unsigned long pfn = pdx_to_pfn(i);
> -
> -            /*
> -             * XXX Should we really map all non-RAM (above 4G)? Minimally
> -             * a pfn_valid() check would seem desirable here.
> -             */
> -            if ( mfn_valid(_mfn(pfn)) )
> -            {
> -                int ret = amd_iommu_map_page(d, pfn, pfn,
> -
> IOMMUF_readable|IOMMUF_writable);
> -
> -                if ( !rc )
> -                    rc = ret;
> -            }
> -
> -            if ( !(i & 0xfffff) )
> -                process_pending_softirqs();
> -        }
> -
> -        if ( rc )
> -            AMD_IOMMU_DEBUG("d%d: IOMMU mapping failed: %d\n",
> -                            d->domain_id, rc);
> -    }
> -
>      for_each_amd_iommu ( iommu )
>          if ( iomem_deny_access(d, PFN_DOWN(iommu->mmio_base_phys),
>                                 PFN_DOWN(iommu->mmio_base_phys +
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index bb422ec58c..cf8a80d7a1 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1304,13 +1304,6 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
> 
> -    /* Inclusive mappings are enabled by default on Intel hardware for
> PV. */
> -    if ( iommu_hwdom_inclusive == -1 )
> -        iommu_hwdom_inclusive = is_pv_domain(d);
> -    /* Reserved IOMMU mappings are enabled by default on Intel hardware.
> */
> -    if ( iommu_hwdom_reserved == -1 )
> -        iommu_hwdom_reserved = 1;
> -
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
>      setup_hwdom_rmrr(d);
>      /* Make sure workarounds are applied before enabling the IOMMU(s). */
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index b7c8b5be41..2de8822c59 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain
> *d)
> 
>      BUG_ON(!is_hardware_domain(d));
> 
> -    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> +    /* Inclusive mappings are enabled by default for PV. */
> +    if ( iommu_hwdom_inclusive == -1 )
> +        iommu_hwdom_inclusive = is_pv_domain(d);
> +    /* Reserved IOMMU mappings are enabled by default. */
> +    if ( iommu_hwdom_reserved == -1 )
> +        iommu_hwdom_reserved = 1;
> +
>      if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
>      {
>          printk(XENLOG_WARNING
> --
> 2.19.0

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

Re: [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Jan Beulich]

>>> On 21.09.18 at 17:20, <roger.pau@citrix.com> wrote:
> And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> mappings as it's done for Intel.
> 
> AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> max_pdx, remove this since it's now a duplication of
> arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> below 4GB, so this is a functional change for AMD.
> 
> Move the default setting of iommu_hwdom_{inclusive/reserved} to
> arch_iommu_hwdom_init since the defaults are now the same for both
> Intel and AMD.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Tian, Kevin]

> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Friday, September 21, 2018 11:21 PM
> 
> And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> mappings as it's done for Intel.
> 
> AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up
> to
> max_pdx, remove this since it's now a duplication of
> arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> below 4GB, so this is a functional change for AMD.
> 
> Move the default setting of iommu_hwdom_{inclusive/reserved} to
> arch_iommu_hwdom_init since the defaults are now the same for both
> Intel and AMD.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

AMD Ping [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Roger Pau Monné]

Can I please get an Ack or otherwise from the AMD maintainers?

Thanks, Roger.

On Fri, Sep 21, 2018 at 05:20:47PM +0200, Roger Pau Monne wrote:
> And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> mappings as it's done for Intel.
> 
> AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> max_pdx, remove this since it's now a duplication of
> arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> below 4GB, so this is a functional change for AMD.
> 
> Move the default setting of iommu_hwdom_{inclusive/reserved} to
> arch_iommu_hwdom_init since the defaults are now the same for both
> Intel and AMD.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
>  xen/drivers/passthrough/vtd/iommu.c         |  7 ----
>  xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
>  3 files changed, 7 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 4a633ca940..821fe03df5 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -250,50 +250,11 @@ static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev);
>  
>  static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
>  {
> -    unsigned long i; 
>      const struct amd_iommu *iommu;
>  
> -    /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
> -    if ( iommu_hwdom_inclusive == -1 )
> -        iommu_hwdom_inclusive = 0;
> -    /* Reserved IOMMU mappings are disabled by default on AMD hardware. */
> -    if ( iommu_hwdom_reserved == -1 )
> -        iommu_hwdom_reserved = 0;
> -
>      if ( allocate_domain_resources(dom_iommu(d)) )
>          BUG();
>  
> -    if ( !iommu_hwdom_passthrough && !need_iommu(d) )
> -    {
> -        int rc = 0;
> -
> -        /* Set up 1:1 page table for dom0 */
> -        for ( i = 0; i < max_pdx; i++ )
> -        {
> -            unsigned long pfn = pdx_to_pfn(i);
> -
> -            /*
> -             * XXX Should we really map all non-RAM (above 4G)? Minimally
> -             * a pfn_valid() check would seem desirable here.
> -             */
> -            if ( mfn_valid(_mfn(pfn)) )
> -            {
> -                int ret = amd_iommu_map_page(d, pfn, pfn,
> -                                             IOMMUF_readable|IOMMUF_writable);
> -
> -                if ( !rc )
> -                    rc = ret;
> -            }
> -
> -            if ( !(i & 0xfffff) )
> -                process_pending_softirqs();
> -        }
> -
> -        if ( rc )
> -            AMD_IOMMU_DEBUG("d%d: IOMMU mapping failed: %d\n",
> -                            d->domain_id, rc);
> -    }
> -
>      for_each_amd_iommu ( iommu )
>          if ( iomem_deny_access(d, PFN_DOWN(iommu->mmio_base_phys),
>                                 PFN_DOWN(iommu->mmio_base_phys +
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index bb422ec58c..cf8a80d7a1 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1304,13 +1304,6 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>  {
>      struct acpi_drhd_unit *drhd;
>  
> -    /* Inclusive mappings are enabled by default on Intel hardware for PV. */
> -    if ( iommu_hwdom_inclusive == -1 )
> -        iommu_hwdom_inclusive = is_pv_domain(d);
> -    /* Reserved IOMMU mappings are enabled by default on Intel hardware. */
> -    if ( iommu_hwdom_reserved == -1 )
> -        iommu_hwdom_reserved = 1;
> -
>      setup_hwdom_pci_devices(d, setup_hwdom_device);
>      setup_hwdom_rmrr(d);
>      /* Make sure workarounds are applied before enabling the IOMMU(s). */
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index b7c8b5be41..2de8822c59 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>  
>      BUG_ON(!is_hardware_domain(d));
>  
> -    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> +    /* Inclusive mappings are enabled by default for PV. */
> +    if ( iommu_hwdom_inclusive == -1 )
> +        iommu_hwdom_inclusive = is_pv_domain(d);
> +    /* Reserved IOMMU mappings are enabled by default. */
> +    if ( iommu_hwdom_reserved == -1 )
> +        iommu_hwdom_reserved = 1;
> +
>      if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
>      {
>          printk(XENLOG_WARNING
> -- 
> 2.19.0
> 

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

Re: [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Suravee Suthikulpanit]

Roger,

I'm still catching up on the map-inclusive and map-reserved stuff.
I have a couple questions below.

On 9/21/18 10:20 PM, Roger Pau Monne wrote:
> And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> mappings as it's done for Intel.
> 
> AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> max_pdx, remove this since it's now a duplication of
> arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> below 4GB, so this is a functional change for AMD.

Is there any reasons why limit to only below 4GB?

> Move the default setting of iommu_hwdom_{inclusive/reserved} to
> arch_iommu_hwdom_init since the defaults are now the same for both
> Intel and AMD.
> 
> Reported-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> ---
>   xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
>   xen/drivers/passthrough/vtd/iommu.c         |  7 ----
>   xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
>   3 files changed, 7 insertions(+), 47 deletions(-)
> 
> ...
 >
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index b7c8b5be41..2de8822c59 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>   
>       BUG_ON(!is_hardware_domain(d));
>   
> -    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);

Not sure if this was a typo. The logic looks strange. Anyhow, it is removed.

However, I notice in the xen/drivers/passthrough/iommu.c that
the parsing logic for the map-reserved option is setting the iommu_hwdom_inclusive
instead of the iommu_hwdom_reserved. Is that intentional?

Also, what's the difference b/w the option map-inclusive parameter in drivers/passthrough/iommu.c:
parse_dom0_iommu_param() and the option iommu_inclusive_mapping in drivers/passthrough/vtd/x86/vtd.c?

> +    /* Inclusive mappings are enabled by default for PV. */
> +    if ( iommu_hwdom_inclusive == -1 )
> +        iommu_hwdom_inclusive = is_pv_domain(d);
> +    /* Reserved IOMMU mappings are enabled by default. */
> +    if ( iommu_hwdom_reserved == -1 )
> +        iommu_hwdom_reserved = 1;
> +
>       if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
>       {
>           printk(XENLOG_WARNING
> 

Thanks,
Suravee

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

Re: [PATCH] amd/iommu: remove hidden AMD inclusive mappings

[Roger Pau Monné]

On Tue, Oct 02, 2018 at 02:37:31PM +0700, Suravee Suthikulpanit wrote:
> Roger,
> 
> I'm still catching up on the map-inclusive and map-reserved stuff.
> I have a couple questions below.
> 
> On 9/21/18 10:20 PM, Roger Pau Monne wrote:
> > And just rely on arch_iommu_hwdom_init to setup the correct inclusive
> > mappings as it's done for Intel.
> > 
> > AMD has code in amd_iommu_hwdom_init to setup inclusive mappings up to
> > max_pdx, remove this since it's now a duplication of
> > arch_iommu_hwdom_init. Note that AMD mapped every page with a valid
> > mfn up to max_pdx, arch_iommu_hwdom_init will only do so for memory
> > below 4GB, so this is a functional change for AMD.
> 
> Is there any reasons why limit to only below 4GB?

This is what Intel IOMMU has always done, so this patch unifies the
inclusive mappings code between Intel and AMD. Note that reserved
regions above 4GB will still be mapped.

> > Move the default setting of iommu_hwdom_{inclusive/reserved} to
> > arch_iommu_hwdom_init since the defaults are now the same for both
> > Intel and AMD.
> > 
> > Reported-by: Paul Durrant <paul.durrant@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > Cc: Brian Woods <brian.woods@amd.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >   xen/drivers/passthrough/amd/pci_amd_iommu.c | 39 ---------------------
> >   xen/drivers/passthrough/vtd/iommu.c         |  7 ----
> >   xen/drivers/passthrough/x86/iommu.c         |  8 ++++-
> >   3 files changed, 7 insertions(+), 47 deletions(-)
> > 
> > ...
> >
> > diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> > index b7c8b5be41..2de8822c59 100644
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -210,7 +210,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> >       BUG_ON(!is_hardware_domain(d));
> > -    ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> 
> Not sure if this was a typo. The logic looks strange. Anyhow, it is removed.

Oh right, this should check inclusive and reserved.

> However, I notice in the xen/drivers/passthrough/iommu.c that
> the parsing logic for the map-reserved option is setting the iommu_hwdom_inclusive
> instead of the iommu_hwdom_reserved. Is that intentional?

It's the same typo as above, will send a patch to fix it.

> Also, what's the difference b/w the option map-inclusive parameter in drivers/passthrough/iommu.c:
> parse_dom0_iommu_param() and the option iommu_inclusive_mapping in drivers/passthrough/vtd/x86/vtd.c?

The old iommu_inclusive_mapping option is keep for compatibility
reasons, but the documentation has been updated to mark that option as
deprecated.

Thanks, Roger.

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