Xen devel - Feb 2008 - [PATCH] Simplify paging_invlpg when flush is not required.

Simplify paging_invlpg when flush is not required.

New ''flush'' parameter is added to paging_invlpg, to allow
caller assigning whether flush check is required. It''s
wasteful to always validate shadow linear mapping if caller
doesn''t check return value at all.

Signed-off-by Kevin Tian <kevin.tian@intel.com>

Thanks,
Kevin


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

Is there a significant advantage to doing this? One other comment is that I
don''t like extra boolean 0/1 arguments to functions. I''d
rather have
something like paging_invlpg() and paging_invlpg_noflush() and only have the
boolean argument to paging.mode->invlpg().

 -- Keir

On 3/2/08 05:59, "Tian, Kevin" <kevin.tian@intel.com> wrote:
> Simplify paging_invlpg when flush is not required.
> 
> New ''flush'' parameter is added to paging_invlpg, to allow
> caller assigning whether flush check is required. It''s
> wasteful to always validate shadow linear mapping if caller
> doesn''t check return value at all.
> 
> Signed-off-by Kevin Tian <kevin.tian@intel.com>
> 
> Thanks,
> Kevin
> _______________________________________________
> 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

>From: Keir Fraser
>Sent: 2008年2月3日 16:56
>
>Is there a significant advantage to doing this? One other 
>comment is that I
>don''t like extra boolean 0/1 arguments to functions. I''d
rather have
>something like paging_invlpg() and paging_invlpg_noflush() and 
>only have the
>boolean argument to paging.mode->invlpg().
>
> -- Keir
No significant advantage, and just eye-balled this redundant work.
Here''s updated version. Only check compilation this time.

----

Simplify paging_invlpg when flush is not required.

Add paging_invlpg_noflush, when flush check is not required.
It''s wasteful to always validate shadow linear mapping if
caller doesn''t check return value at all.

Signed-off-by Kevin Tian <kevin.tian@intel.com>

Thanks,
Kevin



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

I''ll hold it pending ack/nack from Tim.

 -- Keir

On 3/2/08 09:08, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>> From: Keir Fraser
>> Sent: 2008年2月3日 16:56
>> 
>> Is there a significant advantage to doing this? One other
>> comment is that I
>> don''t like extra boolean 0/1 arguments to functions.
I''d rather have
>> something like paging_invlpg() and paging_invlpg_noflush() and
>> only have the
>> boolean argument to paging.mode->invlpg().
>> 
>> -- Keir
> 
> No significant advantage, and just eye-balled this redundant work.
> Here''s updated version. Only check compilation this time.
> 
> ----
> 
> Simplify paging_invlpg when flush is not required.
> 
> Add paging_invlpg_noflush, when flush check is not required.
> It''s wasteful to always validate shadow linear mapping if
> caller doesn''t check return value at all.
> 
> Signed-off-by Kevin Tian <kevin.tian@intel.com>
> 
> Thanks,
> Kevin
> 


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

At 08:55 +0000 on 03 Feb (1202028939), Keir Fraser
wrote:
> Is there a significant advantage to doing this? One other comment is that I
> don''t like extra boolean 0/1 arguments to functions. I''d
rather have
> something like paging_invlpg() and paging_invlpg_noflush() and only have
the
> boolean argument to paging.mode->invlpg().
As someone (probably Kevin) pointed out before, the entire guest-walk
in sh_invlpg is redundant because we maintain TLB flush discipline on
shadow l2es ourselves.  Cleaning that up was on the list of things to do
when the out-of-sync shadows are done.  If we''re going to change this
code now, the right thing to do is to kill off sh_invlpg entirely, except
for the call to vtlb_flush().  Then there''s no need for it to return a
value at all. 

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

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

>From: Tim Deegan
>Sent: 2008年2月4日 19:05
>
>At 08:55 +0000 on 03 Feb (1202028939), Keir Fraser wrote:
>> Is there a significant advantage to doing this? One other 
>comment is that I
>> don''t like extra boolean 0/1 arguments to functions.
I''d rather have
>> something like paging_invlpg() and paging_invlpg_noflush() 
>and only have the
>> boolean argument to paging.mode->invlpg().
>
>As someone (probably Kevin) pointed out before, the entire guest-walk
>in sh_invlpg is redundant because we maintain TLB flush discipline on
>shadow l2es ourselves.  Cleaning that up was on the list of 
>things to do
>when the out-of-sync shadows are done.  If we''re going to change
this
>code now, the right thing to do is to kill off sh_invlpg 
>entirely, except
>for the call to vtlb_flush().  Then there''s no need for it to
return a
>value at all. 
>
Well, I guess it should be Xin who since educated me about such
redundancy before. :-) But I''m inclined to keep this interface for
several
reasons:

* Shadow currently requires to intercept invlpg as what you mentioned
about vtlb and also future fast_emulation I posted before. Call those
vtlb-like invalidation inside sh_invlpg is cleaner

* I''m not sure how future out-of-sync logic may be implemented, but I
guess it may be a dynamic hook, and only active at some special 
points. If that''s the case, sometimes TLB flush may be required and
others may not

But anyway, this is not important. We can also wait for out-of-sync
feature ready and then see how this interface may be affected.

Thanks,
Kevin

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