[PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Liliang]

From: Li Liang <liangx.z.li@intel.com>

The commit aa9114edd added the needless function call memory_type_changed,
it is unnessary because the PAT content does not influence the result of
epte_get_entry_emt(). If it is called, the cache will be flushed excessively
and make the nested guest very slowly, just like blocked.

Signed-off-by: Li Liang <liangx.z.li@intel.com>
---
 xen/arch/x86/hvm/hvm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d40c48e..e8f41c1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -257,8 +257,6 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
     if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
         v->arch.hvm_vcpu.pat_cr = guest_pat;
 
-    memory_type_changed(v->domain);
-
     return 1;
 }
 
-- 
1.9.1

[PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Liliang]

From: Li Liang <liangx.z.li@intel.com>

The function memory_type_changed should not be called in hvm_set_guest_pat.
or the cache will be flushed excessively and make the nested guest very slowly,
just like blocked.

Signed-off-by: Li Liang <liangx.z.li@intel.com>
---
 xen/arch/x86/hvm/hvm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d40c48e..1a0534b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -257,7 +257,6 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
     if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
         v->arch.hvm_vcpu.pat_cr = guest_pat;
 
-    memory_type_changed(v->domain);
 
     return 1;
 }
-- 
1.9.1

Re: [PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Andrew Cooper]

On 18/08/14 12:11, Liliang wrote:
> From: Li Liang <liangx.z.li@intel.com>
>
> The function memory_type_changed should not be called in hvm_set_guest_pat.
> or the cache will be flushed excessively and make the nested guest very slowly,
> just like blocked.
>
> Signed-off-by: Li Liang <liangx.z.li@intel.com>

Whyso?  Without it, the caching types are likely wrong, especially after
setting pat.

~Andrew

> ---
>  xen/arch/x86/hvm/hvm.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d40c48e..1a0534b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -257,7 +257,6 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
>      if ( !hvm_funcs.set_guest_pat(v, guest_pat) )
>          v->arch.hvm_vcpu.pat_cr = guest_pat;
>  
> -    memory_type_changed(v->domain);
>  
>      return 1;
>  }

Re: [PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Jan Beulich]

>>> Liliang <liangx.z.li@intel.com> 08/18/14 1:15 PM >>>
>From: Li Liang <liangx.z.li@intel.com>
>
>The function memory_type_changed should not be called in hvm_set_guest_pat.
>or the cache will be flushed excessively and make the nested guest very slowly,
>just like blocked.

As Andrew already said - blindly removing the call is wrong, it is there for a
reason. What you first of all need to explain is why there are so many PAT
modifications, as it's perhaps _that_ which needs adjustment (possibly e.g.
including some filtering to hide writes that don't change the actual value, if
that happens frequently).

Jan

Re: [PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Jan Beulich]

>>>> "Jan Beulich" <jbeulich@suse.com> 08/18/14 3:10 PM >>>
>>>> Liliang <liangx.z.li@intel.com> 08/18/14 1:15 PM >>>
>>From: Li Liang <liangx.z.li@intel.com>
>>
>>The function memory_type_changed should not be called in hvm_set_guest_pat.
>>or the cache will be flushed excessively and make the nested guest very slowly,
>>just like blocked.
>
>As Andrew already said - blindly removing the call is wrong, it is there for a
>reason. What you first of all need to explain is why there are so many PAT
>modifications, as it's perhaps _that_ which needs adjustment (possibly e.g.
>including some filtering to hide writes that don't change the actual value, if
>that happens frequently).

Actually I have to correct myself - I think the call here is indeed not needed,
as the PAT contents is being used by the CPU itself when determining the
memory type (and does not influence the result of epte_get_entry_emt()). But
the patch description needs to better describe the situation, preferably also
referencing the commit that needlessly added the call.

Jan

Re: [PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Olaf Hering]

On Mon, Aug 18, Liliang wrote:

> From: Li Liang <liangx.z.li@intel.com>
> 
> The function memory_type_changed should not be called in hvm_set_guest_pat.
> or the cache will be flushed excessively and make the nested guest very slowly,
> just like blocked.

The patch subject is wrongly worded, it will cause bogus 'git log --oneline' output. 
I'm sure there are several bugs which cause nested guests to fail.

Olaf

Re: [PATCH] x86/HVM: Fixed the bug which cause the nested hvm guest fail to boot

[Jan Beulich]

>>> On 25.08.14 at 10:08, <olaf@aepfle.de> wrote:
> On Mon, Aug 18, Liliang wrote:
> 
>> From: Li Liang <liangx.z.li@intel.com>
>> 
>> The function memory_type_changed should not be called in hvm_set_guest_pat.
>> or the cache will be flushed excessively and make the nested guest very 
> slowly,
>> just like blocked.
> 
> The patch subject is wrongly worded, it will cause bogus 'git log --oneline' 
> output. 
> I'm sure there are several bugs which cause nested guests to fail.

And as you may have seen I changed the title (for this very
reason) before applying.

Jan