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

[Keir Fraser <keir.fraser@xxxxxxxxxxxxx>]

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@xxxxxxxxx> wrote:

> Keir,
> Thanks for your comments. Attached is the updated version.
> 
> Thanks
> Xiaohui
> 
>> -----Original Message-----
>> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>> [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
>> Sent: 2009年1月22日 21:14
>> To: Xin, Xiaohui; xen-devel@xxxxxxxxxxxxxxxxxxx
>> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel



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