[Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Jan Beulich]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Paul Durrant]

> -----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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[George Dunlap]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Roger Pau Monné]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[George Dunlap]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Roger Pau Monné]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[George Dunlap]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Roger Pau Monné]

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Jan Beulich]

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.

Aiui we still didn't get to the bottom of this. Together with
my (much) earlier reply I think I'll drop this from my list
of pending patches, unless you indicate otherwise.

Jan

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

Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 12:24:29PM +0100, Jan Beulich wrote:
> 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.
> 
> Aiui we still didn't get to the bottom of this. Together with
> my (much) earlier reply I think I'll drop this from my list
> of pending patches, unless you indicate otherwise.

Yes, IIRC Roman said this was likely caused by a firmware issue?

There was an issue as a result of other IOMMU changes and the code
that was already fixed by a different patch, I think this can be
safely dropped.

Thanks, Roger.

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