guest PTE write emulation

guest PTE write emulation

[Dong, Eddie]

Not sure if we should use PT_USER_MASK | PT_WRITABLE_MASK here.

diff --git a/drivers/kvm/paging_tmpl.h b/drivers/kvm/paging_tmpl.h
index 6dd0da9..183d4ca 100644
--- a/drivers/kvm/paging_tmpl.h
+++ b/drivers/kvm/paging_tmpl.h
@@ -213,7 +213,7 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu,
struct
        if (~gpte & (PT_PRESENT_MASK | PT_ACCESSED_MASK))
                return;
        pgprintk("%s: gpte %llx spte %p\n", __FUNCTION__, (u64)gpte,
spte);
-       FNAME(set_pte)(vcpu, gpte, spte, 6,
+       FNAME(set_pte)(vcpu, gpte, spte, PT_USER_MASK |
PT_WRITABLE_MASK,
                       (gpte & PT_BASE_ADDR_MASK) >> PAGE_SHIFT);
 }


But then it seems we are not correctly emulating gpte ACCESSED bit
though it is likely harmless. Comments?

thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Avi Kivity]

Dong, Eddie wrote:
> Not sure if we should use PT_USER_MASK | PT_WRITABLE_MASK here.
>
> diff --git a/drivers/kvm/paging_tmpl.h b/drivers/kvm/paging_tmpl.h
> index 6dd0da9..183d4ca 100644
> --- a/drivers/kvm/paging_tmpl.h
> +++ b/drivers/kvm/paging_tmpl.h
> @@ -213,7 +213,7 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu,
> struct
>         if (~gpte & (PT_PRESENT_MASK | PT_ACCESSED_MASK))
>                 return;
>         pgprintk("%s: gpte %llx spte %p\n", __FUNCTION__, (u64)gpte,
> spte);
> -       FNAME(set_pte)(vcpu, gpte, spte, 6,
> +       FNAME(set_pte)(vcpu, gpte, spte, PT_USER_MASK |
> PT_WRITABLE_MASK,
>                        (gpte & PT_BASE_ADDR_MASK) >> PAGE_SHIFT);
>  }
>
>
> But then it seems we are not correctly emulating gpte ACCESSED bit
> though it is likely harmless. Comments?
>   

Earlier we check if the accessed bit is off, and if so, we don't set the 
shadow pte.  This won't happen in practice because the guest's page 
fault handler will set the accessed bit when it modifies a pte to avoid 
an RMW cycle by the hardware page table walker.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Dong, Eddie]

Avi Kivity wrote:
> Earlier we check if the accessed bit is off, and if so, we
> don't set the
> shadow pte.  This won't happen in practice because the guest's page
> fault handler will set the accessed bit when it modifies a pte
> to avoid
> an RMW cycle by the hardware page table walker.
> 
Thanks! That is good enough.
BTW, in kvm_mmu_pte_write, I feel a little bit stranger for following
code:


>                level = page->role.level;
>                npte = 1;
>                if (page->role.glevels == PT32_ROOT_LEVEL) {
>                        page_offset <<= 1;      /* 32->64 */          
> /* 
>                         * A 32-bit pde maps 4MB while the shadow pdes
> map 
>                         * only 2MB.  So we need to double the offset
> again 
>                         * and zap two pdes instead of one.           
>                        */ if (level == PT32_ROOT_LEVEL) {
>                                page_offset &= ~7; /* kill rounding
>                                error */ page_offset <<= 1;
>                                npte = 2;

1st of all, do we have 2MB pde in shadow PT? I think we don't provide
contiguous physical mfn for shadow PT.

If page->role.level == PT32_ROOT_LEVEL, sequencial call of
mmu_pte_write_new_pte will simply return w/o any real spte operation.
Does this ever happen w/ a real spte write?

I tried a 32 bits FC5 with no found of this combination till Linux boot.

>                        }
>                        quadrant = page_offset >> PAGE_SHIFT;
>                        page_offset &= ~PAGE_MASK;
>                        if (quadrant != page->role.quadrant)
>                                continue;
>                }
>                spte = &page->spt[page_offset / sizeof(*spte)];
>                while (npte--) {
>                        mmu_pte_write_zap_pte(vcpu, page, spte);
>                        mmu_pte_write_new_pte(vcpu, page,
> spte, new, bytes);

If above comments is correct, we need to provide an increased (gfn) gpte
so that the next 2MB spte can have right gfn from gpte. Am I missing
something?

>                        ++spte;
>                }


Thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Avi Kivity]

Dong, Eddie wrote:
> BTW, in kvm_mmu_pte_write, I feel a little bit stranger for following
> code:
>
>
>   
>>                level = page->role.level;
>>                npte = 1;
>>                if (page->role.glevels == PT32_ROOT_LEVEL) {
>>                        page_offset <<= 1;      /* 32->64 */          
>> /* 
>>                         * A 32-bit pde maps 4MB while the shadow pdes
>> map 
>>                         * only 2MB.  So we need to double the offset
>> again 
>>                         * and zap two pdes instead of one.           
>>                        */ if (level == PT32_ROOT_LEVEL) {
>>                                page_offset &= ~7; /* kill rounding
>>                                error */ page_offset <<= 1;
>>                                npte = 2;
>>     
>
> 1st of all, do we have 2MB pde in shadow PT? I think we don't provide
> contiguous physical mfn for shadow PT.
>   

No, we don't.  The intent is that a shadow pde maps 2MB through a shadow 
page table, not through a a large host page.

> If page->role.level == PT32_ROOT_LEVEL, sequencial call of
> mmu_pte_write_new_pte will simply return w/o any real spte operation.
> Does this ever happen w/ a real spte write?
>   

Yes.  This is a missed optimization, but I don't think that it impacts 
performance.  pde updates are very rare.

> I tried a 32 bits FC5 with no found of this combination till Linux boot.
>
>   

Usually pde updates are before the page directory is shadowed, so the 
host doesn't see it.

>>                        }
>>                        quadrant = page_offset >> PAGE_SHIFT;
>>                        page_offset &= ~PAGE_MASK;
>>                        if (quadrant != page->role.quadrant)
>>                                continue;
>>                }
>>                spte = &page->spt[page_offset / sizeof(*spte)];
>>                while (npte--) {
>>                        mmu_pte_write_zap_pte(vcpu, page, spte);
>>                        mmu_pte_write_new_pte(vcpu, page,
>> spte, new, bytes);
>>     
>
> If above comments is correct, we need to provide an increased (gfn) gpte
> so that the next 2MB spte can have right gfn from gpte. Am I missing
> something?

On a pte update, npte will always be 1.  On a pde update, we won't do 
anything in mmu_pte_write_new_pte because it doesn't handle pdes.  If we 
extend it to handle pdes, then we need either to modify the new gpde or 
to have the update take the quadrant into account.

I don't think there's a real need to extend it though.  A pde write will 
usually be followed by lots of pte writes.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

SMP support of MMU

[Dong, Eddie]

Avi:
	I noticed the current MMU data structure is defined as per VCPU
such as:

struct kvm_vcpu {
 	.....
        struct list_head free_pages;
        struct kvm_mmu_page page_header_buf[KVM_NUM_MMU_PAGES];
	.....
}

	This is a hot topics in Xen development time: whether should we
support global shadow PT or per VCPU shadow PT?  Per VCPU shadow is much
close to native from architecture point of view, but definitely much
complicated in implementation.  Do we really need to implement per VCPU
shadow? Xen today is still global shadow.
	If global shadow is good enough, then I will try to move
free_pages and page_header_buf[KVM_NUM_MMU_PAGES] to struct kvm.
Comments?

thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: SMP support of MMU

[Avi Kivity]

Dong, Eddie wrote:
> Avi:
> 	I noticed the current MMU data structure is defined as per VCPU
> such as:
>
> struct kvm_vcpu {
>  	.....
>         struct list_head free_pages;
>         struct kvm_mmu_page page_header_buf[KVM_NUM_MMU_PAGES];
> 	.....
> }
>
> 	This is a hot topics in Xen development time: whether should we
> support global shadow PT or per VCPU shadow PT?  Per VCPU shadow is much
> close to native from architecture point of view, but definitely much
> complicated in implementation.  Do we really need to implement per VCPU
> shadow? Xen today is still global shadow.
> 	If global shadow is good enough, then I will try to move
> free_pages and page_header_buf[KVM_NUM_MMU_PAGES] to struct kvm.
> Comments?
>   

I think a global shadow is actually better.  Consider a multithreaded
application where threads are free to migrate (e.g. many more threads
than vcpus); a per-vcpu shadow will cause many more faults than a global
shadow.

I have patch that converts page_header_buf into a slab cache; that
should remove the per-vcpu fields.  I just have to debug it before I can
apply it.  I'll dig it up.

(the reason they are actually in kvm_vcpu is that we started with a
non-caching mmu implementation, where you can't share pages anyway)

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: SMP support of MMU

[Dong, Eddie]

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Avi:
>> 	I noticed the current MMU data structure is defined as per VCPU
>> such as: 
>> 
>> struct kvm_vcpu {
>>  	.....
>>         struct list_head free_pages;
>>         struct kvm_mmu_page page_header_buf[KVM_NUM_MMU_PAGES];
>> ..... }
>> 
>> 	This is a hot topics in Xen development time: whether should we
>> support global shadow PT or per VCPU shadow PT?  Per VCPU shadow is
>> much close to native from architecture point of view, but definitely
>> much complicated in implementation.  Do we really need to implement
>> per VCPU shadow? Xen today is still global shadow.
>> 	If global shadow is good enough, then I will try to move
>> free_pages and page_header_buf[KVM_NUM_MMU_PAGES] to struct kvm.
>> Comments? 
>> 
> 
> I think a global shadow is actually better.  Consider a multithreaded
> application where threads are free to migrate (e.g. many more threads
> than vcpus); a per-vcpu shadow will cause many more faults
> than a global
> shadow.

Yep, we are on same page :-)

> 
> I have patch that converts page_header_buf into a slab cache; that
> should remove the per-vcpu fields.  I just have to debug it
> before I can
> apply it.  I'll dig it up.

Great! then the shadow is almost SMP support free with big lock at
beginning.

> 
> (the reason they are actually in kvm_vcpu is that we started with a
> non-caching mmu implementation, where you can't share pages anyway)
> 

Eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Dong, Eddie]

Avi Kivity wrote:
> On a pte update, npte will always be 1.  On a pde update, we won't do
> anything in mmu_pte_write_new_pte because it doesn't handle
> pdes.  If we
> extend it to handle pdes, then we need either to modify the
> new gpde or
> to have the update take the quadrant into account.

Agree, so we either just skip npte=2 branch or polish it.
How about following changes?  

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index c85c664..37a7dc5 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -1156,7 +1156,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
 	struct hlist_node *node, *n;
 	struct hlist_head *bucket;
 	unsigned index;
-	u64 *spte;
+	u64 *spte, gpte[2];
 	unsigned offset = offset_in_page(gpa);
 	unsigned pte_size;
 	unsigned page_offset;
@@ -1164,7 +1164,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
 	unsigned quadrant;
 	int level;
 	int flooded = 0;
-	int npte;
+	int npte, i;
 
 	pgprintk("%s: gpa %llx bytes %d\n", __FUNCTION__, gpa, bytes);
 	if (gfn == vcpu->last_pt_write_gfn) {
@@ -1202,6 +1202,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
 		page_offset = offset;
 		level = page->role.level;
 		npte = 1;
+		gpte[0] = *(long*)new;
 		if (page->role.glevels == PT32_ROOT_LEVEL) {
 			page_offset <<= 1;	/* 32->64 */
 			/*
@@ -1209,10 +1210,16 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
 			 * only 2MB.  So we need to double the offset
again
 			 * and zap two pdes instead of one.
 			 */
-			if (level == PT32_ROOT_LEVEL) {
+			if ((level == PT_DIRECTORY_LEVEL) &&
+				(vcpu->cr4 & CR4_PSE_MASK) &&
+				(gpte[0] & PT_PAGE_SIZE_MASK)) {
 				page_offset &= ~7; /* kill rounding
error */
 				page_offset <<= 1;
 				npte = 2;
+				gpte[1] = gpte[0];
+				gpte[1] += 1 << 21;
 			}
 			quadrant = page_offset >> PAGE_SHIFT;
 			page_offset &= ~PAGE_MASK;
@@ -1220,9 +1227,9 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
 				continue;
 		}
 		spte = &page->spt[page_offset / sizeof(*spte)];
-		while (npte--) {
+		for (i=0; i < npte; i++) {
 			mmu_pte_write_zap_pte(vcpu, page, spte);
-			mmu_pte_write_new_pte(vcpu, page, spte, new,
bytes);
+			mmu_pte_write_new_pte(vcpu, page, spte,
&gpte[i], bytes);
 			++spte;
 		}
 	}

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Avi Kivity]

Dong, Eddie wrote:
> Avi Kivity wrote:
>   
>> On a pte update, npte will always be 1.  On a pde update, we won't do
>> anything in mmu_pte_write_new_pte because it doesn't handle
>> pdes.  If we
>> extend it to handle pdes, then we need either to modify the
>> new gpde or
>> to have the update take the quadrant into account.
>>     
>
> Agree, so we either just skip npte=2 branch or polish it.
> How about following changes?  
>   

Isn't it all dead code, as gpte[1] will never be used?

Maybe just move the page->role.level test from mmu_pte_write_new_pte() 
back to kvm_mmu_pte_write(), so that it's clear that the new data isn't 
used for the non-pte case?


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: guest PTE write emulation

[Dong, Eddie]

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Avi Kivity wrote:
>> 
>>> On a pte update, npte will always be 1.  On a pde update, we won't
>>> do anything in mmu_pte_write_new_pte because it doesn't handle
>>> pdes.  If we extend it to handle pdes, then we need either to
>>> modify the new gpde or to have the update take the quadrant into
>>> account. 
>>> 
>> 
>> Agree, so we either just skip npte=2 branch or polish it. How about
>> following changes? 
>> 
> 
> Isn't it all dead code, as gpte[1] will never be used?

If removing dead code, we can do like following:

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index c85c664..7d7cd42 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -1209,11 +1209,6 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
gpa_t gpa,
                         * only 2MB.  So we need to double the offset
again
                         * and zap two pdes instead of one.
                         */
-                       if (level == PT32_ROOT_LEVEL) {
-                               page_offset &= ~7; /* kill rounding
error */
-                               page_offset <<= 1;
-                               npte = 2;
-                       }
                        quadrant = page_offset >> PAGE_SHIFT;
                        page_offset &= ~PAGE_MASK;
                        if (quadrant != page->role.quadrant)


> 
> Maybe just move the page->role.level test from mmu_pte_write_new_pte()

role test need to combine with guest CR4 and PS bits.
Current code find about 3.5K times of fake 4M page.

> back to kvm_mmu_pte_write(), so that it's clear that the new
> data isn't
> used for the non-pte case?

Agree.

Eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/