diff mbox series

[v6,1/4] KVM: arm64: Introduce cache maintenance callbacks for guest stage-2

Message ID 20210616095200.38008-2-wangyanan55@huawei.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Improve efficiency of stage2 page table | expand

Commit Message

wangyanan (Y) June 16, 2021, 9:51 a.m. UTC
To prepare for performing guest CMOs in the fault handlers in pgtable.c,
introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.

The new callbacks are specific for guest stage-2, so they will only be
initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

wangyanan (Y) June 17, 2021, 6:48 a.m. UTC | #1
Hi Marc,

On 2021/6/16 21:21, Marc Zyngier wrote:
> Hi Yanan,
>
> On Wed, 16 Jun 2021 10:51:57 +0100,
> Yanan Wang <wangyanan55@huawei.com> wrote:
>> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
>> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
>>
>> The new callbacks are specific for guest stage-2, so they will only be
>> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
>> index c3674c47d48c..302eca32e0af 100644
>> --- a/arch/arm64/include/asm/kvm_pgtable.h
>> +++ b/arch/arm64/include/asm/kvm_pgtable.h
>> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
>>    *			in the current context.
>>    * @virt_to_phys:	Convert a virtual address mapped in the current context
>>    *			into a physical address.
>> + * @flush_dcache:	Clean data cache for a guest page address range before
>> + *			creating the corresponding stage-2 mapping.
> Please don't reintroduce the word 'flush'. We are really trying to
> move away from it as it doesn't describe what we want to do.
I agree with this. I intended to make the names short and laconic, but this
missed the information about the callback's actual behaviors.
> Here this
> should be 'clean_invalidate_dcache' which, despite being a mouthful,
> describe accurately what we expect it to do.
Sure, I will change the name as you suggested.
> The comment is also missing the invalidate part, and we shouldn't
> assume that this is only used for S2 mapping.
Ok, will refine the comment. I think something like"Clean and invalidate the
date cache for the specified memory address range" may be generic enough.
>> + * @flush_icache:	Invalidate instruction cache for a guest page address
>> + *			range before creating or updating the corresponding
>> + *			stage-2 mapping.
> Same thing here; this should be 'invalidate_icache', and the comment
> cleaned up.
Thanks, I will also correct this part.

Besides the callback names and comments, is there anything else that still
needs some adjustment in the other three patches? :)

Regards,
Yanan
.
>>    */
>>   struct kvm_pgtable_mm_ops {
>>   	void*		(*zalloc_page)(void *arg);
>> @@ -54,6 +59,8 @@ struct kvm_pgtable_mm_ops {
>>   	int		(*page_count)(void *addr);
>>   	void*		(*phys_to_virt)(phys_addr_t phys);
>>   	phys_addr_t	(*virt_to_phys)(void *addr);
>> +	void		(*flush_dcache)(void *addr, size_t size);
>> +	void		(*flush_icache)(void *addr, size_t size);
>>   };
>>   
>>   /**
> Thanks,
>
> 	M.
>
Marc Zyngier June 17, 2021, 8:03 a.m. UTC | #2
On Thu, 17 Jun 2021 07:48:29 +0100,
"wangyanan (Y)" <wangyanan55@huawei.com> wrote:
> 
> Hi Marc,
> 
> On 2021/6/16 21:21, Marc Zyngier wrote:
> > Hi Yanan,
> > 
> > On Wed, 16 Jun 2021 10:51:57 +0100,
> > Yanan Wang <wangyanan55@huawei.com> wrote:
> >> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
> >> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
> >> 
> >> The new callbacks are specific for guest stage-2, so they will only be
> >> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
> >> 
> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >> 
> >> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> >> index c3674c47d48c..302eca32e0af 100644
> >> --- a/arch/arm64/include/asm/kvm_pgtable.h
> >> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> >> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
> >>    *			in the current context.
> >>    * @virt_to_phys:	Convert a virtual address mapped in the current context
> >>    *			into a physical address.
> >> + * @flush_dcache:	Clean data cache for a guest page address range before
> >> + *			creating the corresponding stage-2 mapping.
> > Please don't reintroduce the word 'flush'. We are really trying to
> > move away from it as it doesn't describe what we want to do.
> I agree with this. I intended to make the names short and laconic, but this
> missed the information about the callback's actual behaviors.
> > Here this
> > should be 'clean_invalidate_dcache' which, despite being a mouthful,
> > describe accurately what we expect it to do.
> Sure, I will change the name as you suggested.
> > The comment is also missing the invalidate part, and we shouldn't
> > assume that this is only used for S2 mapping.
> Ok, will refine the comment. I think something like"Clean and invalidate the
> date cache for the specified memory address range" may be generic enough.
> >> + * @flush_icache:	Invalidate instruction cache for a guest page address
> >> + *			range before creating or updating the corresponding
> >> + *			stage-2 mapping.
> > Same thing here; this should be 'invalidate_icache', and the comment
> > cleaned up.
> Thanks, I will also correct this part.
> 
> Besides the callback names and comments, is there anything else that still
> needs some adjustment in the other three patches? :)

It looks pretty good so far, much nicer than the previous versions.

I have a small nit on the last patch, which should be dead easy to
address. I'm currently running a bunch of tests, hopefully nothing bad
will come out of it.

If you respin it shortly, that nothing fails, and unless someone
shouts, I'll queue it for -next.

Thanks,

	M.
wangyanan (Y) June 17, 2021, 8:22 a.m. UTC | #3
On 2021/6/17 16:03, Marc Zyngier wrote:
> On Thu, 17 Jun 2021 07:48:29 +0100,
> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>> Hi Marc,
>>
>> On 2021/6/16 21:21, Marc Zyngier wrote:
>>> Hi Yanan,
>>>
>>> On Wed, 16 Jun 2021 10:51:57 +0100,
>>> Yanan Wang <wangyanan55@huawei.com> wrote:
>>>> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
>>>> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
>>>>
>>>> The new callbacks are specific for guest stage-2, so they will only be
>>>> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
>>>>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
>>>> index c3674c47d48c..302eca32e0af 100644
>>>> --- a/arch/arm64/include/asm/kvm_pgtable.h
>>>> +++ b/arch/arm64/include/asm/kvm_pgtable.h
>>>> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
>>>>     *			in the current context.
>>>>     * @virt_to_phys:	Convert a virtual address mapped in the current context
>>>>     *			into a physical address.
>>>> + * @flush_dcache:	Clean data cache for a guest page address range before
>>>> + *			creating the corresponding stage-2 mapping.
>>> Please don't reintroduce the word 'flush'. We are really trying to
>>> move away from it as it doesn't describe what we want to do.
>> I agree with this. I intended to make the names short and laconic, but this
>> missed the information about the callback's actual behaviors.
>>> Here this
>>> should be 'clean_invalidate_dcache' which, despite being a mouthful,
>>> describe accurately what we expect it to do.
>> Sure, I will change the name as you suggested.
>>> The comment is also missing the invalidate part, and we shouldn't
>>> assume that this is only used for S2 mapping.
>> Ok, will refine the comment. I think something like"Clean and invalidate the
>> date cache for the specified memory address range" may be generic enough.
>>>> + * @flush_icache:	Invalidate instruction cache for a guest page address
>>>> + *			range before creating or updating the corresponding
>>>> + *			stage-2 mapping.
>>> Same thing here; this should be 'invalidate_icache', and the comment
>>> cleaned up.
>> Thanks, I will also correct this part.
>>
>> Besides the callback names and comments, is there anything else that still
>> needs some adjustment in the other three patches? :)
> It looks pretty good so far, much nicer than the previous versions.
>
> I have a small nit on the last patch, which should be dead easy to
> address. I'm currently running a bunch of tests, hopefully nothing bad
> will come out of it.
>
> If you respin it shortly, that nothing fails, and unless someone
> shouts, I'll queue it for -next.
It would be nice, thanks!
I will address the nit and respin the series soon.

Thanks,
Yanan
.
> Thanks,
>
> 	M.
>
Marc Zyngier June 17, 2021, 8:44 a.m. UTC | #4
On Thu, 17 Jun 2021 09:22:51 +0100,
"wangyanan (Y)" <wangyanan55@huawei.com> wrote:
> 
> 
> 
> On 2021/6/17 16:03, Marc Zyngier wrote:
> > On Thu, 17 Jun 2021 07:48:29 +0100,
> > "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
> >> Hi Marc,
> >> 
> >> On 2021/6/16 21:21, Marc Zyngier wrote:
> >>> Hi Yanan,
> >>> 
> >>> On Wed, 16 Jun 2021 10:51:57 +0100,
> >>> Yanan Wang <wangyanan55@huawei.com> wrote:
> >>>> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
> >>>> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
> >>>> 
> >>>> The new callbacks are specific for guest stage-2, so they will only be
> >>>> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
> >>>> 
> >>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> >>>> ---
> >>>>    arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
> >>>>    1 file changed, 7 insertions(+)
> >>>> 
> >>>> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> >>>> index c3674c47d48c..302eca32e0af 100644
> >>>> --- a/arch/arm64/include/asm/kvm_pgtable.h
> >>>> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> >>>> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
> >>>>     *			in the current context.
> >>>>     * @virt_to_phys:	Convert a virtual address mapped in the current context
> >>>>     *			into a physical address.
> >>>> + * @flush_dcache:	Clean data cache for a guest page address range before
> >>>> + *			creating the corresponding stage-2 mapping.
> >>> Please don't reintroduce the word 'flush'. We are really trying to
> >>> move away from it as it doesn't describe what we want to do.
> >> I agree with this. I intended to make the names short and laconic, but this
> >> missed the information about the callback's actual behaviors.
> >>> Here this
> >>> should be 'clean_invalidate_dcache' which, despite being a mouthful,
> >>> describe accurately what we expect it to do.
> >> Sure, I will change the name as you suggested.
> >>> The comment is also missing the invalidate part, and we shouldn't
> >>> assume that this is only used for S2 mapping.
> >> Ok, will refine the comment. I think something like"Clean and invalidate the
> >> date cache for the specified memory address range" may be generic enough.
> >>>> + * @flush_icache:	Invalidate instruction cache for a guest page address
> >>>> + *			range before creating or updating the corresponding
> >>>> + *			stage-2 mapping.
> >>> Same thing here; this should be 'invalidate_icache', and the comment
> >>> cleaned up.
> >> Thanks, I will also correct this part.
> >> 
> >> Besides the callback names and comments, is there anything else that still
> >> needs some adjustment in the other three patches? :)
> > It looks pretty good so far, much nicer than the previous versions.
> > 
> > I have a small nit on the last patch, which should be dead easy to
> > address. I'm currently running a bunch of tests, hopefully nothing bad
> > will come out of it.
> > 
> > If you respin it shortly, that nothing fails, and unless someone
> > shouts, I'll queue it for -next.
> It would be nice, thanks!
> I will address the nit and respin the series soon.

By the way, what the status of your selftest series that originally
came with this series? Are you planning to respin it? It would be
useful to have something that checks for regressions, and that series
did seem to do the trick.

Thanks,

	M.
wangyanan (Y) June 17, 2021, 9:43 a.m. UTC | #5
Hi Marc,

On 2021/6/17 16:44, Marc Zyngier wrote:
> On Thu, 17 Jun 2021 09:22:51 +0100,
> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>>
>>
>> On 2021/6/17 16:03, Marc Zyngier wrote:
>>> On Thu, 17 Jun 2021 07:48:29 +0100,
>>> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>>>> Hi Marc,
>>>>
>>>> On 2021/6/16 21:21, Marc Zyngier wrote:
>>>>> Hi Yanan,
>>>>>
>>>>> On Wed, 16 Jun 2021 10:51:57 +0100,
>>>>> Yanan Wang <wangyanan55@huawei.com> wrote:
>>>>>> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
>>>>>> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
>>>>>>
>>>>>> The new callbacks are specific for guest stage-2, so they will only be
>>>>>> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
>>>>>>
>>>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>>>> ---
>>>>>>     arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
>>>>>> index c3674c47d48c..302eca32e0af 100644
>>>>>> --- a/arch/arm64/include/asm/kvm_pgtable.h
>>>>>> +++ b/arch/arm64/include/asm/kvm_pgtable.h
>>>>>> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
>>>>>>      *			in the current context.
>>>>>>      * @virt_to_phys:	Convert a virtual address mapped in the current context
>>>>>>      *			into a physical address.
>>>>>> + * @flush_dcache:	Clean data cache for a guest page address range before
>>>>>> + *			creating the corresponding stage-2 mapping.
>>>>> Please don't reintroduce the word 'flush'. We are really trying to
>>>>> move away from it as it doesn't describe what we want to do.
>>>> I agree with this. I intended to make the names short and laconic, but this
>>>> missed the information about the callback's actual behaviors.
>>>>> Here this
>>>>> should be 'clean_invalidate_dcache' which, despite being a mouthful,
>>>>> describe accurately what we expect it to do.
>>>> Sure, I will change the name as you suggested.
>>>>> The comment is also missing the invalidate part, and we shouldn't
>>>>> assume that this is only used for S2 mapping.
>>>> Ok, will refine the comment. I think something like"Clean and invalidate the
>>>> date cache for the specified memory address range" may be generic enough.
>>>>>> + * @flush_icache:	Invalidate instruction cache for a guest page address
>>>>>> + *			range before creating or updating the corresponding
>>>>>> + *			stage-2 mapping.
>>>>> Same thing here; this should be 'invalidate_icache', and the comment
>>>>> cleaned up.
>>>> Thanks, I will also correct this part.
>>>>
>>>> Besides the callback names and comments, is there anything else that still
>>>> needs some adjustment in the other three patches? :)
>>> It looks pretty good so far, much nicer than the previous versions.
>>>
>>> I have a small nit on the last patch, which should be dead easy to
>>> address. I'm currently running a bunch of tests, hopefully nothing bad
>>> will come out of it.
>>>
>>> If you respin it shortly, that nothing fails, and unless someone
>>> shouts, I'll queue it for -next.
>> It would be nice, thanks!
>> I will address the nit and respin the series soon.
> By the way, what the status of your selftest series that originally
> came with this series? Are you planning to respin it? It would be
> useful to have something that checks for regressions, and that series
> did seem to do the trick.
Actually they have already gone into upstream, since v5.13-rc1. :)
The path is tools/testing/selftests/kvm/kvm_page_table_test.c, so it
will be much convenient to test a 5.13 kernel, you can also have a try.

I am using the original test data from v3 in the cover-letter because
I think the test results will be almost the same with a different kernel.

Thanks,
Yanan
.
> Thanks,
>
> 	M.
>
Marc Zyngier June 17, 2021, 10:43 a.m. UTC | #6
On Thu, 17 Jun 2021 10:43:23 +0100,
"wangyanan (Y)" <wangyanan55@huawei.com> wrote:
> 
> Hi Marc,
> 
> On 2021/6/17 16:44, Marc Zyngier wrote:
> > By the way, what the status of your selftest series that originally
> > came with this series? Are you planning to respin it? It would be
> > useful to have something that checks for regressions, and that series
> > did seem to do the trick.
> Actually they have already gone into upstream, since v5.13-rc1. :)
> The path is tools/testing/selftests/kvm/kvm_page_table_test.c, so it
> will be much convenient to test a 5.13 kernel, you can also have a try.

Ah, I missed it! Good stuff.

	M.
Fuad Tabba June 18, 2021, 8:50 a.m. UTC | #7
Hi Yanan,

On Wed, Jun 16, 2021 at 10:52 AM Yanan Wang <wangyanan55@huawei.com> wrote:
>
> To prepare for performing guest CMOs in the fault handlers in pgtable.c,
> introduce two cache maintenance callbacks in struct kvm_pgtable_mm_ops.
>
> The new callbacks are specific for guest stage-2, so they will only be
> initialized in 'struct kvm_pgtable_mm_ops kvm_s2_mm_ops'.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_pgtable.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index c3674c47d48c..302eca32e0af 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -44,6 +44,11 @@ typedef u64 kvm_pte_t;
>   *                     in the current context.
>   * @virt_to_phys:      Convert a virtual address mapped in the current context
>   *                     into a physical address.
> + * @flush_dcache:      Clean data cache for a guest page address range before
> + *                     creating the corresponding stage-2 mapping.
> + * @flush_icache:      Invalidate instruction cache for a guest page address
> + *                     range before creating or updating the corresponding
> + *                     stage-2 mapping.
>   */
>  struct kvm_pgtable_mm_ops {
>         void*           (*zalloc_page)(void *arg);
> @@ -54,6 +59,8 @@ struct kvm_pgtable_mm_ops {
>         int             (*page_count)(void *addr);
>         void*           (*phys_to_virt)(phys_addr_t phys);
>         phys_addr_t     (*virt_to_phys)(void *addr);
> +       void            (*flush_dcache)(void *addr, size_t size);
> +       void            (*flush_icache)(void *addr, size_t size);
>  };
>

Just to add to Marc's comment on naming, flush_dcache is in this case
a clean and invalidate: I see that in patch 4 it eventually does a
civac. So, yes, although it is a mouthful, I think it should be
dcache_clean_inval and not just dcache_clean. An alternative, if it's
acceptable by Marc and the others, is to name the parameters dcmo/icmo
or something like that, where the nature of the maintenance operation
is not necessarily tied to the name.

For reference, this is the patch Marc mentioned, where we're trying to
fix the naming to make it consistent with Arm terminology (Arm doesn't
define what a flush is):
https://lore.kernel.org/linux-arm-kernel/20210524083001.2586635-19-tabba@google.com/

Otherwise:
Reviewed-by: Fuad Tabba <tabba@google.com>

Cheers,
/fuad


>  /**
> --
> 2.23.0
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
index c3674c47d48c..302eca32e0af 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -44,6 +44,11 @@  typedef u64 kvm_pte_t;
  *			in the current context.
  * @virt_to_phys:	Convert a virtual address mapped in the current context
  *			into a physical address.
+ * @flush_dcache:	Clean data cache for a guest page address range before
+ *			creating the corresponding stage-2 mapping.
+ * @flush_icache:	Invalidate instruction cache for a guest page address
+ *			range before creating or updating the corresponding
+ *			stage-2 mapping.
  */
 struct kvm_pgtable_mm_ops {
 	void*		(*zalloc_page)(void *arg);
@@ -54,6 +59,8 @@  struct kvm_pgtable_mm_ops {
 	int		(*page_count)(void *addr);
 	void*		(*phys_to_virt)(phys_addr_t phys);
 	phys_addr_t	(*virt_to_phys)(void *addr);
+	void		(*flush_dcache)(void *addr, size_t size);
+	void		(*flush_icache)(void *addr, size_t size);
 };
 
 /**