[PATCH 0 of 3] Fixes for paging/sharing with AMD processors

[PATCH 0 of 3] Fixes for paging/sharing with AMD processors

[Andres Lagar-Cavilla]

These are patches previously posted as RFC towards making paging and sharing
work for AMD NPT.

They are now ready for inclusion in the tree. They do not yet completely fix
paging in AMD, but each fixes an existing bug:

- Add "no-sharept" cli to allow disabling of sharing of iommu and p2m tables.
- Mask high order bits when installing INVALID_MFN on a pte, so those bits
don't trample over flags such as _PAGE_NX. I've used a suggestion from Tim
Deegan in this patch, so I took the liberty of adding his SoB.
- Teach paging to the page table-based p2m (p2m-pt). This involves fixing a few
if's and ASSERT's.

They've been tested to not break existing modes (shadow, AMD hap, dom0 as test
for pv domains)

Signed-off-by: Andres Lagar-Cavilla<andres@lagarcavilla.org>
Signed-off-by: Tim Deegan <tim@xen.org>

 xen/drivers/passthrough/iommu.c |   2 ++
 xen/include/asm-x86/page.h      |  12 ++++++++----
 xen/arch/x86/mm/p2m-pt.c        |  30 +++++++++++++++++++-----------
 3 files changed, 29 insertions(+), 15 deletions(-)

[PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Andres Lagar-Cavilla]

 xen/drivers/passthrough/iommu.c |  2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


The default is 1, and the command line parameter sets to 1. Disabling may be
desired if the host will contain VMs that do paging, sharing or mem access, and
won't be doing passthrough. These two features are mutually exclusive for AMD
processors.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r d7e417afcbe4 -r 642c0e6a01c2 xen/drivers/passthrough/iommu.c
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha
             iommu_dom0_strict = 1;
         else if ( !strcmp(s, "sharept") )
             iommu_hap_pt_share = 1;
+        else if ( !strcmp(s, "no-sharept") )
+            iommu_hap_pt_share = 0;
 
         s = ss + 1;
     } while ( ss );

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Jan Beulich]

 >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> xen/drivers/passthrough/iommu.c |  2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> 
> The default is 1, and the command line parameter sets to 1. Disabling may be
> desired if the host will contain VMs that do paging, sharing or mem access, 
> and
> won't be doing passthrough. These two features are mutually exclusive for 
> AMD
> processors.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r d7e417afcbe4 -r 642c0e6a01c2 xen/drivers/passthrough/iommu.c
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha
>              iommu_dom0_strict = 1;
>          else if ( !strcmp(s, "sharept") )
>              iommu_hap_pt_share = 1;
> +        else if ( !strcmp(s, "no-sharept") )
> +            iommu_hap_pt_share = 0;

Taking the description above into consideration - why not simply
replace the pointless enabling option with the disabling one?

Jan

>  
>          s = ss + 1;
>      } while ( ss );
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Tim Deegan]

At 09:17 +0000 on 22 Mar (1332407837), Jan Beulich wrote:
>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> > The default is 1, and the command line parameter sets to 1. Disabling may be
> > desired if the host will contain VMs that do paging, sharing or mem access, 
> > and
> > won't be doing passthrough. These two features are mutually exclusive for 
> > AMD
> > processors.
> > 
> > Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> > 
> > diff -r d7e417afcbe4 -r 642c0e6a01c2 xen/drivers/passthrough/iommu.c
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha
> >              iommu_dom0_strict = 1;
> >          else if ( !strcmp(s, "sharept") )
> >              iommu_hap_pt_share = 1;
> > +        else if ( !strcmp(s, "no-sharept") )
> > +            iommu_hap_pt_share = 0;
> 
> Taking the description above into consideration - why not simply
> replace the pointless enabling option with the disabling one?

Let's just fix the whole function to handle 'no-' the way the main
parser does:

-----

IOMMU: clean up handling of 'foo' and 'no-foo' command-line options

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 8180cb3895af xen/drivers/passthrough/iommu.c
--- a/xen/drivers/passthrough/iommu.c	Thu Mar 22 10:26:45 2012 +0000
+++ b/xen/drivers/passthrough/iommu.c	Thu Mar 22 10:38:48 2012 +0000
@@ -57,36 +57,41 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_
 static void __init parse_iommu_param(char *s)
 {
     char *ss;
+    int val;
 
     do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
         ss = strchr(s, ',');
         if ( ss )
             *ss = '\0';
 
         if ( !parse_bool(s) )
-            iommu_enabled = 0;
+            iommu_enabled = val;
         else if ( !strcmp(s, "force") || !strcmp(s, "required") )
-            force_iommu = 1;
+            force_iommu = val;
         else if ( !strcmp(s, "workaround_bios_bug") )
             iommu_workaround_bios_bug = 1;
         else if ( !strcmp(s, "verbose") )
-            iommu_verbose = 1;
-        else if ( !strcmp(s, "no-snoop") )
-            iommu_snoop = 0;
-        else if ( !strcmp(s, "no-qinval") )
-            iommu_qinval = 0;
-        else if ( !strcmp(s, "no-intremap") )
-            iommu_intremap = 0;
+            iommu_verbose = val;
+        else if ( !strcmp(s, "snoop") )
+            iommu_snoop = val;
+        else if ( !strcmp(s, "qinval") )
+            iommu_qinval = val;
+        else if ( !strcmp(s, "intremap") )
+            iommu_intremap = val;
         else if ( !strcmp(s, "debug") )
-            iommu_debug = 1;
+            iommu_debug = val;
         else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
-            amd_iommu_perdev_intremap = 1;
+            amd_iommu_perdev_intremap = val;
         else if ( !strcmp(s, "dom0-passthrough") )
-            iommu_passthrough = 1;
+            iommu_passthrough = val;
         else if ( !strcmp(s, "dom0-strict") )
-            iommu_dom0_strict = 1;
+            iommu_dom0_strict = val;
         else if ( !strcmp(s, "sharept") )
-            iommu_hap_pt_share = 1;
+            iommu_hap_pt_share = val;
 
         s = ss + 1;
     } while ( ss );

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Jan Beulich]

>>> On 22.03.12 at 11:44, Tim Deegan <tim@xen.org> wrote:
> At 09:17 +0000 on 22 Mar (1332407837), Jan Beulich wrote:
>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
>> > The default is 1, and the command line parameter sets to 1. Disabling may 
> be
>> > desired if the host will contain VMs that do paging, sharing or mem access, 
> 
>> > and
>> > won't be doing passthrough. These two features are mutually exclusive for 
>> > AMD
>> > processors.
>> > 
>> > Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>> > 
>> > diff -r d7e417afcbe4 -r 642c0e6a01c2 xen/drivers/passthrough/iommu.c
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha
>> >              iommu_dom0_strict = 1;
>> >          else if ( !strcmp(s, "sharept") )
>> >              iommu_hap_pt_share = 1;
>> > +        else if ( !strcmp(s, "no-sharept") )
>> > +            iommu_hap_pt_share = 0;
>> 
>> Taking the description above into consideration - why not simply
>> replace the pointless enabling option with the disabling one?
> 
> Let's just fix the whole function to handle 'no-' the way the main
> parser does:

Good idea, but ...

> -----
> 
> IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 8180cb3895af xen/drivers/passthrough/iommu.c
> --- a/xen/drivers/passthrough/iommu.c	Thu Mar 22 10:26:45 2012 +0000
> +++ b/xen/drivers/passthrough/iommu.c	Thu Mar 22 10:38:48 2012 +0000
> @@ -57,36 +57,41 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> +    int val;
>  
>      do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;
> +
>          ss = strchr(s, ',');
>          if ( ss )
>              *ss = '\0';
>  
>          if ( !parse_bool(s) )
> -            iommu_enabled = 0;
> +            iommu_enabled = val;

... this one must remain unchanged, and the handling of the "no-"
prefix should come afterwards.

Jan

>          else if ( !strcmp(s, "force") || !strcmp(s, "required") )
> -            force_iommu = 1;
> +            force_iommu = val;
>          else if ( !strcmp(s, "workaround_bios_bug") )
>              iommu_workaround_bios_bug = 1;
>          else if ( !strcmp(s, "verbose") )
> -            iommu_verbose = 1;
> -        else if ( !strcmp(s, "no-snoop") )
> -            iommu_snoop = 0;
> -        else if ( !strcmp(s, "no-qinval") )
> -            iommu_qinval = 0;
> -        else if ( !strcmp(s, "no-intremap") )
> -            iommu_intremap = 0;
> +            iommu_verbose = val;
> +        else if ( !strcmp(s, "snoop") )
> +            iommu_snoop = val;
> +        else if ( !strcmp(s, "qinval") )
> +            iommu_qinval = val;
> +        else if ( !strcmp(s, "intremap") )
> +            iommu_intremap = val;
>          else if ( !strcmp(s, "debug") )
> -            iommu_debug = 1;
> +            iommu_debug = val;
>          else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
> -            amd_iommu_perdev_intremap = 1;
> +            amd_iommu_perdev_intremap = val;
>          else if ( !strcmp(s, "dom0-passthrough") )
> -            iommu_passthrough = 1;
> +            iommu_passthrough = val;
>          else if ( !strcmp(s, "dom0-strict") )
> -            iommu_dom0_strict = 1;
> +            iommu_dom0_strict = val;
>          else if ( !strcmp(s, "sharept") )
> -            iommu_hap_pt_share = 1;
> +            iommu_hap_pt_share = val;
>  
>          s = ss + 1;
>      } while ( ss );

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Tim Deegan]

At 10:57 +0000 on 22 Mar (1332413830), Jan Beulich wrote:
> >          if ( !parse_bool(s) )
> > -            iommu_enabled = 0;
> > +            iommu_enabled = val;
> 
> ... this one must remain unchanged, and the handling of the "no-"
> prefix should come afterwards.

Meh.  I reserve the right to set 'iommu=no-yes'. :)  
That is to say, I don't think it makes any real difference.

Tim.

> >          else if ( !strcmp(s, "force") || !strcmp(s, "required") )
> > -            force_iommu = 1;
> > +            force_iommu = val;
> >          else if ( !strcmp(s, "workaround_bios_bug") )
> >              iommu_workaround_bios_bug = 1;
> >          else if ( !strcmp(s, "verbose") )
> > -            iommu_verbose = 1;
> > -        else if ( !strcmp(s, "no-snoop") )
> > -            iommu_snoop = 0;
> > -        else if ( !strcmp(s, "no-qinval") )
> > -            iommu_qinval = 0;
> > -        else if ( !strcmp(s, "no-intremap") )
> > -            iommu_intremap = 0;
> > +            iommu_verbose = val;
> > +        else if ( !strcmp(s, "snoop") )
> > +            iommu_snoop = val;
> > +        else if ( !strcmp(s, "qinval") )
> > +            iommu_qinval = val;
> > +        else if ( !strcmp(s, "intremap") )
> > +            iommu_intremap = val;
> >          else if ( !strcmp(s, "debug") )
> > -            iommu_debug = 1;
> > +            iommu_debug = val;
> >          else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
> > -            amd_iommu_perdev_intremap = 1;
> > +            amd_iommu_perdev_intremap = val;
> >          else if ( !strcmp(s, "dom0-passthrough") )
> > -            iommu_passthrough = 1;
> > +            iommu_passthrough = val;
> >          else if ( !strcmp(s, "dom0-strict") )
> > -            iommu_dom0_strict = 1;
> > +            iommu_dom0_strict = val;
> >          else if ( !strcmp(s, "sharept") )
> > -            iommu_hap_pt_share = 1;
> > +            iommu_hap_pt_share = val;
> >  
> >          s = ss + 1;
> >      } while ( ss );
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Andres Lagar-Cavilla]

> At 10:57 +0000 on 22 Mar (1332413830), Jan Beulich wrote:
>> >          if ( !parse_bool(s) )
>> > -            iommu_enabled = 0;
>> > +            iommu_enabled = val;
>>
>> ... this one must remain unchanged, and the handling of the "no-"
>> prefix should come afterwards.
>
> Meh.  I reserve the right to set 'iommu=no-yes'. :)
> That is to say, I don't think it makes any real difference.

Well, it'd be great if one version or the other goes in before 4.2 closes :)
Thanks
Andres

>
> Tim.
>
>> >          else if ( !strcmp(s, "force") || !strcmp(s, "required") )
>> > -            force_iommu = 1;
>> > +            force_iommu = val;
>> >          else if ( !strcmp(s, "workaround_bios_bug") )
>> >              iommu_workaround_bios_bug = 1;
>> >          else if ( !strcmp(s, "verbose") )
>> > -            iommu_verbose = 1;
>> > -        else if ( !strcmp(s, "no-snoop") )
>> > -            iommu_snoop = 0;
>> > -        else if ( !strcmp(s, "no-qinval") )
>> > -            iommu_qinval = 0;
>> > -        else if ( !strcmp(s, "no-intremap") )
>> > -            iommu_intremap = 0;
>> > +            iommu_verbose = val;
>> > +        else if ( !strcmp(s, "snoop") )
>> > +            iommu_snoop = val;
>> > +        else if ( !strcmp(s, "qinval") )
>> > +            iommu_qinval = val;
>> > +        else if ( !strcmp(s, "intremap") )
>> > +            iommu_intremap = val;
>> >          else if ( !strcmp(s, "debug") )
>> > -            iommu_debug = 1;
>> > +            iommu_debug = val;
>> >          else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
>> > -            amd_iommu_perdev_intremap = 1;
>> > +            amd_iommu_perdev_intremap = val;
>> >          else if ( !strcmp(s, "dom0-passthrough") )
>> > -            iommu_passthrough = 1;
>> > +            iommu_passthrough = val;
>> >          else if ( !strcmp(s, "dom0-strict") )
>> > -            iommu_dom0_strict = 1;
>> > +            iommu_dom0_strict = val;
>> >          else if ( !strcmp(s, "sharept") )
>> > -            iommu_hap_pt_share = 1;
>> > +            iommu_hap_pt_share = val;
>> >
>> >          s = ss + 1;
>> >      } while ( ss );
>>
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Tim Deegan]

At 08:19 -0700 on 27 Mar (1332836366), Andres Lagar-Cavilla wrote:
> > At 10:57 +0000 on 22 Mar (1332413830), Jan Beulich wrote:
> >> >          if ( !parse_bool(s) )
> >> > -            iommu_enabled = 0;
> >> > +            iommu_enabled = val;
> >>
> >> ... this one must remain unchanged, and the handling of the "no-"
> >> prefix should come afterwards.
> >
> > Meh.  I reserve the right to set 'iommu=no-yes'. :)
> > That is to say, I don't think it makes any real difference.
> 
> Well, it'd be great if one version or the other goes in before 4.2 closes :)

OK, here's a version that keeps the 'iommu_enabled = 0' (so we never
turn it _on_ in this function).  Jan, does that address your concern?

Any IOMMU maintainers have an opinion about this patch?

Keir, I think you're maintainer for this file - can I get an Ack?

Cheers,

Tim.

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1333016609 -3600
# Node ID 67418ef0095b93b0084c2fae5fd87bdae714a69e
# Parent  74d2af0b56ea7e6072bdfd6e493be5f108808bb7
IOMMU: clean up handling of 'foo' and 'no-foo' command-line options

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 74d2af0b56ea -r 67418ef0095b xen/drivers/passthrough/iommu.c
--- a/xen/drivers/passthrough/iommu.c	Wed Mar 28 16:59:02 2012 +0200
+++ b/xen/drivers/passthrough/iommu.c	Thu Mar 29 11:23:29 2012 +0100
@@ -57,8 +57,13 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_
 static void __init parse_iommu_param(char *s)
 {
     char *ss;
+    int val;
 
     do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
         ss = strchr(s, ',');
         if ( ss )
             *ss = '\0';
@@ -66,27 +71,27 @@ static void __init parse_iommu_param(cha
         if ( !parse_bool(s) )
             iommu_enabled = 0;
         else if ( !strcmp(s, "force") || !strcmp(s, "required") )
-            force_iommu = 1;
+            force_iommu = val;
         else if ( !strcmp(s, "workaround_bios_bug") )
             iommu_workaround_bios_bug = 1;
         else if ( !strcmp(s, "verbose") )
-            iommu_verbose = 1;
-        else if ( !strcmp(s, "no-snoop") )
-            iommu_snoop = 0;
-        else if ( !strcmp(s, "no-qinval") )
-            iommu_qinval = 0;
-        else if ( !strcmp(s, "no-intremap") )
-            iommu_intremap = 0;
+            iommu_verbose = val;
+        else if ( !strcmp(s, "snoop") )
+            iommu_snoop = val;
+        else if ( !strcmp(s, "qinval") )
+            iommu_qinval = val;
+        else if ( !strcmp(s, "intremap") )
+            iommu_intremap = val;
         else if ( !strcmp(s, "debug") )
-            iommu_debug = 1;
+            iommu_debug = val;
         else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
-            amd_iommu_perdev_intremap = 1;
+            amd_iommu_perdev_intremap = val;
         else if ( !strcmp(s, "dom0-passthrough") )
-            iommu_passthrough = 1;
+            iommu_passthrough = val;
         else if ( !strcmp(s, "dom0-strict") )
-            iommu_dom0_strict = 1;
+            iommu_dom0_strict = val;
         else if ( !strcmp(s, "sharept") )
-            iommu_hap_pt_share = 1;
+            iommu_hap_pt_share = val;
 
         s = ss + 1;
     } while ( ss );

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Keir Fraser]

On 29/03/2012 11:27, "Tim Deegan" <tim@xen.org> wrote:

> At 08:19 -0700 on 27 Mar (1332836366), Andres Lagar-Cavilla wrote:
>>> At 10:57 +0000 on 22 Mar (1332413830), Jan Beulich wrote:
>>>>>          if ( !parse_bool(s) )
>>>>> -            iommu_enabled = 0;
>>>>> +            iommu_enabled = val;
>>>> 
>>>> ... this one must remain unchanged, and the handling of the "no-"
>>>> prefix should come afterwards.
>>> 
>>> Meh.  I reserve the right to set 'iommu=no-yes'. :)
>>> That is to say, I don't think it makes any real difference.
>> 
>> Well, it'd be great if one version or the other goes in before 4.2 closes :)
> 
> OK, here's a version that keeps the 'iommu_enabled = 0' (so we never
> turn it _on_ in this function).  Jan, does that address your concern?
> 
> Any IOMMU maintainers have an opinion about this patch?
> 
> Keir, I think you're maintainer for this file - can I get an Ack?

Why does workaround_bios_bug not respect val like all the other options?
Apart from that:
Acked-by: Keir Fraser <keir@xen.org>

> Cheers,
> 
> Tim.
> 
> # HG changeset patch
> # User Tim Deegan <tim@xen.org>
> # Date 1333016609 -3600
> # Node ID 67418ef0095b93b0084c2fae5fd87bdae714a69e
> # Parent  74d2af0b56ea7e6072bdfd6e493be5f108808bb7
> IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 74d2af0b56ea -r 67418ef0095b xen/drivers/passthrough/iommu.c
> --- a/xen/drivers/passthrough/iommu.c Wed Mar 28 16:59:02 2012 +0200
> +++ b/xen/drivers/passthrough/iommu.c Thu Mar 29 11:23:29 2012 +0100
> @@ -57,8 +57,13 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> +    int val;
>  
>      do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;
> +
>          ss = strchr(s, ',');
>          if ( ss )
>              *ss = '\0';
> @@ -66,27 +71,27 @@ static void __init parse_iommu_param(cha
>          if ( !parse_bool(s) )
>              iommu_enabled = 0;
>          else if ( !strcmp(s, "force") || !strcmp(s, "required") )
> -            force_iommu = 1;
> +            force_iommu = val;
>          else if ( !strcmp(s, "workaround_bios_bug") )
>              iommu_workaround_bios_bug = 1;
>          else if ( !strcmp(s, "verbose") )
> -            iommu_verbose = 1;
> -        else if ( !strcmp(s, "no-snoop") )
> -            iommu_snoop = 0;
> -        else if ( !strcmp(s, "no-qinval") )
> -            iommu_qinval = 0;
> -        else if ( !strcmp(s, "no-intremap") )
> -            iommu_intremap = 0;
> +            iommu_verbose = val;
> +        else if ( !strcmp(s, "snoop") )
> +            iommu_snoop = val;
> +        else if ( !strcmp(s, "qinval") )
> +            iommu_qinval = val;
> +        else if ( !strcmp(s, "intremap") )
> +            iommu_intremap = val;
>          else if ( !strcmp(s, "debug") )
> -            iommu_debug = 1;
> +            iommu_debug = val;
>          else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
> -            amd_iommu_perdev_intremap = 1;
> +            amd_iommu_perdev_intremap = val;
>          else if ( !strcmp(s, "dom0-passthrough") )
> -            iommu_passthrough = 1;
> +            iommu_passthrough = val;
>          else if ( !strcmp(s, "dom0-strict") )
> -            iommu_dom0_strict = 1;
> +            iommu_dom0_strict = val;
>          else if ( !strcmp(s, "sharept") )
> -            iommu_hap_pt_share = 1;
> +            iommu_hap_pt_share = val;
>  
>          s = ss + 1;
>      } while ( ss );
> 
> 

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Tim Deegan]

At 11:57 +0100 on 29 Mar (1333022256), Keir Fraser wrote:
> On 29/03/2012 11:27, "Tim Deegan" <tim@xen.org> wrote:
> 
> > At 08:19 -0700 on 27 Mar (1332836366), Andres Lagar-Cavilla wrote:
> >>> At 10:57 +0000 on 22 Mar (1332413830), Jan Beulich wrote:
> >>>>>          if ( !parse_bool(s) )
> >>>>> -            iommu_enabled = 0;
> >>>>> +            iommu_enabled = val;
> >>>> 
> >>>> ... this one must remain unchanged, and the handling of the "no-"
> >>>> prefix should come afterwards.
> >>> 
> >>> Meh.  I reserve the right to set 'iommu=no-yes'. :)
> >>> That is to say, I don't think it makes any real difference.
> >> 
> >> Well, it'd be great if one version or the other goes in before 4.2 closes :)
> > 
> > OK, here's a version that keeps the 'iommu_enabled = 0' (so we never
> > turn it _on_ in this function).  Jan, does that address your concern?
> > 
> > Any IOMMU maintainers have an opinion about this patch?
> > 
> > Keir, I think you're maintainer for this file - can I get an Ack?
> 
> Why does workaround_bios_bug not respect val like all the other options?

Oversight.  I've changed it to '= val' too.

> Apart from that:
> Acked-by: Keir Fraser <keir@xen.org>

Ta.  Applied, with that one change. 

Tim.

Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

[Jan Beulich]

>>> Tim Deegan <tim@xen.org> 03/29/12 12:27 PM >>>
>OK, here's a version that keeps the 'iommu_enabled = 0' (so we never
>turn it _on_ in this function).  Jan, does that address your concern?

Yes, thanks.

Jan

[PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Andres Lagar-Cavilla]

 xen/include/asm-x86/page.h |  12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)


Otherwise, INVALID_MFN tramples over high order bits used for additional flags.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 642c0e6a01c2 -r e325da663345 xen/include/asm-x86/page.h
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -107,13 +107,17 @@
 
 /* Construct a pte from a pfn and access flags. */
 #define l1e_from_pfn(pfn, flags)   \
-    ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) })
+    ((l1_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
+                        | put_pte_flags(flags) })
 #define l2e_from_pfn(pfn, flags)   \
-    ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) })
+    ((l2_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
+                        | put_pte_flags(flags) })
 #define l3e_from_pfn(pfn, flags)   \
-    ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) })
+    ((l3_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
+                        | put_pte_flags(flags) })
 #define l4e_from_pfn(pfn, flags)   \
-    ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) })
+    ((l4_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
+                        | put_pte_flags(flags) })
 
 /* Construct a pte from a physical address and access flags. */
 #ifndef __ASSEMBLY__

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Jan Beulich]

 >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> xen/include/asm-x86/page.h |  12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> 
> Otherwise, INVALID_MFN tramples over high order bits used for additional 
> flags.

But is passing INVALID_PFN into these macros valid/sensible in the first
place?

Jan

> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Signed-off-by: Tim Deegan <tim@xen.org>
> 
> diff -r 642c0e6a01c2 -r e325da663345 xen/include/asm-x86/page.h
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -107,13 +107,17 @@
>  
>  /* Construct a pte from a pfn and access flags. */
>  #define l1e_from_pfn(pfn, flags)   \
> -    ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> })
> +    ((l1_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> +                        | put_pte_flags(flags) })
>  #define l2e_from_pfn(pfn, flags)   \
> -    ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> })
> +    ((l2_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> +                        | put_pte_flags(flags) })
>  #define l3e_from_pfn(pfn, flags)   \
> -    ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> })
> +    ((l3_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> +                        | put_pte_flags(flags) })
>  #define l4e_from_pfn(pfn, flags)   \
> -    ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> })
> +    ((l4_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> +                        | put_pte_flags(flags) })
>  
>  /* Construct a pte from a physical address and access flags. */
>  #ifndef __ASSEMBLY__
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Tim Deegan]

At 09:18 +0000 on 22 Mar (1332407899), Jan Beulich wrote:
>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
> > xen/include/asm-x86/page.h |  12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > 
> > Otherwise, INVALID_MFN tramples over high order bits used for additional 
> > flags.
> 
> But is passing INVALID_PFN into these macros valid/sensible in the first
> place?

The p2m code uses pte layout even for entries that don't have the
_PAGE_PRESENT bit set.  We can:
 - mask out in these macros, making everything safe;
 - make new macros just for p2m code;
 - rewrite p2m callers not to use INVALID_MFN; or
 - have the p2m code explicitly replace INVALID_MFN with some other
   value when callers specify it.

I'm happy to do it this way but I guess maybe it slows down paths that
don't need the mask.

Tim.

> > Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > 
> > diff -r 642c0e6a01c2 -r e325da663345 xen/include/asm-x86/page.h
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -107,13 +107,17 @@
> >  
> >  /* Construct a pte from a pfn and access flags. */
> >  #define l1e_from_pfn(pfn, flags)   \
> > -    ((l1_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> > })
> > +    ((l1_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> > +                        | put_pte_flags(flags) })
> >  #define l2e_from_pfn(pfn, flags)   \
> > -    ((l2_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> > })
> > +    ((l2_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> > +                        | put_pte_flags(flags) })
> >  #define l3e_from_pfn(pfn, flags)   \
> > -    ((l3_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> > })
> > +    ((l3_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> > +                        | put_pte_flags(flags) })
> >  #define l4e_from_pfn(pfn, flags)   \
> > -    ((l4_pgentry_t) { ((intpte_t)(pfn) << PAGE_SHIFT) | put_pte_flags(flags) 
> > })
> > +    ((l4_pgentry_t) { (((intpte_t)(pfn) << PAGE_SHIFT) & PADDR_MASK)    \
> > +                        | put_pte_flags(flags) })
> >  
> >  /* Construct a pte from a physical address and access flags. */
> >  #ifndef __ASSEMBLY__
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org 
> > http://lists.xen.org/xen-devel 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Jan Beulich]

>>> On 22.03.12 at 11:50, Tim Deegan <tim@xen.org> wrote:
> At 09:18 +0000 on 22 Mar (1332407899), Jan Beulich wrote:
>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@lagarcavilla.org> wrote:
>> > xen/include/asm-x86/page.h |  12 ++++++++----
>> >  1 files changed, 8 insertions(+), 4 deletions(-)
>> > 
>> > 
>> > Otherwise, INVALID_MFN tramples over high order bits used for additional 
>> > flags.
>> 
>> But is passing INVALID_PFN into these macros valid/sensible in the first
>> place?
> 
> The p2m code uses pte layout even for entries that don't have the
> _PAGE_PRESENT bit set.  We can:
>  - mask out in these macros, making everything safe;
>  - make new macros just for p2m code;
>  - rewrite p2m callers not to use INVALID_MFN; or
>  - have the p2m code explicitly replace INVALID_MFN with some other
>    value when callers specify it.

As the transformation backwards doesn't yield INVALID_MFN anyway,
I'd prefer one of those options.

> I'm happy to do it this way but I guess maybe it slows down paths that
> don't need the mask.

Yes, that was one of the concerns - less because of the slowdown,
but more because the generated code then becomes even worse to
look at (which at least I happen to be required to do now and then,
albeit admittedly less frequently over time, i.e. as the general code
quality improves).

Jan

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Andres Lagar-Cavilla]

>>>> On 22.03.12 at 11:50, Tim Deegan <tim@xen.org> wrote:
>> At 09:18 +0000 on 22 Mar (1332407899), Jan Beulich wrote:
>>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla
>>> <andres@lagarcavilla.org> wrote:
>>> > xen/include/asm-x86/page.h |  12 ++++++++----
>>> >  1 files changed, 8 insertions(+), 4 deletions(-)
>>> >
>>> >
>>> > Otherwise, INVALID_MFN tramples over high order bits used for
>>> additional
>>> > flags.
>>>
>>> But is passing INVALID_PFN into these macros valid/sensible in the
>>> first
>>> place?
>>
>> The p2m code uses pte layout even for entries that don't have the
>> _PAGE_PRESENT bit set.  We can:
>>  - mask out in these macros, making everything safe;
>>  - make new macros just for p2m code;
>>  - rewrite p2m callers not to use INVALID_MFN; or
>>  - have the p2m code explicitly replace INVALID_MFN with some other
>>    value when callers specify it.
>
> As the transformation backwards doesn't yield INVALID_MFN anyway,
> I'd prefer one of those options.

The whole issue originates in p2m-pt, which uses l1e_from_pfn with
INVALID_PFN -- sometimes. My favourite option would be to replace that
with p2m_l1e_from_pfn, which does the masking of the pfn and then calls
l1e_from_pfn.

Limits the masking to the only places where it is relevant. If that's ok
with you I'll prepare and resend.

Andres

>
>> I'm happy to do it this way but I guess maybe it slows down paths that
>> don't need the mask.
>
> Yes, that was one of the concerns - less because of the slowdown,
> but more because the generated code then becomes even worse to
> look at (which at least I happen to be required to do now and then,
> albeit admittedly less frequently over time, i.e. as the general code
> quality improves).
>
> Jan
>
>

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Jan Beulich]

>>> On 22.03.12 at 15:54, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> wrote:
>>>>> On 22.03.12 at 11:50, Tim Deegan <tim@xen.org> wrote:
>>> At 09:18 +0000 on 22 Mar (1332407899), Jan Beulich wrote:
>>>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla
>>>> <andres@lagarcavilla.org> wrote:
>>>> > xen/include/asm-x86/page.h |  12 ++++++++----
>>>> >  1 files changed, 8 insertions(+), 4 deletions(-)
>>>> >
>>>> >
>>>> > Otherwise, INVALID_MFN tramples over high order bits used for
>>>> additional
>>>> > flags.
>>>>
>>>> But is passing INVALID_PFN into these macros valid/sensible in the
>>>> first
>>>> place?
>>>
>>> The p2m code uses pte layout even for entries that don't have the
>>> _PAGE_PRESENT bit set.  We can:
>>>  - mask out in these macros, making everything safe;
>>>  - make new macros just for p2m code;
>>>  - rewrite p2m callers not to use INVALID_MFN; or
>>>  - have the p2m code explicitly replace INVALID_MFN with some other
>>>    value when callers specify it.
>>
>> As the transformation backwards doesn't yield INVALID_MFN anyway,
>> I'd prefer one of those options.
> 
> The whole issue originates in p2m-pt, which uses l1e_from_pfn with
> INVALID_PFN -- sometimes. My favourite option would be to replace that
> with p2m_l1e_from_pfn, which does the masking of the pfn and then calls
> l1e_from_pfn.
> 
> Limits the masking to the only places where it is relevant. If that's ok
> with you I'll prepare and resend.

Yes, that sounds good to me.

Jan

Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE

[Andres Lagar-Cavilla]

>>>> On 22.03.12 at 15:54, "Andres Lagar-Cavilla" <andres@lagarcavilla.org>
>>>> wrote:
>>>>>> On 22.03.12 at 11:50, Tim Deegan <tim@xen.org> wrote:
>>>> At 09:18 +0000 on 22 Mar (1332407899), Jan Beulich wrote:
>>>>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla
>>>>> <andres@lagarcavilla.org> wrote:
>>>>> > xen/include/asm-x86/page.h |  12 ++++++++----
>>>>> >  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>> >
>>>>> >
>>>>> > Otherwise, INVALID_MFN tramples over high order bits used for
>>>>> additional
>>>>> > flags.
>>>>>
>>>>> But is passing INVALID_PFN into these macros valid/sensible in the
>>>>> first
>>>>> place?
>>>>
>>>> The p2m code uses pte layout even for entries that don't have the
>>>> _PAGE_PRESENT bit set.  We can:
>>>>  - mask out in these macros, making everything safe;
>>>>  - make new macros just for p2m code;
>>>>  - rewrite p2m callers not to use INVALID_MFN; or
>>>>  - have the p2m code explicitly replace INVALID_MFN with some other
>>>>    value when callers specify it.
>>>
>>> As the transformation backwards doesn't yield INVALID_MFN anyway,
>>> I'd prefer one of those options.
>>
>> The whole issue originates in p2m-pt, which uses l1e_from_pfn with
>> INVALID_PFN -- sometimes. My favourite option would be to replace that
>> with p2m_l1e_from_pfn, which does the masking of the pfn and then calls
>> l1e_from_pfn.
>>
>> Limits the masking to the only places where it is relevant. If that's ok
>> with you I'll prepare and resend.
>
> Yes, that sounds good to me.

Posted here
http://lists.xen.org/archives/html/xen-devel/2012-03/msg01982.html

(just making sure it doesn't slip through the cracks)
Andres

>
> Jan
>
>

[PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m

[Andres Lagar-Cavilla]

 xen/arch/x86/mm/p2m-pt.c |  30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)


The p2m-pt.c code, used by both shadow and AMD NPT modes, was not aware of
paging types, and the implications those types have on p2m entries. Add support
to the page table-based p2m to understand the paging types. This is a necessary
step towards enabling memory paging on AMD NPT mode, but not yet the full
solution.

Tested not to break neither shadow mode nor "normal" (i.e. no paging) AMD NPT
mode.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r e325da663345 -r 7704c9e0f5ff xen/arch/x86/mm/p2m-pt.c
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -77,6 +77,9 @@ static unsigned long p2m_type_to_flags(p
     case p2m_invalid:
     case p2m_mmio_dm:
     case p2m_populate_on_demand:
+    case p2m_ram_paging_out:
+    case p2m_ram_paged:
+    case p2m_ram_paging_in:
     default:
         return flags;
     case p2m_ram_ro:
@@ -168,7 +171,7 @@ p2m_next_level(struct p2m_domain *p2m, m
                                       shift, max)) )
         return 0;
 
-    /* PoD: Not present doesn't imply empty. */
+    /* PoD/paging: Not present doesn't imply empty. */
     if ( !l1e_get_flags(*p2m_entry) )
     {
         struct page_info *pg;
@@ -384,7 +387,7 @@ p2m_set_entry(struct p2m_domain *p2m, un
                                    0, L1_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
         
-        if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct) )
+        if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct) || p2m_is_paging(p2mt) )
             entry_content = l1e_from_pfn(mfn_x(mfn),
                                          p2m_type_to_flags(p2mt, mfn));
         else
@@ -615,11 +618,12 @@ pod_retry_l1:
                            sizeof(l1e));
             
     if ( ret == 0 ) {
+        unsigned long l1e_mfn = l1e_get_pfn(l1e);
         p2mt = p2m_flags_to_type(l1e_get_flags(l1e));
-        ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt));
+        ASSERT( (l1e_mfn != INVALID_MFN || !p2m_is_ram(p2mt)) ||
+                (l1e_mfn == INVALID_MFN && p2m_is_paging(p2mt)) );
 
-        if ( p2m_flags_to_type(l1e_get_flags(l1e))
-             == p2m_populate_on_demand )
+        if ( p2mt == p2m_populate_on_demand )
         {
             /* The read has succeeded, so we know that the mapping
              * exits at this point.  */
@@ -641,7 +645,7 @@ pod_retry_l1:
         }
 
         if ( p2m_is_valid(p2mt) || p2m_is_grant(p2mt) )
-            mfn = _mfn(l1e_get_pfn(l1e));
+            mfn = _mfn(l1e_mfn);
         else 
             /* XXX see above */
             p2mt = p2m_mmio_dm;
@@ -663,6 +667,8 @@ p2m_gfn_to_mfn(struct p2m_domain *p2m, u
     paddr_t addr = ((paddr_t)gfn) << PAGE_SHIFT;
     l2_pgentry_t *l2e;
     l1_pgentry_t *l1e;
+    unsigned long l1e_flags;
+    p2m_type_t l1t;
 
     ASSERT(paging_mode_translate(p2m->domain));
 
@@ -781,10 +787,12 @@ pod_retry_l2:
     l1e = map_domain_page(mfn_x(mfn));
     l1e += l1_table_offset(addr);
 pod_retry_l1:
-    if ( (l1e_get_flags(*l1e) & _PAGE_PRESENT) == 0 )
+    l1e_flags = l1e_get_flags(*l1e);
+    l1t = p2m_flags_to_type(l1e_flags);
+    if ( ((l1e_flags & _PAGE_PRESENT) == 0) && (!p2m_is_paging(l1t)) )
     {
         /* PoD: Try to populate */
-        if ( p2m_flags_to_type(l1e_get_flags(*l1e)) == p2m_populate_on_demand )
+        if ( l1t == p2m_populate_on_demand )
         {
             if ( q & P2M_ALLOC ) {
                 if ( !p2m_pod_demand_populate(p2m, gfn, PAGE_ORDER_4K, q) )
@@ -792,15 +800,15 @@ pod_retry_l1:
             } else
                 *t = p2m_populate_on_demand;
         }
-    
+ 
         unmap_domain_page(l1e);
         return _mfn(INVALID_MFN);
     }
     mfn = _mfn(l1e_get_pfn(*l1e));
-    *t = p2m_flags_to_type(l1e_get_flags(*l1e));
+    *t = l1t;
     unmap_domain_page(l1e);
 
-    ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
+    ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t) || p2m_is_paging(*t));
     if ( page_order )
         *page_order = PAGE_ORDER_4K;
     return (p2m_is_valid(*t) || p2m_is_grant(*t)) ? mfn : _mfn(INVALID_MFN);

Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m

[Tim Deegan]

Hi, 

The last version of this patch had the beginnings of an interlock to
avoid iommu-pt-sharing and p2m-fu happening at the same time.  I
suggested taht it wasn't complete enough, but it seems to have gone away
entirely!

Also: 

At 15:23 -0400 on 21 Mar (1332343380), Andres Lagar-Cavilla wrote:
> @@ -615,11 +618,12 @@ pod_retry_l1:
>                             sizeof(l1e));
>              
>      if ( ret == 0 ) {
> +        unsigned long l1e_mfn = l1e_get_pfn(l1e);
>          p2mt = p2m_flags_to_type(l1e_get_flags(l1e));
> -        ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt));
> +        ASSERT( (l1e_mfn != INVALID_MFN || !p2m_is_ram(p2mt)) ||
> +                (l1e_mfn == INVALID_MFN && p2m_is_paging(p2mt)) );

I guess, given the discussion in the other subthread, that this ASSERT
always passes, and should be using mfn_valid() instead?

Cheers,

Tim.

Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m

[Andres Lagar-Cavilla]

> Hi,
>
> The last version of this patch had the beginnings of an interlock to
> avoid iommu-pt-sharing and p2m-fu happening at the same time.  I
> suggested taht it wasn't complete enough, but it seems to have gone away
> entirely!

Tim,
a follow up patch would allow enabling paging on AMD, with the interlock
in place. It would be of the form:

diff -r 0f8002f1efad xen/arch/x86/mm/mem_event.c
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -563,8 +563,11 @@ int mem_event_domctl(struct domain *d, x
             if ( !hap_enabled(d) )
                 break;

-            /* Currently only EPT is supported */
-            if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
+            /* Currently EPT or AMD with no iommu/hap page table sharing are
+             * supported */
+            if ( !((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) ||
+                   ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
+                     !need_iommu(d))) )
                 break;

             rc = -EXDEV;

Right now I haven't posted that as paging on AMD doesn't fully work. The
current patch moves us closer into that direction.

The interlock itself would need more fleshing out methinks. AMD needs
hap_pt_share to be disabled in order to enable paging (and in the fullness
of time, mem access). But more generally, wouldn't both Intel and AMD
require !need_iommu()? Also, is it enough to just check during
enable-p2m-foo time? What about a "p2m-foo on" flag to prevent passthrough
from happening at a later time, as well?

>
> Also:
>
> At 15:23 -0400 on 21 Mar (1332343380), Andres Lagar-Cavilla wrote:
>> @@ -615,11 +618,12 @@ pod_retry_l1:
>>                             sizeof(l1e));
>>
>>      if ( ret == 0 ) {
>> +        unsigned long l1e_mfn = l1e_get_pfn(l1e);
>>          p2mt = p2m_flags_to_type(l1e_get_flags(l1e));
>> -        ASSERT(l1e_get_pfn(l1e) != INVALID_MFN || !p2m_is_ram(p2mt));
>> +        ASSERT( (l1e_mfn != INVALID_MFN || !p2m_is_ram(p2mt)) ||
>> +                (l1e_mfn == INVALID_MFN && p2m_is_paging(p2mt)) );
>
> I guess, given the discussion in the other subthread, that this ASSERT
> always passes, and should be using mfn_valid() instead?

Correct right now. Depends on where we end up wrt to patch #2 of this series.

Thanks
Andres
>
> Cheers,
>
> Tim.
>