[PATCH 0/4] x86/mm: assorted adjustments

[PATCH 0/4] x86/mm: assorted adjustments

[Jan Beulich]

1: P2M: write_p2m_entry() is HVM-only anyway
2: P2M: un-indent write_p2m_entry()
3: paging: vCPU host mode is always set
4: shadow: correct shadow_vcpu_init()'s comment

Jan

[PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway

[Jan Beulich]

The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
functions") the function is obviously unreachable for PV guests. Hence
the paging_mode_enabled(d) check is pointless.

Further host mode of a vCPU is always set, by virtue of
paging_vcpu_init() being part of vCPU creation. Hence the
paging_get_hostmode() check is pointless.

With that the v local variable is unnecessary too. Drop the "if()"
conditional and its corresponding "else".

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have to confess that this if() has been puzzling me before.

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,12 +110,7 @@ static int write_p2m_entry(struct p2m_do
                            unsigned int level)
 {
     struct domain *d = p2m->domain;
-    const struct vcpu *v = current;
 
-    if ( v->domain != d )
-        v = d->vcpu ? d->vcpu[0] : NULL;
-    if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v)) ||
-         p2m_is_nestedp2m(p2m) )
     {
         unsigned int oflags;
         mfn_t omfn;
@@ -156,8 +151,6 @@ static int write_p2m_entry(struct p2m_do
               !perms_strictly_increased(oflags, l1e_get_flags(new))) )
             p2m_flush_nestedp2m(d);
     }
-    else
-        safe_write_pte(p, new);
 
     return 0;
 }

Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway

[Andrew Cooper]

On 23/04/2024 3:31 pm, Jan Beulich wrote:
> The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
> functions") the function is obviously unreachable for PV guests.

This doesn't parse.  Do you mean "Since e2b2ff677958 ..." ?

>  Hence
> the paging_mode_enabled(d) check is pointless.
>
> Further host mode of a vCPU is always set, by virtue of
> paging_vcpu_init() being part of vCPU creation. Hence the
> paging_get_hostmode() check is pointless.
>
> With that the v local variable is unnecessary too. Drop the "if()"
> conditional and its corresponding "else".
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I have to confess that this if() has been puzzling me before.

Puzzling yes, but it can't blindly be dropped.

This is the "did the toolstack initiate this update" check.  i.e. I
think it's "bypass the normal side effects of making this update".

I suspect it exists because of improper abstraction between the guest
physmap and the shadow pagetables as-were - which were/are tighly
coupled to vCPUs even for aspects where they shouldn't have been.

For better or worse, the toolstack can add_to_physmap() before it
creates vCPUs, and it will take this path you're trying to delete. 
There may be other cases too; I could see foreign mapping ending up
ticking this too.

Whether we ought to permit a toolstack to do this is a different
question, but seeing as we explicitly intend (eventually for AMX) have a
set_policy call between domain_create() and vcpu_create(), I don't think
we can reasably restrict other hypercalls too in this period.

~Andrew

Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway

[Jan Beulich]

On 23.04.2024 21:29, Andrew Cooper wrote:
> On 23/04/2024 3:31 pm, Jan Beulich wrote:
>> The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
>> functions") the function is obviously unreachable for PV guests.
> 
> This doesn't parse.  Do you mean "Since e2b2ff677958 ..." ?

Well. I'm sure you at least get the point of "the lastest as of", even
if that may not be proper English. I specifically didn't use "since"
because the fact mentioned may have been true before (more or less
obviously). I'd therefore appreciate a wording suggestion which gets
this across.

>>  Hence
>> the paging_mode_enabled(d) check is pointless.
>>
>> Further host mode of a vCPU is always set, by virtue of
>> paging_vcpu_init() being part of vCPU creation. Hence the
>> paging_get_hostmode() check is pointless.
>>
>> With that the v local variable is unnecessary too. Drop the "if()"
>> conditional and its corresponding "else".
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I have to confess that this if() has been puzzling me before.
> 
> Puzzling yes, but it can't blindly be dropped.

And I'm not doing so "blindly". Every part of what is being dropped is
being explained.

> This is the "did the toolstack initiate this update" check.  i.e. I
> think it's "bypass the normal side effects of making this update".

Why would we want to bypass side effects?

> I suspect it exists because of improper abstraction between the guest
> physmap and the shadow pagetables as-were - which were/are tighly
> coupled to vCPUs even for aspects where they shouldn't have been.
> 
> For better or worse, the toolstack can add_to_physmap() before it
> creates vCPUs, and it will take this path you're trying to delete. 
> There may be other cases too; I could see foreign mapping ending up
> ticking this too.
> 
> Whether we ought to permit a toolstack to do this is a different
> question, but seeing as we explicitly intend (eventually for AMX) have a
> set_policy call between domain_create() and vcpu_create(), I don't think
> we can reasably restrict other hypercalls too in this period.

None of which explains what's wrong with the provided justification.
The P2M isn't per-vCPU. Presence of vCPU-s therefore shouldn't matter
for alterations of the P2M.

Jan

Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway

[Jan Beulich]

On 24.04.2024 08:36, Jan Beulich wrote:
> On 23.04.2024 21:29, Andrew Cooper wrote:
>> On 23/04/2024 3:31 pm, Jan Beulich wrote:
>>> The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
>>> functions") the function is obviously unreachable for PV guests.
>>
>> This doesn't parse.  Do you mean "Since e2b2ff677958 ..." ?
> 
> Well. I'm sure you at least get the point of "the lastest as of", even
> if that may not be proper English. I specifically didn't use "since"
> because the fact mentioned may have been true before (more or less
> obviously). I'd therefore appreciate a wording suggestion which gets
> this across.
> 
>>>  Hence
>>> the paging_mode_enabled(d) check is pointless.
>>>
>>> Further host mode of a vCPU is always set, by virtue of
>>> paging_vcpu_init() being part of vCPU creation. Hence the
>>> paging_get_hostmode() check is pointless.
>>>
>>> With that the v local variable is unnecessary too. Drop the "if()"
>>> conditional and its corresponding "else".
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I have to confess that this if() has been puzzling me before.
>>
>> Puzzling yes, but it can't blindly be dropped.
> 
> And I'm not doing so "blindly". Every part of what is being dropped is
> being explained.
> 
>> This is the "did the toolstack initiate this update" check.  i.e. I
>> think it's "bypass the normal side effects of making this update".
> 
> Why would we want to bypass side effects?
> 
>> I suspect it exists because of improper abstraction between the guest
>> physmap and the shadow pagetables as-were - which were/are tighly
>> coupled to vCPUs even for aspects where they shouldn't have been.
>>
>> For better or worse, the toolstack can add_to_physmap() before it
>> creates vCPUs, and it will take this path you're trying to delete. 
>> There may be other cases too; I could see foreign mapping ending up
>> ticking this too.
>>
>> Whether we ought to permit a toolstack to do this is a different
>> question, but seeing as we explicitly intend (eventually for AMX) have a
>> set_policy call between domain_create() and vcpu_create(), I don't think
>> we can reasably restrict other hypercalls too in this period.
> 
> None of which explains what's wrong with the provided justification.
> The P2M isn't per-vCPU. Presence of vCPU-s therefore shouldn't matter
> for alterations of the P2M.

I've gone and checked further: The "side effects" are what the
write_p2m_entry_{pre,post}() hooks would do. Prior to the VM being
started that is a little bit of extra code which all ends up doing
nothing: There's nothing to flush, and there are no shadows to drop.
There's in particular no use of a vCPU anywhere, afaics. Plus, just
to mention it explicitly, the full path was forced anyway for nested
P2Ms, so there's no behavioral change there at all.

In fact I question the correctness of the plain safe_write_pte(),
without p2m_entry_modify(), if that path would have been taken (when
the domain has no vCPU-s yet).

Jan

[PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Jan Beulich]

Drop the inner scope that was left from earlier if/else removal. Take
the opportunity and make the paging_unlock() invocation common to
success and error paths, though.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,49 +110,43 @@ static int write_p2m_entry(struct p2m_do
                            unsigned int level)
 {
     struct domain *d = p2m->domain;
-
+    unsigned int oflags;
+    mfn_t omfn;
+    int rc;
+
+    paging_lock(d);
+
+    if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
+        p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
+
+    oflags = l1e_get_flags(*p);
+    omfn = l1e_get_mfn(*p);
+
+    rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
+                          p2m_flags_to_type(oflags), l1e_get_mfn(new),
+                          omfn, level);
+    if ( !rc )
     {
-        unsigned int oflags;
-        mfn_t omfn;
-        int rc;
-
-        paging_lock(d);
-
-        if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
-            p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
-
-        oflags = l1e_get_flags(*p);
-        omfn = l1e_get_mfn(*p);
-
-        rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
-                              p2m_flags_to_type(oflags), l1e_get_mfn(new),
-                              omfn, level);
-        if ( rc )
-        {
-            paging_unlock(d);
-            return rc;
-        }
-
         safe_write_pte(p, new);
 
         if ( p2m->write_p2m_entry_post )
             p2m->write_p2m_entry_post(p2m, oflags);
+    }
 
-        paging_unlock(d);
+    paging_unlock(d);
 
-        if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
-             (oflags & _PAGE_PRESENT) &&
-             !p2m_get_hostp2m(d)->defer_nested_flush &&
-             /*
-              * We are replacing a valid entry so we need to flush nested p2ms,
-              * unless the only change is an increase in access rights.
-              */
-             (!mfn_eq(omfn, l1e_get_mfn(new)) ||
-              !perms_strictly_increased(oflags, l1e_get_flags(new))) )
-            p2m_flush_nestedp2m(d);
-    }
+    if ( !rc && nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
+         (oflags & _PAGE_PRESENT) &&
+         !p2m_get_hostp2m(d)->defer_nested_flush &&
+         /*
+          * We are replacing a valid entry so we need to flush nested p2ms,
+          * unless the only change is an increase in access rights.
+          */
+         (!mfn_eq(omfn, l1e_get_mfn(new)) ||
+          !perms_strictly_increased(oflags, l1e_get_flags(new))) )
+        p2m_flush_nestedp2m(d);
 
-    return 0;
+    return rc;
 }
 
 // Find the next level's P2M entry, checking for out-of-range gfn's...

Re: [PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Roger Pau Monné]

On Tue, Apr 23, 2024 at 04:32:14PM +0200, Jan Beulich wrote:
> Drop the inner scope that was left from earlier if/else removal. Take
> the opportunity and make the paging_unlock() invocation common to
> success and error paths, though.

TBH I'm not sure I prefer the fact to continue function execution
after an error is found, I specially dislike that you have to add a
!rc check to the nestedhvm conditional block, and because anything
that we further add to the function would also need a !rc check.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I do prefer the extra call to paging_unlock() and early return
from the function in case of error.

Thanks, Roger.

Re: [PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Jan Beulich]

On 24.04.2024 11:16, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 04:32:14PM +0200, Jan Beulich wrote:
>> Drop the inner scope that was left from earlier if/else removal. Take
>> the opportunity and make the paging_unlock() invocation common to
>> success and error paths, though.
> 
> TBH I'm not sure I prefer the fact to continue function execution
> after an error is found, I specially dislike that you have to add a
> !rc check to the nestedhvm conditional block, and because anything
> that we further add to the function would also need a !rc check.
> 
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Albeit I do prefer the extra call to paging_unlock() and early return
> from the function in case of error.

Which puts me in the middle of your preference and the one George voiced
in the context of what is now cc950c49ae6a ("x86/PoD: tie together P2M
update and increment of entry count"). Doing the extra adjustment was
merely in the hope of meeting his desires ...

Jan

[PATCH 3/4] x86/paging: vCPU host mode is always set

[Jan Beulich]

... thanks to paging_vcpu_init() being part of vCPU creation. Further
if paging is enabled on a domain, it's also guaranteed to be either HAP
or shadow. Drop respective unnecessary (parts of) conditionals.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -937,19 +937,12 @@ void paging_dump_vcpu_info(struct vcpu *
     {
         printk("    paging assistance: ");
         if ( paging_mode_shadow(v->domain) )
-        {
-            if ( paging_get_hostmode(v) )
-                printk("shadowed %u-on-%u\n",
-                       paging_get_hostmode(v)->guest_levels,
-                       paging_get_hostmode(v)->shadow.shadow_levels);
-            else
-                printk("not shadowed\n");
-        }
-        else if ( paging_mode_hap(v->domain) && paging_get_hostmode(v) )
+            printk("shadowed %u-on-%u\n",
+                   paging_get_hostmode(v)->guest_levels,
+                   paging_get_hostmode(v)->shadow.shadow_levels);
+        else
             printk("hap, %u levels\n",
                    paging_get_hostmode(v)->guest_levels);
-        else
-            printk("none\n");
     }
 }
 

Re: [PATCH 3/4] x86/paging: vCPU host mode is always set

[Roger Pau Monné]

On Tue, Apr 23, 2024 at 04:32:32PM +0200, Jan Beulich wrote:
> ... thanks to paging_vcpu_init() being part of vCPU creation. Further
> if paging is enabled on a domain, it's also guaranteed to be either HAP
> or shadow. Drop respective unnecessary (parts of) conditionals.

Is there some commit that changed when arch.paging.mode gets set, so
that this is actually safe to do now, but not when the code in
paging_dump_vcpu_info() was introduced?

I get the feeling we want to reference some change here in order to
explain why is now always guaranteed to be set.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/4] x86/paging: vCPU host mode is always set

[Jan Beulich]

On 24.04.2024 11:34, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 04:32:32PM +0200, Jan Beulich wrote:
>> ... thanks to paging_vcpu_init() being part of vCPU creation. Further
>> if paging is enabled on a domain, it's also guaranteed to be either HAP
>> or shadow. Drop respective unnecessary (parts of) conditionals.
> 
> Is there some commit that changed when arch.paging.mode gets set, so
> that this is actually safe to do now, but not when the code in
> paging_dump_vcpu_info() was introduced?
> 
> I get the feeling we want to reference some change here in order to
> explain why is now always guaranteed to be set.

I was indeed meaning to, but when I found the same even in 3.2, I stopped
searching further.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan

Re: [PATCH 3/4] x86/paging: vCPU host mode is always set

[Roger Pau Monné]

On Wed, Apr 24, 2024 at 01:41:25PM +0200, Jan Beulich wrote:
> On 24.04.2024 11:34, Roger Pau Monné wrote:
> > On Tue, Apr 23, 2024 at 04:32:32PM +0200, Jan Beulich wrote:
> >> ... thanks to paging_vcpu_init() being part of vCPU creation. Further
> >> if paging is enabled on a domain, it's also guaranteed to be either HAP
> >> or shadow. Drop respective unnecessary (parts of) conditionals.
> > 
> > Is there some commit that changed when arch.paging.mode gets set, so
> > that this is actually safe to do now, but not when the code in
> > paging_dump_vcpu_info() was introduced?
> > 
> > I get the feeling we want to reference some change here in order to
> > explain why is now always guaranteed to be set.
> 
> I was indeed meaning to, but when I found the same even in 3.2, I stopped
> searching further.

Fair enough :).

Thanks, Roger.

[PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment

[Jan Beulich]

As of the commit referenced below the update_paging_modes() hook is per-
domain and hence also set (already) during domain construction.

Fixes: d0816a9085b5 ("x86/paging: move update_paging_modes() hook")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -99,11 +99,12 @@ int shadow_domain_init(struct domain *d)
     return 0;
 }
 
-/* Setup the shadow-specfic parts of a vcpu struct. Note: The most important
- * job is to initialize the update_paging_modes() function pointer, which is
- * used to initialized the rest of resources. Therefore, it really does not
- * matter to have v->arch.paging.mode pointing to any mode, as long as it can
- * be compiled.
+/*
+ * Setup the shadow-specific parts of a vcpu struct. Note: The
+ * update_paging_modes() function pointer, which is used to initialize other
+ * resources, was already set during domain creation. Therefore it really does
+ * not matter to have v->arch.paging.mode pointing to any (legitimate) mode,
+ * as long as it can be compiled.
  */
 void shadow_vcpu_init(struct vcpu *v)
 {

Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment

[Roger Pau Monné]

On Tue, Apr 23, 2024 at 04:33:09PM +0200, Jan Beulich wrote:
> As of the commit referenced below the update_paging_modes() hook is per-
> domain and hence also set (already) during domain construction.
> 
> Fixes: d0816a9085b5 ("x86/paging: move update_paging_modes() hook")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -99,11 +99,12 @@ int shadow_domain_init(struct domain *d)
>      return 0;
>  }
>  
> -/* Setup the shadow-specfic parts of a vcpu struct. Note: The most important
> - * job is to initialize the update_paging_modes() function pointer, which is
> - * used to initialized the rest of resources. Therefore, it really does not
> - * matter to have v->arch.paging.mode pointing to any mode, as long as it can
> - * be compiled.
> +/*
> + * Setup the shadow-specific parts of a vcpu struct. Note: The
> + * update_paging_modes() function pointer, which is used to initialize other
> + * resources, was already set during domain creation. Therefore it really does
> + * not matter to have v->arch.paging.mode pointing to any (legitimate) mode,
> + * as long as it can be compiled.

Do you need to keep the last sentence?  If update_paging_modes is
already set at domain create, the 'Therefore it really does...'
doesn't seem to make much sense anymore, as it's no longer
shadow_vcpu_init() that sets it.

Possibly with that dropped:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment

[Jan Beulich]

On 24.04.2024 12:06, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 04:33:09PM +0200, Jan Beulich wrote:
>> As of the commit referenced below the update_paging_modes() hook is per-
>> domain and hence also set (already) during domain construction.
>>
>> Fixes: d0816a9085b5 ("x86/paging: move update_paging_modes() hook")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -99,11 +99,12 @@ int shadow_domain_init(struct domain *d)
>>      return 0;
>>  }
>>  
>> -/* Setup the shadow-specfic parts of a vcpu struct. Note: The most important
>> - * job is to initialize the update_paging_modes() function pointer, which is
>> - * used to initialized the rest of resources. Therefore, it really does not
>> - * matter to have v->arch.paging.mode pointing to any mode, as long as it can
>> - * be compiled.
>> +/*
>> + * Setup the shadow-specific parts of a vcpu struct. Note: The
>> + * update_paging_modes() function pointer, which is used to initialize other
>> + * resources, was already set during domain creation. Therefore it really does
>> + * not matter to have v->arch.paging.mode pointing to any (legitimate) mode,
>> + * as long as it can be compiled.
> 
> Do you need to keep the last sentence?  If update_paging_modes is
> already set at domain create, the 'Therefore it really does...'
> doesn't seem to make much sense anymore, as it's no longer
> shadow_vcpu_init() that sets it.

I thought about dropping, but the "any mode does" seemed to me to be still
relevant to mention. I thought about re-wording, too, without coming to any
good alternative. Hence, despite agreeing with you that 'Therefore ...' does
not quite fit (anymore), I left that as is.

> Possibly with that dropped:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan

Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment

[Roger Pau Monné]

On Wed, Apr 24, 2024 at 01:44:39PM +0200, Jan Beulich wrote:
> On 24.04.2024 12:06, Roger Pau Monné wrote:
> > On Tue, Apr 23, 2024 at 04:33:09PM +0200, Jan Beulich wrote:
> >> As of the commit referenced below the update_paging_modes() hook is per-
> >> domain and hence also set (already) during domain construction.
> >>
> >> Fixes: d0816a9085b5 ("x86/paging: move update_paging_modes() hook")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/arch/x86/mm/shadow/common.c
> >> +++ b/xen/arch/x86/mm/shadow/common.c
> >> @@ -99,11 +99,12 @@ int shadow_domain_init(struct domain *d)
> >>      return 0;
> >>  }
> >>  
> >> -/* Setup the shadow-specfic parts of a vcpu struct. Note: The most important
> >> - * job is to initialize the update_paging_modes() function pointer, which is
> >> - * used to initialized the rest of resources. Therefore, it really does not
> >> - * matter to have v->arch.paging.mode pointing to any mode, as long as it can
> >> - * be compiled.
> >> +/*
> >> + * Setup the shadow-specific parts of a vcpu struct. Note: The
> >> + * update_paging_modes() function pointer, which is used to initialize other
> >> + * resources, was already set during domain creation. Therefore it really does
> >> + * not matter to have v->arch.paging.mode pointing to any (legitimate) mode,
> >> + * as long as it can be compiled.
> > 
> > Do you need to keep the last sentence?  If update_paging_modes is
> > already set at domain create, the 'Therefore it really does...'
> > doesn't seem to make much sense anymore, as it's no longer
> > shadow_vcpu_init() that sets it.
> 
> I thought about dropping, but the "any mode does" seemed to me to be still
> relevant to mention. I thought about re-wording, too, without coming to any
> good alternative. Hence, despite agreeing with you that 'Therefore ...' does
> not quite fit (anymore), I left that as is.

To me the "was already set during domain creation" does already imply
it's set to "any (legitimate) mode", and hence the tailing sentence
feels out of place.

> > Possibly with that dropped:
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Feel free to keep the chunk and the Ack, I guess we could always
remove at a later point anyway.

Thanks, Roger.