Xen devel - Jan 2009 - [PATCH 2/2] Enhance MTRR/PAT virtualization for EPT & VT-d enabled both

The patch attached is to set effective memory type for EPT according to the VT-d
snoop control capability, and also includes some cleansup for EPT & VT-d
both enabled.

Signed-off-by: Zhai, Edwin
Edwin.Zhai@intel.com<mailto:Edwin.Zhai@intel.com>

Signed-off-by: Xin, Xiaohui
xiaohui.xin@intel.com<mailto:xiaohui.xin@intel.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

You add a new flag ''is_untaint''. What is tainted/untainted? No
comment and
not a descriptive field name.

 -- Keir

On 22/01/2009 09:48, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> The patch attached is to set effective memory type for EPT according to the
> VT-d snoop control capability, and also includes some cleansup for EPT
& VT-d
> both enabled.
>  
> Signed-off-by: Zhai, Edwin Edwin.Zhai@intel.com
>  
> Signed-off-by: Xin, Xiaohui xiaohui.xin@intel.com
>  
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Looking further, it appears to be an ugly way of getting a parameter to an
p2m-ept.c-internal function. I don''t like that. If you need a
parameter,
define it and pass it down properly.

 -- Keir

On 22/01/2009 10:15, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
> You add a new flag ''is_untaint''. What is
tainted/untainted? No comment and
> not a descriptive field name.
> 
>  -- Keir
> 
> On 22/01/2009 09:48, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> 
>> The patch attached is to set effective memory type for EPT according to
the
>> VT-d snoop control capability, and also includes some cleansup for EPT
& VT-d
>> both enabled.
>>  
>> Signed-off-by: Zhai, Edwin Edwin.Zhai@intel.com
>>  
>> Signed-off-by: Xin, Xiaohui xiaohui.xin@intel.com
>>  
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The new flag is used to not to reconstruct vt-d page tables when guest tried to
set MTRR range. At that time, we called hvm_set_uc_mode() to refill the emt fied
of EPT entries. But we don't need to reconstruct the vt-d page tables. It
costs too much especially when guest memory is large. Basically, the vt-d page
table is constructed when p2m tables is created which during the domain creation
time, so we may skip the step during hvm_set_uc_mode().

Thanks
Xiaohui
>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: 2009年1月22日 18:16
>To: Xin, Xiaohui; xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel][PATCH 2/2] Enhance MTRR/PAT virtualization for EPT
&
>VT-d enabled both
>
>You add a new flag 'is_untaint'. What is tainted/untainted? No
comment and
>not a descriptive field name.
>
> -- Keir
>
>On 22/01/2009 09:48, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
>
>> The patch attached is to set effective memory type for EPT according to
the
>> VT-d snoop control capability, and also includes some cleansup for EPT
& VT-d
>> both enabled.
>>
>> Signed-off-by: Zhai, Edwin Edwin.Zhai@intel.com
>>
>> Signed-off-by: Xin, Xiaohui xiaohui.xin@intel.com
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I knew that. But since at last we should add the parameter in ept_set_entry()
which will then taint p2m_set_entry() to add an unused parameter, and the
parameter is meaningless at all in shadow mode. And the flag is used in the same
way as the flag is_in_uc_mode in hvm_set_uc_mode().
Do you like to add a parameter in set-entry() and then ept_set_entry() and
p2m_set_entry()?

Thanks
Xiaohui
>-----Original Message-----
>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: 2009年1月22日 19:43
>To: Xin, Xiaohui; xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel][PATCH 2/2] Enhance MTRR/PAT virtualization for EPT
&
>VT-d enabled both
>
>Looking further, it appears to be an ugly way of getting a parameter to an
>p2m-ept.c-internal function. I don't like that. If you need a parameter,
>define it and pass it down properly.
>
> -- Keir
>
>On 22/01/2009 10:15, "Keir Fraser"
<keir.fraser@eu.citrix.com> wrote:
>
>> You add a new flag 'is_untaint'. What is tainted/untainted? No
comment and
>> not a descriptive field name.
>>
>>  -- Keir
>>
>> On 22/01/2009 09:48, "Xin, Xiaohui"
<xiaohui.xin@intel.com> wrote:
>>
>>> The patch attached is to set effective memory type for EPT
according to the
>>> VT-d snoop control capability, and also includes some cleansup for
EPT & VT-d
>>> both enabled.
>>>
>>> Signed-off-by: Zhai, Edwin Edwin.Zhai@intel.com
>>>
>>> Signed-off-by: Xin, Xiaohui xiaohui.xin@intel.com
>>>
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 22/01/2009 12:37, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> I knew that. But since at last we should add the parameter in
ept_set_entry()
> which will then taint p2m_set_entry() to add an unused parameter, and the
> parameter is meaningless at all in shadow mode. And the flag is used in the
> same way as the flag is_in_uc_mode in hvm_set_uc_mode().
That''s not true, since is_in_uc_mode is a state which persists beyond
any
single function''s scope. It is a reflection of a real aspect of guest
state.
> Do you like to add a parameter in set-entry() and then ept_set_entry() and
> p2m_set_entry()?
This is not necessary. Rename ept_set_entry() to something else (e.g.,
__ept_set_entry() if you can''t think of anything better), taking this
new
parameter. Then create a new ept_set_entry(), calling your renamed original
function, passing a default value for the new parameter.

This can work because the vmx_set_uc_mode() calls ept_* functions directly,
and hence having this parameter configurable via the generic p2m interface
is not necessary (as far as I can see). Only callers of ept_set_entry()
which need to specify the new parameter need to call the renamed function,
and I think all such callers are in vmx/ept code.

And please add a comment to give some idea of what this new parameter
actually means and represents (something higher level than it being an
optimisation hack :-).

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir,
Thanks for your comments. Attached is the updated version.

Thanks
Xiaohui
>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: 2009年1月22日 21:14
>To: Xin, Xiaohui; xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel][PATCH 2/2] Enhance MTRR/PAT virtualization for EPT
&
>VT-d enabled both
>
>On 22/01/2009 12:37, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
>
>> I knew that. But since at last we should add the parameter in
ept_set_entry()
>> which will then taint p2m_set_entry() to add an unused parameter, and
the
>> parameter is meaningless at all in shadow mode. And the flag is used in
the
>> same way as the flag is_in_uc_mode in hvm_set_uc_mode().
>
>That's not true, since is_in_uc_mode is a state which persists beyond
any
>single function's scope. It is a reflection of a real aspect of guest
state.
>
>> Do you like to add a parameter in set-entry() and then ept_set_entry()
and
>> p2m_set_entry()?
>
>This is not necessary. Rename ept_set_entry() to something else (e.g.,
>__ept_set_entry() if you can't think of anything better), taking this
new
>parameter. Then create a new ept_set_entry(), calling your renamed original
>function, passing a default value for the new parameter.
>
>This can work because the vmx_set_uc_mode() calls ept_* functions directly,
>and hence having this parameter configurable via the generic p2m interface
>is not necessary (as far as I can see). Only callers of ept_set_entry()
>which need to specify the new parameter need to call the renamed function,
>and I think all such callers are in vmx/ept code.
>
>And please add a comment to give some idea of what this new parameter
>actually means and represents (something higher level than it being an
>optimisation hack :-).
>
> -- Keir
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Yeah, this is better, but now I understand this patch a bit more, I wonder
why you need a parameter at all?

Would it not be quite easy for ept_set_entry() to determine whether any
gfn->mfn mapping has changed, since it can see the old EPTE as it modifies
it? In which case why not have that function determine whether it needs to
modify VT-d tables (i.e, default to 0 at top of function, and flip to 1 as
soon as you see a gfn->mfn translation change)?

Now, I could accept your current patch if I''m missing something,
because I
prefer this new interface to the way you did it before, but perhaps you can
hide this new flag entirely inside ept_set_entry() and avoid any extra
interface complexity at all?

What do you think?

 -- Keir

On 23/01/2009 02:41, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> Keir,
> Thanks for your comments. Attached is the updated version.
> 
> Thanks
> Xiaohui
> 
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>> Sent: 2009年1月22日 21:14
>> To: Xin, Xiaohui; xen-devel@lists.xensource.com
>> Subject: Re: [Xen-devel][PATCH 2/2] Enhance MTRR/PAT virtualization for
EPT &
>> VT-d enabled both
>> 
>> On 22/01/2009 12:37, "Xin, Xiaohui"
<xiaohui.xin@intel.com> wrote:
>> 
>>> I knew that. But since at last we should add the parameter in
>>> ept_set_entry()
>>> which will then taint p2m_set_entry() to add an unused parameter,
and the
>>> parameter is meaningless at all in shadow mode. And the flag is
used in the
>>> same way as the flag is_in_uc_mode in hvm_set_uc_mode().
>> 
>> That''s not true, since is_in_uc_mode is a state which persists
beyond any
>> single function''s scope. It is a reflection of a real aspect
of guest state.
>> 
>>> Do you like to add a parameter in set-entry() and then
ept_set_entry() and
>>> p2m_set_entry()?
>> 
>> This is not necessary. Rename ept_set_entry() to something else (e.g.,
>> __ept_set_entry() if you can''t think of anything better),
taking this new
>> parameter. Then create a new ept_set_entry(), calling your renamed
original
>> function, passing a default value for the new parameter.
>> 
>> This can work because the vmx_set_uc_mode() calls ept_* functions
directly,
>> and hence having this parameter configurable via the generic p2m
interface
>> is not necessary (as far as I can see). Only callers of ept_set_entry()
>> which need to specify the new parameter need to call the renamed
function,
>> and I think all such callers are in vmx/ept code.
>> 
>> And please add a comment to give some idea of what this new parameter
>> actually means and represents (something higher level than it being an
>> optimisation hack :-).
>> 
>> -- Keir
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 23/01/2009 12:03, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
> Would it not be quite easy for ept_set_entry() to determine whether any
> gfn->mfn mapping has changed, since it can see the old EPTE as it
modifies
> it? In which case why not have that function determine whether it needs to
> modify VT-d tables (i.e, default to 0 at top of function, and flip to 1 as
> soon as you see a gfn->mfn translation change)?
> 
> Now, I could accept your current patch if I''m missing something,
because I
> prefer this new interface to the way you did it before, but perhaps you can
> hide this new flag entirely inside ept_set_entry() and avoid any extra
> interface complexity at all?
Or indeed would it be simpler to have ept_change_entry_emt_with_range()
contain the bits of ept_set_entry() it needs, rather than calling it?
I''m
not sure about that -- I suppose it depends on how much of ept_set_entry()
would disappear if you basically inlined it. It might not make sense to take
this approach.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel