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

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.

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

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.