Patchwork [7/8] KVM: Add page map arch callback

login
register
mail settings
Submitter Alexander Graf
Date Aug. 7, 2012, 10:57 a.m.
Message ID <1344337036-22244-8-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/175588/
State New
Headers show

Comments

Alexander Graf - Aug. 7, 2012, 10:57 a.m.
Some archs need to ensure that their icache is flushed when mapping a new
page. Add a callback to the generic code for an arch to implement any cache
flush logic it may need.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 virt/kvm/kvm_main.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Avi Kivity - Aug. 7, 2012, 1:32 p.m.
On 08/07/2012 01:57 PM, Alexander Graf wrote:
> Some archs need to ensure that their icache is flushed when mapping a new
> page. Add a callback to the generic code for an arch to implement any cache
> flush logic it may need.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  virt/kvm/kvm_main.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d42591d..4e0882d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1161,8 +1161,12 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
>  			pfn = get_fault_pfn();
>  		}
>  		up_read(&current->mm->mmap_sem);
> -	} else
> +	} else {
>  		pfn = page_to_pfn(page[0]);
> +#ifdef __KVM_HAVE_ARCH_MAP_PAGE
> +		kvm_arch_map_page(page[0]);
> +#endif
> +	}
>  

Please call it unconditionally, and have a stub inline ifndef
__KVM_HAVE_ARCH_MAP_PAGE.

Is this the correct place?  Who says the caller of hva_to_pfn() is going
to map it?
Alexander Graf - Aug. 7, 2012, 1:44 p.m.
On 07.08.2012, at 15:32, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 01:57 PM, Alexander Graf wrote:
>> Some archs need to ensure that their icache is flushed when mapping a new
>> page. Add a callback to the generic code for an arch to implement any cache
>> flush logic it may need.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> virt/kvm/kvm_main.c |    6 +++++-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>> 
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index d42591d..4e0882d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1161,8 +1161,12 @@ static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
>>            pfn = get_fault_pfn();
>>        }
>>        up_read(&current->mm->mmap_sem);
>> -    } else
>> +    } else {
>>        pfn = page_to_pfn(page[0]);
>> +#ifdef __KVM_HAVE_ARCH_MAP_PAGE
>> +        kvm_arch_map_page(page[0]);
>> +#endif
>> +    }
>> 
> 
> Please call it unconditionally, and have a stub inline ifndef
> __KVM_HAVE_ARCH_MAP_PAGE.

Sure, works fine with me.

> 
> Is this the correct place?  Who says the caller of hva_to_pfn() is going
> to map it?

I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.

Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Aug. 7, 2012, 1:58 p.m.
On 08/07/2012 04:44 PM, Alexander Graf wrote:
> 
>> 
>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>> to map it?
> 
> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
> 
> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.

Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
one place.
Alexander Graf - Aug. 7, 2012, 2:08 p.m.
On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>> 
>>> 
>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>> to map it?
>> 
>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>> 
>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
> 
> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
> one place.

Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.

But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Aug. 7, 2012, 2:10 p.m.
On 08/07/2012 05:08 PM, Alexander Graf wrote:
> 
> 
> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>> 
>>>> 
>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>> to map it?
>>> 
>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>> 
>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>> 
>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>> one place.
> 
> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
> 
> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.

I'm not sure.  We have lots of functions of this sort, and their number
keeps increasing.  Maybe a better place is pre-map.
Alexander Graf - Aug. 7, 2012, 2:14 p.m.
On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>> 
>> 
>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>> 
>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>> 
>>>>> 
>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>> to map it?
>>>> 
>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>> 
>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>> 
>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>> one place.
>> 
>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>> 
>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
> 
> I'm not sure.  We have lots of functions of this sort, and their number
> keeps increasing.  Maybe a better place is pre-map.

Pre-map? How?

Alex

> 
> 
> -- 
> error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Aug. 7, 2012, 2:20 p.m.
On 08/07/2012 05:14 PM, Alexander Graf wrote:
> 
> 
> On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:
> 
>> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>>> 
>>> 
>>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>>> 
>>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>>> 
>>>>>> 
>>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>>> to map it?
>>>>> 
>>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>>> 
>>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>>> 
>>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>>> one place.
>>> 
>>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>>> 
>>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
>> 
>> I'm not sure.  We have lots of functions of this sort, and their number
>> keeps increasing.  Maybe a better place is pre-map.
> 
> Pre-map? How?

In arch code before you install the page in a pte/tlbe.

We don't have a single point we can hook unfortunately.
Alexander Graf - Aug. 7, 2012, 2:24 p.m.
On 07.08.2012, at 16:20, Avi Kivity <avi@redhat.com> wrote:

> On 08/07/2012 05:14 PM, Alexander Graf wrote:
>> 
>> 
>> On 07.08.2012, at 16:10, Avi Kivity <avi@redhat.com> wrote:
>> 
>>> On 08/07/2012 05:08 PM, Alexander Graf wrote:
>>>> 
>>>> 
>>>> On 07.08.2012, at 15:58, Avi Kivity <avi@redhat.com> wrote:
>>>> 
>>>>> On 08/07/2012 04:44 PM, Alexander Graf wrote:
>>>>>> 
>>>>>>> 
>>>>>>> Is this the correct place?  Who says the caller of hva_to_pfn() is going
>>>>>>> to map it?
>>>>>> 
>>>>>> I don't think anyone is. However, we need the struct page, and all the generic kvm mm code tries hard to hide it from its users. The alternative would be to expose all those details, and I'm not sure that's a good idea.
>>>>>> 
>>>>>> Essentially, we don't care if we're overly cautious. Clearing one page too much is way better than clearing one too few.
>>>>> 
>>>>> Are you sure everyone uses hva_to_pfn()?  x86 uses gfn_to_hva_many(), in
>>>>> one place.
>>>> 
>>>> Nope, I only checked that e500 adheres to that flow so far. I'm not even 100% sure that book3s is always happy yet.
>>>> 
>>>> But I figured this is a step in the right direction. If we missed out on one, we can always add it later. The many function is a good spot. Maybe I'll just ckeck up all of kvm_main.c again for potential users.
>>> 
>>> I'm not sure.  We have lots of functions of this sort, and their number
>>> keeps increasing.  Maybe a better place is pre-map.
>> 
>> Pre-map? How?
> 
> In arch code before you install the page in a pte/tlbe.

So how do I get to the struct page in there?

Alex

> 
> We don't have a single point we can hook unfortunately.
> 
> 
> -- 
> error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Aug. 7, 2012, 2:31 p.m.
On 08/07/2012 05:24 PM, Alexander Graf wrote:
>>> 
>>> Pre-map? How?
>> 
>> In arch code before you install the page in a pte/tlbe.
> 
> So how do I get to the struct page in there?
> 

pfn_to_page()

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d42591d..4e0882d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1161,8 +1161,12 @@  static pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async,
 			pfn = get_fault_pfn();
 		}
 		up_read(&current->mm->mmap_sem);
-	} else
+	} else {
 		pfn = page_to_pfn(page[0]);
+#ifdef __KVM_HAVE_ARCH_MAP_PAGE
+		kvm_arch_map_page(page[0]);
+#endif
+	}
 
 	return pfn;
 }