Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
@ 2019-08-02  9:22 Roger Pau Monne
  2019-08-02 13:09 ` Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Roger Pau Monne @ 2019-08-02  9:22 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, Roger Pau Monne

Switch rmrr_identity_mapping to use iommu_{un}map in order to
establish RMRR mappings for PV domains, like it's done in
arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
not getting RMRR mappings because {set/clear}_identity_p2m_entry was
not properly updating the iommu page tables.

As rmrr_identity_mapping was the last user of
{set/clear}_identity_p2m_entry against PV domains modify the function
so it's only usable against translated domains, as the other p2m
related functions.

Reported-by: Roman Shaposhnik <roman@zededa.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Roman Shaposhnik <roman@zededa.com>
---
 xen/arch/x86/mm/p2m.c               | 11 ++++-------
 xen/drivers/passthrough/vtd/iommu.c | 23 ++++++++++++++++++-----
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index fef97c82f6..d36a58b1a6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1341,10 +1341,8 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
 
     if ( !paging_mode_translate(p2m->domain) )
     {
-        if ( !need_iommu_pt_sync(d) )
-            return 0;
-        return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
-                                IOMMUF_readable | IOMMUF_writable);
+        ASSERT_UNREACHABLE();
+        return -ENXIO;
     }
 
     gfn_lock(p2m, gfn, 0);
@@ -1432,9 +1430,8 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
 
     if ( !paging_mode_translate(d) )
     {
-        if ( !need_iommu_pt_sync(d) )
-            return 0;
-        return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
+        ASSERT_UNREACHABLE();
+        return -ENXIO;
     }
 
     gfn_lock(p2m, gfn, 0);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5d72270c5b..62df5ca5aa 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1969,6 +1969,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
     unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> PAGE_SHIFT_4K;
     struct mapped_rmrr *mrmrr;
     struct domain_iommu *hd = dom_iommu(d);
+    unsigned int flush_flags = 0;
 
     ASSERT(pcidevs_locked());
     ASSERT(rmrr->base_address < rmrr->end_address);
@@ -1982,7 +1983,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
         if ( mrmrr->base == rmrr->base_address &&
              mrmrr->end == rmrr->end_address )
         {
-            int ret = 0;
+            int ret = 0, err;
 
             if ( map )
             {
@@ -1995,13 +1996,20 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
 
             while ( base_pfn < end_pfn )
             {
-                if ( clear_identity_p2m_entry(d, base_pfn) )
-                    ret = -ENXIO;
+                if ( paging_mode_translate(d) )
+                    ret = clear_identity_p2m_entry(d, base_pfn);
+                else
+                    ret = iommu_unmap(d, _dfn(base_pfn), PAGE_ORDER_4K,
+                                      &flush_flags);
                 base_pfn++;
             }
 
             list_del(&mrmrr->list);
             xfree(mrmrr);
+            /* Keep the previous error code if there's one. */
+            err = iommu_iotlb_flush_all(d, flush_flags);
+            if ( !ret )
+                ret = err;
             return ret;
         }
     }
@@ -2011,8 +2019,13 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
 
     while ( base_pfn < end_pfn )
     {
-        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
+        int err;
 
+        if ( paging_mode_translate(d) )
+            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
+        else
+            err = iommu_map(d, _dfn(base_pfn), _mfn(base_pfn), PAGE_ORDER_4K,
+                            IOMMUF_readable | IOMMUF_writable, &flush_flags);
         if ( err )
             return err;
         base_pfn++;
@@ -2026,7 +2039,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
     mrmrr->count = 1;
     list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
 
-    return 0;
+    return iommu_iotlb_flush_all(d, flush_flags);
 }
 
 static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev)
-- 
2.20.1 (Apple Git-117)


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-02  9:22 [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains Roger Pau Monne
@ 2019-08-02 13:09 ` Jan Beulich
  2019-08-12 10:17 ` Paul Durrant
  2019-08-12 13:17 ` George Dunlap
  2 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2019-08-02 13:09 UTC (permalink / raw)
  To: Roger Pau Monne, George Dunlap
  Cc: Kevin Tian, Wei Liu, Andrew Cooper, Roman Shaposhnik,
	Paul Durrant, xen-devel

On 02.08.2019 11:22, Roger Pau Monne wrote:
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.

In which was this was not happening properly is still not really
understood, yet I think this (or at least a plausible theory) is
a prereq for any kind of fix. The code paths for PV and HVM are
probably different enough such that we don't need to be afraid
of there being a similar problem on the HVM side, but what if
there's a more general problem that we would better take care of,
rather perhaps than getting into a similarly puzzling situation
again later on.

> As rmrr_identity_mapping was the last user of
> {set/clear}_identity_p2m_entry against PV domains modify the function
> so it's only usable against translated domains, as the other p2m
> related functions.

Looking at the resulting code I'm actually not convinced that
this is a good move. I would, however, specifically like to
hear George's opinion here.

> @@ -1995,13 +1996,20 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>   
>               while ( base_pfn < end_pfn )
>               {
> -                if ( clear_identity_p2m_entry(d, base_pfn) )
> -                    ret = -ENXIO;
> +                if ( paging_mode_translate(d) )
> +                    ret = clear_identity_p2m_entry(d, base_pfn);
> +                else
> +                    ret = iommu_unmap(d, _dfn(base_pfn), PAGE_ORDER_4K,
> +                                      &flush_flags);
>                   base_pfn++;
>               }
>   
>               list_del(&mrmrr->list);
>               xfree(mrmrr);
> +            /* Keep the previous error code if there's one. */
> +            err = iommu_iotlb_flush_all(d, flush_flags);
> +            if ( !ret )
> +                ret = err;
>               return ret;
>           }
>       }
> @@ -2011,8 +2019,13 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>   
>       while ( base_pfn < end_pfn )
>       {
> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        int err;
>   
> +        if ( paging_mode_translate(d) )
> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        else
> +            err = iommu_map(d, _dfn(base_pfn), _mfn(base_pfn), PAGE_ORDER_4K,
> +                            IOMMUF_readable | IOMMUF_writable, &flush_flags);
>           if ( err )
>               return err;
>           base_pfn++;
> @@ -2026,7 +2039,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>       mrmrr->count = 1;
>       list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
>   
> -    return 0;
> +    return iommu_iotlb_flush_all(d, flush_flags);

This is VT-d code - is there a particular reason why you go through
the generic code wrappers everywhere here?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-02  9:22 [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains Roger Pau Monne
  2019-08-02 13:09 ` Jan Beulich
@ 2019-08-12 10:17 ` Paul Durrant
  2019-08-12 13:17 ` George Dunlap
  2 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2019-08-12 10:17 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel
  Cc: Kevin Tian, Wei Liu, Andrew Cooper, Roman Shaposhnik,
	George Dunlap, Jan Beulich, Roger Pau Monne

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 02 August 2019 10:22
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Roman Shaposhnik <roman@zededa.com>; George Dunlap
> <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Kevin Tian <kevin.tian@intel.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
> 
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.
> 

I'm not sure this is the right approach. TBH it's not clear to me exactly what role the P2M code should play for a PV domain (I'm guessing it's probably there so that shadowing can be turned on) but it would make more sense to me if fewer paths went round the side of it and the internals handled all of the PV/HVM differences.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-02  9:22 [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains Roger Pau Monne
  2019-08-02 13:09 ` Jan Beulich
  2019-08-12 10:17 ` Paul Durrant
@ 2019-08-12 13:17 ` George Dunlap
  2019-08-12 13:56   ` Roger Pau Monné
  2 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2019-08-12 13:17 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich

On 8/2/19 10:22 AM, Roger Pau Monne wrote:
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.

Sorry, I think this description is somewhat backwards: you're saying
what you're doing first, and then saying what the problematic behavior
was, but not actually saying what was causing the problematic behavior.

Why was {set,clear}_identity_p2m not updating the page tables?

I agree with Jan, that figuring that out is a prerequisite for any kind
of fix: if `need_iommu_pt_sync()` is false at that point for the
hardware domain, then there's a bigger problem than RMRRs not being
adjusted properly.

 -George

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-12 13:17 ` George Dunlap
@ 2019-08-12 13:56   ` Roger Pau Monné
  2019-08-12 14:24     ` George Dunlap
  0 siblings, 1 reply; 9+ messages in thread
From: Roger Pau Monné @ 2019-08-12 13:56 UTC (permalink / raw)
  To: George Dunlap
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, xen-devel

On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
> > Switch rmrr_identity_mapping to use iommu_{un}map in order to
> > establish RMRR mappings for PV domains, like it's done in
> > arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> > not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> > not properly updating the iommu page tables.
> 
> Sorry, I think this description is somewhat backwards: you're saying
> what you're doing first, and then saying what the problematic behavior
> was, but not actually saying what was causing the problematic behavior.
> 
> Why was {set,clear}_identity_p2m not updating the page tables?
> 
> I agree with Jan, that figuring that out is a prerequisite for any kind
> of fix: if `need_iommu_pt_sync()` is false at that point for the
> hardware domain, then there's a bigger problem than RMRRs not being
> adjusted properly.

need_iommu_pt_sync is indeed false for a PV hardware domain not
running in strict mode, see:

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html

This is fine for a non-strict PV hardware domain, since it has all the
host memory (minus memory used by Xen) mapped in the iommu page tables
except for RMRR regions not marked as reserved in the e820 memory map,
which are added using set_identity_p2m_entry.

The issue here is that this patch alone doesn't fix the issue for the
reporter, and there seems to be an additional flush required in order
to get the issue solved on his end:

https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html

My theory is that the system Roman is using is booting with DMA
remapping enabled in the iommu, and somehow the call to
iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
properly, while calling iommu_iotlb_flush_all does seem to do the
right thing. I'm still waiting for Roman to come back with the result
of my last debug patches in order to figure out whether my hypothesis
above is true.

This however won't still explain the weird behaviour of
iommu_flush_all, and further debugging will still be required.

Roger.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-12 13:56   ` Roger Pau Monné
@ 2019-08-12 14:24     ` George Dunlap
  2019-08-12 14:55       ` Roger Pau Monné
  0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2019-08-12 14:24 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, xen-devel

On 8/12/19 2:56 PM, Roger Pau Monné wrote:
> On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
>> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
>>> Switch rmrr_identity_mapping to use iommu_{un}map in order to
>>> establish RMRR mappings for PV domains, like it's done in
>>> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
>>> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
>>> not properly updating the iommu page tables.
>>
>> Sorry, I think this description is somewhat backwards: you're saying
>> what you're doing first, and then saying what the problematic behavior
>> was, but not actually saying what was causing the problematic behavior.
>>
>> Why was {set,clear}_identity_p2m not updating the page tables?
>>
>> I agree with Jan, that figuring that out is a prerequisite for any kind
>> of fix: if `need_iommu_pt_sync()` is false at that point for the
>> hardware domain, then there's a bigger problem than RMRRs not being
>> adjusted properly.
> 
> need_iommu_pt_sync is indeed false for a PV hardware domain not
> running in strict mode, see:
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
> 
> This is fine for a non-strict PV hardware domain, since it has all the
> host memory (minus memory used by Xen) mapped in the iommu page tables
> except for RMRR regions not marked as reserved in the e820 memory map,
> which are added using set_identity_p2m_entry.
> 
> The issue here is that this patch alone doesn't fix the issue for the
> reporter, and there seems to be an additional flush required in order
> to get the issue solved on his end:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html
> 
> My theory is that the system Roman is using is booting with DMA
> remapping enabled in the iommu, and somehow the call to
> iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
> properly, while calling iommu_iotlb_flush_all does seem to do the
> right thing. I'm still waiting for Roman to come back with the result
> of my last debug patches in order to figure out whether my hypothesis
> above is true.
> 
> This however won't still explain the weird behaviour of
> iommu_flush_all, and further debugging will still be required.

OK; so this patch has four effects, it looks like:

1. iommu_legacy_[un]map -> iommu_[un]map
2. Move iommu ops out of {set,clear}_identity_p2m for PV guests;
open-code them in rmrr_identity_mapping
3. For non-translated guests, do the operation unconditionally
4. Add a flush

Regarding #3, the previous patch changed it from "if
iommu_needs_pt_sync" to "if has_iommu_pt"; this one changes it to
"always".  Is that what you intended?

I don't really see the point of #2: from the device's perspective, the
"physmap" is the IOMMU for PV guests, and IOMMU+p2m for HVM guests; it
makes sense to have a single place to call for either type of guest,
rather than open-coding every location.

Can't comment on the other aspects.

 -George

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-12 14:24     ` George Dunlap
@ 2019-08-12 14:55       ` Roger Pau Monné
  2019-08-12 15:15         ` George Dunlap
  0 siblings, 1 reply; 9+ messages in thread
From: Roger Pau Monné @ 2019-08-12 14:55 UTC (permalink / raw)
  To: George Dunlap
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, xen-devel

On Mon, Aug 12, 2019 at 03:24:36PM +0100, George Dunlap wrote:
> On 8/12/19 2:56 PM, Roger Pau Monné wrote:
> > On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
> >> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
> >>> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> >>> establish RMRR mappings for PV domains, like it's done in
> >>> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> >>> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> >>> not properly updating the iommu page tables.
> >>
> >> Sorry, I think this description is somewhat backwards: you're saying
> >> what you're doing first, and then saying what the problematic behavior
> >> was, but not actually saying what was causing the problematic behavior.
> >>
> >> Why was {set,clear}_identity_p2m not updating the page tables?
> >>
> >> I agree with Jan, that figuring that out is a prerequisite for any kind
> >> of fix: if `need_iommu_pt_sync()` is false at that point for the
> >> hardware domain, then there's a bigger problem than RMRRs not being
> >> adjusted properly.
> > 
> > need_iommu_pt_sync is indeed false for a PV hardware domain not
> > running in strict mode, see:
> > 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
> > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
> > 
> > This is fine for a non-strict PV hardware domain, since it has all the
> > host memory (minus memory used by Xen) mapped in the iommu page tables
> > except for RMRR regions not marked as reserved in the e820 memory map,
> > which are added using set_identity_p2m_entry.
> > 
> > The issue here is that this patch alone doesn't fix the issue for the
> > reporter, and there seems to be an additional flush required in order
> > to get the issue solved on his end:
> > 
> > https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html
> > 
> > My theory is that the system Roman is using is booting with DMA
> > remapping enabled in the iommu, and somehow the call to
> > iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
> > properly, while calling iommu_iotlb_flush_all does seem to do the
> > right thing. I'm still waiting for Roman to come back with the result
> > of my last debug patches in order to figure out whether my hypothesis
> > above is true.
> > 
> > This however won't still explain the weird behaviour of
> > iommu_flush_all, and further debugging will still be required.
> 
> OK; so this patch has four effects, it looks like:
> 
> 1. iommu_legacy_[un]map -> iommu_[un]map
> 2. Move iommu ops out of {set,clear}_identity_p2m for PV guests;
> open-code them in rmrr_identity_mapping
> 3. For non-translated guests, do the operation unconditionally
> 4. Add a flush

There's already a flush in iommu_legacy_[un]map, so the flush is also
done for both patches, just in different places. The legacy interface
does the flush on every call, while the new interface allows to
postpone the flush until all iommu page-table operations are done.

> 
> Regarding #3, the previous patch changed it from "if
> iommu_needs_pt_sync" to "if has_iommu_pt"; this one changes it to
> "always".  Is that what you intended?

Well, #3 is not actually correct. The code in intel_iommu_hwdom_init
and hence setup_hwdom_rmrr will only be called if the domain has an
iommu, so has_iommu_pt will always be true when adding RMRR regions.
Note rmrr_identity_mapping is the only caller of
{set,clear}_identity_p2m against PV guests.

> I don't really see the point of #2: from the device's perspective, the
> "physmap" is the IOMMU for PV guests, and IOMMU+p2m for HVM guests; it
> makes sense to have a single place to call for either type of guest,
> rather than open-coding every location.

OK, that's fine, but note that {set/clear}_identity_p2m_entry is
AFAICT the only p2m function allowed against PV guests, the rest will
return some kind of error, which IMO makes it the outlier, hence my
proposal to make it available to translated guests only, like the rest
of the p2m functions in that file. I just find it confusing that some
p2m functions can be used against PV guests, but not others.

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-12 14:55       ` Roger Pau Monné
@ 2019-08-12 15:15         ` George Dunlap
  2019-08-12 15:30           ` Roger Pau Monné
  0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2019-08-12 15:15 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, xen-devel

On 8/12/19 3:55 PM, Roger Pau Monné wrote:
> On Mon, Aug 12, 2019 at 03:24:36PM +0100, George Dunlap wrote:
>> On 8/12/19 2:56 PM, Roger Pau Monné wrote:
>>> On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
>>>> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
>>>>> Switch rmrr_identity_mapping to use iommu_{un}map in order to
>>>>> establish RMRR mappings for PV domains, like it's done in
>>>>> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
>>>>> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
>>>>> not properly updating the iommu page tables.
>>>>
>>>> Sorry, I think this description is somewhat backwards: you're saying
>>>> what you're doing first, and then saying what the problematic behavior
>>>> was, but not actually saying what was causing the problematic behavior.
>>>>
>>>> Why was {set,clear}_identity_p2m not updating the page tables?
>>>>
>>>> I agree with Jan, that figuring that out is a prerequisite for any kind
>>>> of fix: if `need_iommu_pt_sync()` is false at that point for the
>>>> hardware domain, then there's a bigger problem than RMRRs not being
>>>> adjusted properly.
>>>
>>> need_iommu_pt_sync is indeed false for a PV hardware domain not
>>> running in strict mode, see:
>>>
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
>>> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
>>>
>>> This is fine for a non-strict PV hardware domain, since it has all the
>>> host memory (minus memory used by Xen) mapped in the iommu page tables
>>> except for RMRR regions not marked as reserved in the e820 memory map,
>>> which are added using set_identity_p2m_entry.
>>>
>>> The issue here is that this patch alone doesn't fix the issue for the
>>> reporter, and there seems to be an additional flush required in order
>>> to get the issue solved on his end:
>>>
>>> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html
>>>
>>> My theory is that the system Roman is using is booting with DMA
>>> remapping enabled in the iommu, and somehow the call to
>>> iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
>>> properly, while calling iommu_iotlb_flush_all does seem to do the
>>> right thing. I'm still waiting for Roman to come back with the result
>>> of my last debug patches in order to figure out whether my hypothesis
>>> above is true.
>>>
>>> This however won't still explain the weird behaviour of
>>> iommu_flush_all, and further debugging will still be required.
>>
>> OK; so this patch has four effects, it looks like:
>>
>> 1. iommu_legacy_[un]map -> iommu_[un]map
>> 2. Move iommu ops out of {set,clear}_identity_p2m for PV guests;
>> open-code them in rmrr_identity_mapping
>> 3. For non-translated guests, do the operation unconditionally
>> 4. Add a flush
> 
> There's already a flush in iommu_legacy_[un]map, so the flush is also
> done for both patches, just in different places. The legacy interface
> does the flush on every call, while the new interface allows to
> postpone the flush until all iommu page-table operations are done.
> 
>>
>> Regarding #3, the previous patch changed it from "if
>> iommu_needs_pt_sync" to "if has_iommu_pt"; this one changes it to
>> "always".  Is that what you intended?
> 
> Well, #3 is not actually correct. The code in intel_iommu_hwdom_init
> and hence setup_hwdom_rmrr will only be called if the domain has an
> iommu, so has_iommu_pt will always be true when adding RMRR regions.
> Note rmrr_identity_mapping is the only caller of
> {set,clear}_identity_p2m against PV guests.

But if iommu is set the same in both cases, and if the flush happens in
both cases, then why did this patch work and the previous patch didn't?

>> I don't really see the point of #2: from the device's perspective, the
>> "physmap" is the IOMMU for PV guests, and IOMMU+p2m for HVM guests; it
>> makes sense to have a single place to call for either type of guest,
>> rather than open-coding every location.
> 
> OK, that's fine, but note that {set/clear}_identity_p2m_entry is
> AFAICT the only p2m function allowed against PV guests, the rest will
> return some kind of error, which IMO makes it the outlier, hence my
> proposal to make it available to translated guests only, like the rest
> of the p2m functions in that file. I just find it confusing that some
> p2m functions can be used against PV guests, but not others.

Right, but that's because set_identity_p2m_entry() isn't really about
the p2m.  It's about making sure that both devices and SMI handlers (or
whatever) can access certain bits of memory.

Lots of functions in p2m.c "tolerate" calls for PV guests; and
guest_physmap_* calls used to actually to IOMMU operations on PV guests
before XSA 288.

We could change that to set_identity_physmap_entry() or something if you
prefer.

 -George




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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
  2019-08-12 15:15         ` George Dunlap
@ 2019-08-12 15:30           ` Roger Pau Monné
  0 siblings, 0 replies; 9+ messages in thread
From: Roger Pau Monné @ 2019-08-12 15:30 UTC (permalink / raw)
  To: George Dunlap
  Cc: Kevin Tian, Wei Liu, George Dunlap, Andrew Cooper,
	Roman Shaposhnik, Paul Durrant, Jan Beulich, xen-devel

On Mon, Aug 12, 2019 at 04:15:38PM +0100, George Dunlap wrote:
> On 8/12/19 3:55 PM, Roger Pau Monné wrote:
> > On Mon, Aug 12, 2019 at 03:24:36PM +0100, George Dunlap wrote:
> >> On 8/12/19 2:56 PM, Roger Pau Monné wrote:
> >>> On Mon, Aug 12, 2019 at 02:17:53PM +0100, George Dunlap wrote:
> >>>> On 8/2/19 10:22 AM, Roger Pau Monne wrote:
> >>>>> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> >>>>> establish RMRR mappings for PV domains, like it's done in
> >>>>> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> >>>>> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> >>>>> not properly updating the iommu page tables.
> >>>>
> >>>> Sorry, I think this description is somewhat backwards: you're saying
> >>>> what you're doing first, and then saying what the problematic behavior
> >>>> was, but not actually saying what was causing the problematic behavior.
> >>>>
> >>>> Why was {set,clear}_identity_p2m not updating the page tables?
> >>>>
> >>>> I agree with Jan, that figuring that out is a prerequisite for any kind
> >>>> of fix: if `need_iommu_pt_sync()` is false at that point for the
> >>>> hardware domain, then there's a bigger problem than RMRRs not being
> >>>> adjusted properly.
> >>>
> >>> need_iommu_pt_sync is indeed false for a PV hardware domain not
> >>> running in strict mode, see:
> >>>
> >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/iommu.c;h=f8c3bf53bd1793df93d7ddea6564dc929f40c156;hb=HEAD#l192
> >>> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
> >>>
> >>> This is fine for a non-strict PV hardware domain, since it has all the
> >>> host memory (minus memory used by Xen) mapped in the iommu page tables
> >>> except for RMRR regions not marked as reserved in the e820 memory map,
> >>> which are added using set_identity_p2m_entry.
> >>>
> >>> The issue here is that this patch alone doesn't fix the issue for the
> >>> reporter, and there seems to be an additional flush required in order
> >>> to get the issue solved on his end:
> >>>
> >>> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00526.html
> >>>
> >>> My theory is that the system Roman is using is booting with DMA
> >>> remapping enabled in the iommu, and somehow the call to
> >>> iommu_flush_all in intel_iommu_hwdom_init doesn't seem to work
> >>> properly, while calling iommu_iotlb_flush_all does seem to do the
> >>> right thing. I'm still waiting for Roman to come back with the result
> >>> of my last debug patches in order to figure out whether my hypothesis
> >>> above is true.
> >>>
> >>> This however won't still explain the weird behaviour of
> >>> iommu_flush_all, and further debugging will still be required.
> >>
> >> OK; so this patch has four effects, it looks like:
> >>
> >> 1. iommu_legacy_[un]map -> iommu_[un]map
> >> 2. Move iommu ops out of {set,clear}_identity_p2m for PV guests;
> >> open-code them in rmrr_identity_mapping
> >> 3. For non-translated guests, do the operation unconditionally
> >> 4. Add a flush
> > 
> > There's already a flush in iommu_legacy_[un]map, so the flush is also
> > done for both patches, just in different places. The legacy interface
> > does the flush on every call, while the new interface allows to
> > postpone the flush until all iommu page-table operations are done.
> > 
> >>
> >> Regarding #3, the previous patch changed it from "if
> >> iommu_needs_pt_sync" to "if has_iommu_pt"; this one changes it to
> >> "always".  Is that what you intended?
> > 
> > Well, #3 is not actually correct. The code in intel_iommu_hwdom_init
> > and hence setup_hwdom_rmrr will only be called if the domain has an
> > iommu, so has_iommu_pt will always be true when adding RMRR regions.
> > Note rmrr_identity_mapping is the only caller of
> > {set,clear}_identity_p2m against PV guests.
> 
> But if iommu is set the same in both cases, and if the flush happens in
> both cases, then why did this patch work and the previous patch didn't?

iommu_legacy_[un]map uses iommu_iotlb_flush while my patch uses
iommu_iotlb_flush_all. I'm still not sure why this difference in
behaviour, similar to what happens with iommu_flush_all.

> >> I don't really see the point of #2: from the device's perspective, the
> >> "physmap" is the IOMMU for PV guests, and IOMMU+p2m for HVM guests; it
> >> makes sense to have a single place to call for either type of guest,
> >> rather than open-coding every location.
> > 
> > OK, that's fine, but note that {set/clear}_identity_p2m_entry is
> > AFAICT the only p2m function allowed against PV guests, the rest will
> > return some kind of error, which IMO makes it the outlier, hence my
> > proposal to make it available to translated guests only, like the rest
> > of the p2m functions in that file. I just find it confusing that some
> > p2m functions can be used against PV guests, but not others.
> 
> Right, but that's because set_identity_p2m_entry() isn't really about
> the p2m.  It's about making sure that both devices and SMI handlers (or
> whatever) can access certain bits of memory.
> 
> Lots of functions in p2m.c "tolerate" calls for PV guests; and
> guest_physmap_* calls used to actually to IOMMU operations on PV guests
> before XSA 288.
> 
> We could change that to set_identity_physmap_entry() or something if you
> prefer.

OK, let me try to first figure out exactly what's going on and then I
can propose something sensible :).

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02  9:22 [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains Roger Pau Monne
2019-08-02 13:09 ` Jan Beulich
2019-08-12 10:17 ` Paul Durrant
2019-08-12 13:17 ` George Dunlap
2019-08-12 13:56   ` Roger Pau Monné
2019-08-12 14:24     ` George Dunlap
2019-08-12 14:55       ` Roger Pau Monné
2019-08-12 15:15         ` George Dunlap
2019-08-12 15:30           ` Roger Pau Monné

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox