Re: [Xen-devel] [PATCH 1/2] x86/shadow: compile most write emulation code just once

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.16 at 13:17, <tim@xxxxxxx> wrote:
> Hi,
> 
> At 03:13 -0700 on 10 Mar (1457579586), Jan Beulich wrote:
>> No need to compile all of this code three times, as most of it really
>> is guest mode independent. The savings are between 3k and 4k of binary
>> code in my builds.
> 
> Thanks for this.  This would have been _much_ easier to review as (at
> least) two patches, with one being motion-only.  As it is I had to
> effectively diff by eye to see the changes.

Well, a precisely code motion only patch wouldn't have worked -
some changes were unavoidable while moving, for the build to not
break.

>> No functional change (i.e. only formatting and naming changes)
> 
> There is one other change, that emulate_gva_to_mfn()'s call to
> gva_to_gfn() now indirects through the paging mode table.  I think
> that's fine, but it's not obvious from this description.

That's not a functional change (as it still results in the same function
getting called), but I've added a sentence to the earlier part of
description.

>> except
>> for
>> - sh_emulate_map_dest()'s user mode check corrected for the PV case
>>   (affecting debugging mode only, this isn't being split out)
>> - simplifying the vaddr argument to emulate_gva_to_mfn() for the second
>>   part in the cross page write case
> 
> Removing the mask with PAGE_MASK is fine, but please keep the '- 1' in
> calculating the final byte.  It is more clearly correct and can easily
> be folded with the earlier calculations.  If you want to make that
> change on committing, then
> 
> Reviewed-by: Tim Deegan <tim@xxxxxxx>

Thanks. It didn't seem questionable to me that the code would be
correct with the "- 1" dropped, but I've added it back.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel