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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 02 May 2019 14:23
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Roger Pau
> Monne <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH 4/9] x86/HVM: move NOFLUSH handling out of hvm_set_cr3()
> 
> >>> On 02.05.19 at 15:07, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> 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").
> 

Ah, missed that... I'd still go for the tighter scope though, but then I don't 
mind the extra braces.

  Paul

> Jan
> 


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