Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment
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
> >>
> >> --- 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é
Feel free to keep the chunk and the Ack, I guess we could always
remove at a later point anyway.
Thanks, Roger.
Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment
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
>>
>> --- 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é
Thanks.
Jan
Re: [PATCH 4/4] x86/shadow: correct shadow_vcpu_init()'s comment
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
>
> --- 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é
Thanks, Roger.
