| Message ID | fea51839-4405-4330-8493-c544b9edf035@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mm: assorted adjustments | expand |
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.
--- 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) {
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>