Regression in RMRRs identity mapping for PVH Dom0

Regression in RMRRs identity mapping for PVH Dom0

[Elena Ufimtseva]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

Hi                                                                              
                                                                                
There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
This causes pages faults and some long 'hang-like' delays during boot and
device assignments.
                                                                                
During construct_dom0, in PVH path  p2m is being constructed and identity mapped
in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
New code used to map RMRRs invoked from rmrr_identity_mapping                   
checks if p2m entry exists with same type and access and if yes, skips iommu
mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
mapped in IOMMU.
                                                                                
This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.
                                                                                
Thanks!                                                                         
                                                                                
Elena         

[-- Attachment #2: 0001-RMRR-regression-debug-for-PVH-Dom0.patch --]
[-- Type: text/x-diff, Size: 1149 bytes --]

>From fb25216760a0c17447faa1f416cc59341600dc1b Mon Sep 17 00:00:00 2001
From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Date: Wed, 23 Sep 2015 11:47:49 -0400
Subject: [PATCH] RMRR regression debug for PVH Dom0

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 xen/arch/x86/mm/p2m.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b2726bd..16c8938 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -970,8 +970,10 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
     if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
         ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
                             p2m_mmio_direct, p2ma);
-    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
-        ret = 0;
+    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct )
+        if ( a == p2ma && !is_pvh_domain(d) )
+            ret = 0;
+        else ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
     else
     {
         if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Regression in RMRRs identity mapping for PVH Dom0

[Chen, Tiejun]

On 9/23/2015 11:56 PM, Elena Ufimtseva wrote:
> Hi
>
> There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
> new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
> This causes pages faults and some long 'hang-like' delays during boot and
> device assignments.
>
> During construct_dom0, in PVH path  p2m is being constructed and identity mapped
> in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
> New code used to map RMRRs invoked from rmrr_identity_mapping
> checks if p2m entry exists with same type and access and if yes, skips iommu
> mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
> mapped in IOMMU.
>
> This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.

Based on your explanation, sounds pvh always creates this mapping 
beforehand, so what about this?

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index cf8485e..d026845 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -964,7 +964,7 @@ int set_identity_p2m_entry(struct domain *d, 
unsigned long gfn,
      struct p2m_domain *p2m = p2m_get_hostp2m(d);
      int ret;

-    if ( !paging_mode_translate(p2m->domain) )
+    if ( !paging_mode_translate(p2m->domain) || is_pvh_domain(d) )
      {
          if ( !need_iommu(d) )
              return 0;

Thanks
Tiejun

Re: Regression in RMRRs identity mapping for PVH Dom0

[Tim Deegan]

At 15:17 +0800 on 24 Sep (1443107852), Chen, Tiejun wrote:
> On 9/23/2015 11:56 PM, Elena Ufimtseva wrote:
> > Hi
> >
> > There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
> > new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
> > This causes pages faults and some long 'hang-like' delays during boot and
> > device assignments.
> >
> > During construct_dom0, in PVH path  p2m is being constructed and identity mapped
> > in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
> > New code used to map RMRRs invoked from rmrr_identity_mapping
> > checks if p2m entry exists with same type and access and if yes, skips iommu
> > mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
> > mapped in IOMMU.
> >
> > This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.
> 
> Based on your explanation, sounds pvh always creates this mapping 
> beforehand, so what about this?
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index cf8485e..d026845 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -964,7 +964,7 @@ int set_identity_p2m_entry(struct domain *d, 
> unsigned long gfn,
>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>       int ret;
> 
> -    if ( !paging_mode_translate(p2m->domain) )
> +    if ( !paging_mode_translate(p2m->domain) || is_pvh_domain(d) )

Sorry, but that wouldn't be safe. :(  PVH domains need the same
protection as any other paging_mode_translate ones.

AIUI the problem is that before the call to set_identity_p2m_entry(),
PVH dom0 has a p2m entry covering this range but no IOMMU entry.  Is
that right?  So the fix will be to make PVH dom0 construction set up
the IOMMU correctly when it sets up the p2m.

Cheers,

Tim.

Re: Regression in RMRRs identity mapping for PVH Dom0

[Wei Liu]

Hi Elena

On Wed, Sep 23, 2015 at 11:56:12AM -0400, Elena Ufimtseva wrote:
> Hi                                                                              
>                                                                                 
> There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
> new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
> This causes pages faults and some long 'hang-like' delays during boot and
> device assignments.
>                                                                                 
> During construct_dom0, in PVH path  p2m is being constructed and identity mapped
> in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
> New code used to map RMRRs invoked from rmrr_identity_mapping                   
> checks if p2m entry exists with same type and access and if yes, skips iommu
> mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
> mapped in IOMMU.
>                                                                                 
> This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.
>                                                                                 

>From a release point of view, PVH Dom0 is not officially supported so I
don't consider this issue a blocker.

We can backport the proper fix to 4.6.1 if necessary, but I doubt this
is the only fix we need to make PVH Dom0 work on 4.6. Am I right?

Wei.

Re: Regression in RMRRs identity mapping for PVH Dom0

[Jan Beulich]

>>> On 24.09.15 at 11:18, <tim@xen.org> wrote:
> AIUI the problem is that before the call to set_identity_p2m_entry(),
> PVH dom0 has a p2m entry covering this range but no IOMMU entry.  Is
> that right?  So the fix will be to make PVH dom0 construction set up
> the IOMMU correctly when it sets up the p2m.

Right, but with the current way of setting up PVH Dom0 I'm afraid
this will be rather intrusive to implement. Hence, however much I
dislike it, I wonder whether a variant of Elena's change (suitably
annotated with a phv fixme) wouldn't be a reasonable thing for 4.6.
With the switch to HVMlite the Dom0 setup will need to be re-done
anyway afaics.

Elena, as to the actual patch:

>--- a/xen/arch/x86/mm/p2m.c
>+++ b/xen/arch/x86/mm/p2m.c
>@@ -970,8 +970,10 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
>     if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
>         ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>                             p2m_mmio_direct, p2ma);
>-    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
>-        ret = 0;
>+    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct )
>+        if ( a == p2ma && !is_pvh_domain(d) )
>+            ret = 0;
>+        else ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);

Besides this wanting figure braces, why do you pull the a == p2ma
check into the inner if()? If this is because of the P2M getting
populated with p2m_rwx, I think _that_ should be changed rather
than breaking the logic here (or, if done properly, complicating it).
There's no reason I can see to map MMIO regions rwx.

Also I think this wants to cover just hwdom and !iommu_use_hap_pt.

Jan

>     else
>     {
>         if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )

Re: Regression in RMRRs identity mapping for PVH Dom0

[Elena Ufimtseva]

On Thu, Sep 24, 2015 at 04:31:09AM -0600, Jan Beulich wrote:
> >>> On 24.09.15 at 11:18, <tim@xen.org> wrote:
> > AIUI the problem is that before the call to set_identity_p2m_entry(),
> > PVH dom0 has a p2m entry covering this range but no IOMMU entry.  Is
> > that right?  So the fix will be to make PVH dom0 construction set up
> > the IOMMU correctly when it sets up the p2m.
> 
> Right, but with the current way of setting up PVH Dom0 I'm afraid
> this will be rather intrusive to implement. Hence, however much I
> dislike it, I wonder whether a variant of Elena's change (suitably
> annotated with a phv fixme) wouldn't be a reasonable thing for 4.6.
> With the switch to HVMlite the Dom0 setup will need to be re-done
> anyway afaics.

I agree here Jan. The PVH Dom0 up page tables is a sort of special case
on its own. And me, Andrew Cooper and Konrad talked about changing it,
but I have not yet started working on it yet, but I think its in my
plan.

> 
> Elena, as to the actual patch:
> 
> >--- a/xen/arch/x86/mm/p2m.c
> >+++ b/xen/arch/x86/mm/p2m.c
> >@@ -970,8 +970,10 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> >     if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> >         ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> >                             p2m_mmio_direct, p2ma);
> >-    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> >-        ret = 0;
> >+    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct )
> >+        if ( a == p2ma && !is_pvh_domain(d) )
> >+            ret = 0;
> >+        else ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
> 
> Besides this wanting figure braces, why do you pull the a == p2ma
> check into the inner if()? If this is because of the P2M getting
> populated with p2m_rwx, I think _that_ should be changed rather
> than breaking the logic here (or, if done properly, complicating it).
> There's no reason I can see to map MMIO regions rwx.

Yes, that is why I did it, because of rwx. I will modify it. 
> 
> Also I think this wants to cover just hwdom and !iommu_use_hap_pt.

Yes, forgot about this one.
> 
> Jan
> 
> >     else
> >     {
> >         if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
> 
> 

Re: Regression in RMRRs identity mapping for PVH Dom0

[Elena Ufimtseva]

On Thu, Sep 24, 2015 at 10:18:54AM +0100, Tim Deegan wrote:
> At 15:17 +0800 on 24 Sep (1443107852), Chen, Tiejun wrote:
> > On 9/23/2015 11:56 PM, Elena Ufimtseva wrote:
> > > Hi
> > >
> > > There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
> > > new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
> > > This causes pages faults and some long 'hang-like' delays during boot and
> > > device assignments.
> > >
> > > During construct_dom0, in PVH path  p2m is being constructed and identity mapped
> > > in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
> > > New code used to map RMRRs invoked from rmrr_identity_mapping
> > > checks if p2m entry exists with same type and access and if yes, skips iommu
> > > mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
> > > mapped in IOMMU.
> > >
> > > This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.
> > 
> > Based on your explanation, sounds pvh always creates this mapping 
> > beforehand, so what about this?
> > 
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index cf8485e..d026845 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -964,7 +964,7 @@ int set_identity_p2m_entry(struct domain *d, 
> > unsigned long gfn,
> >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >       int ret;
> > 
> > -    if ( !paging_mode_translate(p2m->domain) )
> > +    if ( !paging_mode_translate(p2m->domain) || is_pvh_domain(d) )
> 
> Sorry, but that wouldn't be safe. :(  PVH domains need the same
> protection as any other paging_mode_translate ones.
> 
> AIUI the problem is that before the call to set_identity_p2m_entry(),
> PVH dom0 has a p2m entry covering this range but no IOMMU entry.  Is
> that right?  So the fix will be to make PVH dom0 construction set up
> the IOMMU correctly when it sets up the p2m.

Yes, thats right. Rework of construct_dom0 and its PVH part should help.
> 
> Cheers,
> 
> Tim.

Re: Regression in RMRRs identity mapping for PVH Dom0

[Elena Ufimtseva]

On Thu, Sep 24, 2015 at 11:29:54AM +0100, Wei Liu wrote:
> Hi Elena
> 
> On Wed, Sep 23, 2015 at 11:56:12AM -0400, Elena Ufimtseva wrote:
> > Hi                                                                              
> >                                                                                 
> > There is a regression in RMRR patch 5ae03990c120a7b3067a52d9784c9aa72c0705a6 in
> > new set_identity_p2m_entry. RMRRs are not being mapped in IOMMU for PVH Dom0.
> > This causes pages faults and some long 'hang-like' delays during boot and
> > device assignments.
> >                                                                                 
> > During construct_dom0, in PVH path  p2m is being constructed and identity mapped
> > in IOMMU. The p2m type is p2m_mmio_direct and p2m access p2m_rwx.
> > New code used to map RMRRs invoked from rmrr_identity_mapping                   
> > checks if p2m entry exists with same type and access and if yes, skips iommu
> > mapping. Since there are p2m entries for pvh dom0 iomem, RMRRs are not being
> > mapped in IOMMU.
> >                                                                                 
> > This debug patch attached fixes this and Ill be glad to see if there is a more elegant fix.
> >                                                                                 
> 
> From a release point of view, PVH Dom0 is not officially supported so I
> don't consider this issue a blocker.
> 
Understand.

> We can backport the proper fix to 4.6.1 if necessary, but I doubt this
> is the only fix we need to make PVH Dom0 work on 4.6. Am I right?

Dom0 PVH boots with some glitches on Intel platforms and with some others on
AMD and it will see for sure more patches. But this problem will
make Dom0 on some Intel platforms to hang, throw page faults or may not be able
to boot at all (as I have seend that happening for some devices when
doing work on extra RMRRs).
> 
> Wei.