Re: [PATCH 4/9] x86/HVM: move NOFLUSH handling out of hvm_set_cr3()

From: "Jan Beulich" <JBeulich@suse.com>
To: Paul Durrant <paul.durrant@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Wei Liu <wei.liu2@citrix.com>,
	george.dunlap@citrix.com, Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 4/9] x86/HVM: move NOFLUSH handling out of hvm_set_cr3()
Date: Thu, 02 May 2019 07:23:00 -0600	[thread overview]
Message-ID: <5CCAEF34020000780022B400@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <42621a15169c4a409efa29e3d2d7e06e@AMSPEX02CL03.citrite.net>

>>> On 02.05.19 at 15:07, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 02 May 2019 13:20
>> 
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -2072,6 +2072,8 @@ static int hvmemul_write_cr(
>>      HVMTRACE_LONG_2D(CR_WRITE, reg, TRC_PAR_LONG(val));
>>      switch ( reg )
>>      {
>> +        bool noflush;
>> +
> 
> Why introduce 'noflush' with this scope when it could be limited to 'case 
> 3:', although...

Because this would entail introducing another set of braces, and
I pretty much dislike these case-block braces: They either don't
properly indent (as we do commonly), or they needlessly increase
indentation of the enclosed block. Hence my general preference
of switch-scope local variables.

>> @@ -2082,7 +2084,10 @@ static int hvmemul_write_cr(
>>          break;
>> 
>>      case 3:
>> -        rc = hvm_set_cr3(val, true);
>> +        noflush = hvm_pcid_enabled(current) && (val & X86_CR3_NOFLUSH);
>> +        if ( noflush )
>> +            val &= ~X86_CR3_NOFLUSH;
> 
> ... can't you just code this as:
> 
> if ( hvm_pcid_enabled(current) )
>     val &= ~X86_CR3_NOFLUSH;
> 
> ?

Because of ...

>> +        rc = hvm_set_cr3(val, noflush, true);

... this further use of "noflush" (alongside the adjusted "val").

Jan

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