* [PATCH 0 of 3] Fixes for paging/sharing with AMD processors
@ 2012-03-21 19:22 Andres Lagar-Cavilla
2012-03-21 19:22 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-21 19:22 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, keir, andres, tim, wei.wang2, hongkaixing, adin
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(-)
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-21 19:22 [PATCH 0 of 3] Fixes for paging/sharing with AMD processors Andres Lagar-Cavilla
@ 2012-03-21 19:22 ` Andres Lagar-Cavilla
2012-03-22 9:17 ` Jan Beulich
2012-03-21 19:22 ` [PATCH 2 of 3] Clip mfn to allowable width when building a PTE Andres Lagar-Cavilla
2012-03-21 19:23 ` [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m Andres Lagar-Cavilla
2 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-21 19:22 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, keir, andres, tim, wei.wang2, hongkaixing, adin
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 );
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-21 19:22 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
@ 2012-03-22 9:17 ` Jan Beulich
2012-03-22 10:44 ` Tim Deegan
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2012-03-22 9:17 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: olaf, keir, andres, tim, xen-devel, wei.wang2, hongkaixing, adin
>>> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-22 9:17 ` Jan Beulich
@ 2012-03-22 10:44 ` Tim Deegan
2012-03-22 10:57 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Tim Deegan @ 2012-03-22 10:44 UTC (permalink / raw)
To: Jan Beulich
Cc: olaf, keir, andres, xen-devel, wei.wang2, Andres Lagar-Cavilla,
adin, hongkaixing
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 );
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-22 10:44 ` Tim Deegan
@ 2012-03-22 10:57 ` Jan Beulich
2012-03-22 11:03 ` Tim Deegan
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2012-03-22 10:57 UTC (permalink / raw)
To: Tim Deegan
Cc: olaf, keir, andres, xen-devel, wei.wang2, Andres Lagar-Cavilla,
adin, hongkaixing
>>> 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 );
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-22 10:57 ` Jan Beulich
@ 2012-03-22 11:03 ` Tim Deegan
2012-03-27 15:19 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 22+ messages in thread
From: Tim Deegan @ 2012-03-22 11:03 UTC (permalink / raw)
To: Jan Beulich
Cc: olaf, keir, hongkaixing, andres, xen-devel, wei.wang2,
Andres Lagar-Cavilla, adin
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-22 11:03 ` Tim Deegan
@ 2012-03-27 15:19 ` Andres Lagar-Cavilla
2012-03-29 10:27 ` Tim Deegan
0 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-27 15:19 UTC (permalink / raw)
To: Tim Deegan
Cc: olaf, keir, hongkaixing, andres, xen-devel, wei.wang2,
Andres Lagar-Cavilla, Jan Beulich, adin
> 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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-27 15:19 ` Andres Lagar-Cavilla
@ 2012-03-29 10:27 ` Tim Deegan
2012-03-29 10:57 ` Keir Fraser
2012-03-29 15:05 ` Jan Beulich
0 siblings, 2 replies; 22+ messages in thread
From: Tim Deegan @ 2012-03-29 10:27 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: olaf, keir, andres, xen-devel, wei.wang2, hongkaixing,
Jan Beulich, adin
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 );
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-29 10:27 ` Tim Deegan
@ 2012-03-29 10:57 ` Keir Fraser
2012-03-29 11:09 ` Tim Deegan
2012-03-29 15:05 ` Jan Beulich
1 sibling, 1 reply; 22+ messages in thread
From: Keir Fraser @ 2012-03-29 10:57 UTC (permalink / raw)
To: Tim Deegan, Andres Lagar-Cavilla
Cc: olaf, andres, xen-devel, wei.wang2, hongkaixing, Jan Beulich,
adin
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 );
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-29 10:57 ` Keir Fraser
@ 2012-03-29 11:09 ` Tim Deegan
0 siblings, 0 replies; 22+ messages in thread
From: Tim Deegan @ 2012-03-29 11:09 UTC (permalink / raw)
To: Keir Fraser
Cc: olaf, andres, xen-devel, wei.wang2, Andres Lagar-Cavilla,
Jan Beulich, adin, hongkaixing
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-29 10:27 ` Tim Deegan
2012-03-29 10:57 ` Keir Fraser
@ 2012-03-29 15:05 ` Jan Beulich
1 sibling, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2012-03-29 15:05 UTC (permalink / raw)
To: andres, tim; +Cc: olaf, keir, andres, xen-devel, wei.wang2, hongkaixing, adin
>>> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-21 19:22 [PATCH 0 of 3] Fixes for paging/sharing with AMD processors Andres Lagar-Cavilla
2012-03-21 19:22 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
@ 2012-03-21 19:22 ` Andres Lagar-Cavilla
2012-03-22 9:18 ` Jan Beulich
2012-03-21 19:23 ` [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m Andres Lagar-Cavilla
2 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-21 19:22 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, keir, andres, tim, wei.wang2, hongkaixing, adin
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__
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-21 19:22 ` [PATCH 2 of 3] Clip mfn to allowable width when building a PTE Andres Lagar-Cavilla
@ 2012-03-22 9:18 ` Jan Beulich
2012-03-22 10:50 ` Tim Deegan
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2012-03-22 9:18 UTC (permalink / raw)
To: Andres Lagar-Cavilla, xen-devel
Cc: olaf, keir, andres, tim, wei.wang2, hongkaixing, adin
>>> 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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-22 9:18 ` Jan Beulich
@ 2012-03-22 10:50 ` Tim Deegan
2012-03-22 11:02 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Tim Deegan @ 2012-03-22 10:50 UTC (permalink / raw)
To: Jan Beulich
Cc: olaf, keir, andres, xen-devel, wei.wang2, Andres Lagar-Cavilla,
adin, hongkaixing
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-22 10:50 ` Tim Deegan
@ 2012-03-22 11:02 ` Jan Beulich
2012-03-22 14:54 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2012-03-22 11:02 UTC (permalink / raw)
To: Tim Deegan
Cc: olaf, keir, andres, xen-devel, wei.wang2, Andres Lagar-Cavilla,
adin, hongkaixing
>>> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-22 11:02 ` Jan Beulich
@ 2012-03-22 14:54 ` Andres Lagar-Cavilla
2012-03-22 15:31 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-22 14:54 UTC (permalink / raw)
To: Jan Beulich
Cc: olaf, keir, andres, Tim Deegan, xen-devel, wei.wang2, hongkaixing,
adin
>>>> 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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-22 14:54 ` Andres Lagar-Cavilla
@ 2012-03-22 15:31 ` Jan Beulich
2012-03-27 15:21 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2012-03-22 15:31 UTC (permalink / raw)
To: andres
Cc: olaf, keir, andres, Tim Deegan, xen-devel, wei.wang2, hongkaixing,
adin
>>> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2 of 3] Clip mfn to allowable width when building a PTE
2012-03-22 15:31 ` Jan Beulich
@ 2012-03-27 15:21 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-27 15:21 UTC (permalink / raw)
To: Jan Beulich
Cc: olaf, keir, andres, Tim Deegan, xen-devel, wei.wang2, hongkaixing,
adin
>>>> 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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m
2012-03-21 19:22 [PATCH 0 of 3] Fixes for paging/sharing with AMD processors Andres Lagar-Cavilla
2012-03-21 19:22 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
2012-03-21 19:22 ` [PATCH 2 of 3] Clip mfn to allowable width when building a PTE Andres Lagar-Cavilla
@ 2012-03-21 19:23 ` Andres Lagar-Cavilla
2012-03-22 10:55 ` Tim Deegan
2 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-21 19:23 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, keir, andres, tim, wei.wang2, hongkaixing, adin
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);
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m
2012-03-21 19:23 ` [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m Andres Lagar-Cavilla
@ 2012-03-22 10:55 ` Tim Deegan
2012-03-22 14:47 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 22+ messages in thread
From: Tim Deegan @ 2012-03-22 10:55 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: olaf, keir, andres, xen-devel, wei.wang2, hongkaixing, adin
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m
2012-03-22 10:55 ` Tim Deegan
@ 2012-03-22 14:47 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-22 14:47 UTC (permalink / raw)
To: Tim Deegan; +Cc: olaf, keir, andres, xen-devel, wei.wang2, hongkaixing, adin
> 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.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0 of 3] RFC Paging support for AMD NPT V2
@ 2012-03-01 3:15 Andres Lagar-Cavilla
2012-03-01 3:15 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
0 siblings, 1 reply; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-01 3:15 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, andres, tim, wei.wang2, hongkaixing, adin
There has been some progress, but still no joy. Definitely not intended for
inclusion at this point.
Tim, Wei, I added a Xen command line toggle to disable IOMMU and P2M table
sharing.
Tim, I verified that changes to p2m-pt.c don't break shadow mode (64bit
hypervisor and Win 7 guest).
Hongkaixing, I incorporated your suggestion in patch 2, so I should add your
Signed-off-by eventually. Please review.
Olaf, I do not see errors when mapping pages prior to eviction. Let me know how
it goes for you.
If I unleash xenpaging on a domain for a non-ambitious target (64 MiBs paged
out for one minute), we sail by just fine. But if I kick things up a notch,
still dying on an VMEXIT_SHUTDOWN (a.k.a. triple fault). Not sure at all what
to do next.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>
xen/drivers/passthrough/iommu.c | 2 +
xen/arch/x86/mm/p2m-pt.c | 56 ++++++++++++++++++++++++++++------------
xen/arch/x86/mm/mem_event.c | 7 +++-
xen/arch/x86/mm/mem_sharing.c | 7 +++++
4 files changed, 53 insertions(+), 19 deletions(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables
2012-03-01 3:15 [PATCH 0 of 3] RFC Paging support for AMD NPT V2 Andres Lagar-Cavilla
@ 2012-03-01 3:15 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 22+ messages in thread
From: Andres Lagar-Cavilla @ 2012-03-01 3:15 UTC (permalink / raw)
To: xen-devel; +Cc: olaf, andres, tim, wei.wang2, hongkaixing, adin
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 6532b3695d7b -r e4013da987e2 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 );
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-03-29 15:05 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-21 19:22 [PATCH 0 of 3] Fixes for paging/sharing with AMD processors Andres Lagar-Cavilla
2012-03-21 19:22 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
2012-03-22 9:17 ` Jan Beulich
2012-03-22 10:44 ` Tim Deegan
2012-03-22 10:57 ` Jan Beulich
2012-03-22 11:03 ` Tim Deegan
2012-03-27 15:19 ` Andres Lagar-Cavilla
2012-03-29 10:27 ` Tim Deegan
2012-03-29 10:57 ` Keir Fraser
2012-03-29 11:09 ` Tim Deegan
2012-03-29 15:05 ` Jan Beulich
2012-03-21 19:22 ` [PATCH 2 of 3] Clip mfn to allowable width when building a PTE Andres Lagar-Cavilla
2012-03-22 9:18 ` Jan Beulich
2012-03-22 10:50 ` Tim Deegan
2012-03-22 11:02 ` Jan Beulich
2012-03-22 14:54 ` Andres Lagar-Cavilla
2012-03-22 15:31 ` Jan Beulich
2012-03-27 15:21 ` Andres Lagar-Cavilla
2012-03-21 19:23 ` [PATCH 3 of 3] x86/mm: Teach paging to page table-based p2m Andres Lagar-Cavilla
2012-03-22 10:55 ` Tim Deegan
2012-03-22 14:47 ` Andres Lagar-Cavilla
-- strict thread matches above, loose matches on Subject: below --
2012-03-01 3:15 [PATCH 0 of 3] RFC Paging support for AMD NPT V2 Andres Lagar-Cavilla
2012-03-01 3:15 ` [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables Andres Lagar-Cavilla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).