Re: [Xen-devel] [PATCH 1/4] x86: remove CR reads from exit-to-guest path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/01/18 11:10, Jan Beulich wrote:
>>>> On 30.01.18 at 12:01, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 23/01/18 10:36, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/asm_defns.h
>>> +++ b/xen/include/asm-x86/asm_defns.h
>>> @@ -206,13 +206,12 @@ void ret_from_intr(void);
>>>  #define ASM_STAC ASM_AC(STAC)
>>>  #define ASM_CLAC ASM_AC(CLAC)
>>>  
>>> -.macro write_cr3 val:req, tmp1:req, tmp2:req
>>> -        mov   %cr4, %\tmp1
>>> -        mov   %\tmp1, %\tmp2
>>> -        and   $~X86_CR4_PGE, %\tmp1
>>> -        mov   %\tmp1, %cr4
>>> +.macro write_cr3 val:req, cr4:req, tmp:req
>>> +        mov   %\cr4, %\tmp
>>> +        and   $~X86_CR4_PGE, %\cr4
>>> +        mov   %\cr4, %cr4
>> This is confusing to read; It took me a while to work out why it
>> assembled in the first place.  Given that there are only two instances
>> of write_cr3 now, I'd suggest expanding this in the two sites.  It will
>> also make it clear which registers have real values and which are
>> temporary, which isn't clear from the current callsites.
> Hmm, part of the reason I didn't want to drop the macro altogether
> is its similarity with write_cr3(), so it would turn up in grep-s for that
> one. How about switching the two use sites to specify named
> arguments to the macro?

Frankly, neither uses are suitably named.  They are "switch to new cr3
and flush global pages", except that the asm version doesn't tick the
TLB clock.

Using explicitly named parameters would be an improvement over what is
here at the moment, but removing the macro would be an improvement still.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel