xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andy Lutomirski <luto@amacapital.net>,
	Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 2/2] xen/x86: Introduce a new VMASSIST for architectural behaviour of iopl
Date: Thu, 17 Mar 2016 10:45:32 +0000	[thread overview]
Message-ID: <56EA8ACC.2080102@citrix.com> (raw)
In-Reply-To: <56EA941102000078000DDB1E@prv-mh.provo.novell.com>

On 17/03/16 10:25, Jan Beulich wrote:
>>>> On 16.03.16 at 21:05, <andrew.cooper3@citrix.com> wrote:
>> @@ -1742,8 +1742,10 @@ static void load_segments(struct vcpu *n)
>>              cs_and_mask = (unsigned short)regs->cs |
>>                  ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16);
>>              /* Fold upcall mask into RFLAGS.IF. */
>> -            eflags  = regs->_eflags & ~X86_EFLAGS_IF;
>> +            eflags  = regs->_eflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> This and ...
>
>> @@ -1788,8 +1790,10 @@ static void load_segments(struct vcpu *n)
>>              ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32);
>>  
>>          /* Fold upcall mask into RFLAGS.IF. */
>> -        rflags  = regs->rflags & ~X86_EFLAGS_IF;
>> +        rflags  = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> ... this is not really necessary (but also not wrong) - the actual
> EFLAGS.IOPL is always zero (and assumed to be so by code
> further down from the respective adjustments you make). For
> consistency's sake it might be better to either drop the changes
> here, or also adjust the two places masking regs->eflags.

I will adjust the others.  I would prefer not to rely on the assumption
that it is actually 0.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1806,7 +1806,9 @@ static int guest_io_okay(
>>  #define TOGGLE_MODE() if ( user_mode ) toggle_guest_mode(v)
>>  
>>      if ( !vm86_mode(regs) &&
>> -         (v->arch.pv_vcpu.iopl >= (guest_kernel_mode(v, regs) ? 1 : 3)) )
>> +         (MASK_EXTR(v->arch.pv_vcpu.iopl, X86_EFLAGS_IOPL) >=
>> +          (guest_kernel_mode(v, regs) ?
>> +           (VM_ASSIST(v->domain, architectural_iopl) ? 0 : 1) : 3)) )
>>          return 1;
>>  
>>      if ( v->arch.pv_vcpu.iobmp_limit > (port + bytes) )
>> @@ -2367,7 +2369,9 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>>  
>>      case 0xfa: /* CLI */
>>      case 0xfb: /* STI */
>> -        if ( v->arch.pv_vcpu.iopl < (guest_kernel_mode(v, regs) ? 1 : 3) )
>> +        if ( MASK_EXTR(v->arch.pv_vcpu.iopl, X86_EFLAGS_IOPL) <
>> +             (guest_kernel_mode(v, regs) ?
>> +              (VM_ASSIST(v->domain, architectural_iopl) ? 0 : 1) : 3) )
>>              goto fail;
> The similarity of the two together with the growing complexity
> suggests to make this a macro or inline function. Additionally
> resulting binary code would likely be better if you compared
> v->arch.pv_vcpu.iopl with MASK_INSR(<literal value>,
> X86_EFLAGS_IOPL), even if that means having three
> MASK_INSR() (albeit those perhaps again would be hidden in
> a macro, e.g.
>
> #define IOPL(n) MASK_INSR(n, X86_EFLAGS_IOPL)

I will see what I can do.

>
> ).
>
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -277,9 +277,14 @@ compat_create_bounce_frame:
>>          testb %al,%al                   # Bits 0-7: saved_upcall_mask
>>          setz  %ch                       # %ch == !saved_upcall_mask
>>          movl  UREGS_eflags+8(%rsp),%eax
>> -        andl  $~X86_EFLAGS_IF,%eax
>> +        andl  $~(X86_EFLAGS_IF|X86_EFLAGS_IOPL),%eax
> See earlier comment.

I hope I have suitably answered why this is staying.

>
>>          addb  %ch,%ch                   # Bit 9 (EFLAGS.IF)
>>          orb   %ch,%ah                   # Fold EFLAGS.IF into %eax
>> +        movq  VCPU_domain(%rbx),%rcx    # if ( VM_ASSIST(v->domain, architectural_iopl) )
> If you used another register, this could be pulled up quite a bit,
> to hide the latency of the load before the loaded value gets used.

Can do, but given all pipeline stalls which currently exist in this
code, I doubt it will make any observable difference.

>
>> +        testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rcx)
>> +        jz    .Lft6
>> +        movzwl VCPU_iopl(%rbx),%ecx     # Bits 13:12 (EFLAGS.IOPL)
> Why not just MOVL?

Ah yes - this is leftover from the first iteration.

~Andrew

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

  reply	other threads:[~2016-03-17 10:45 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-16 20:05 [PATCH 0/2] XSA-171 Followup work Andrew Cooper
2016-03-16 20:05 ` [PATCH 1/2] xen/x86: Don't hold TRAPBOUNCE_flags in %cl during create_bounce_frame Andrew Cooper
2016-03-16 20:05 ` [PATCH 2/2] xen/x86: Introduce a new VMASSIST for architectural behaviour of iopl Andrew Cooper
2016-03-17 10:25   ` Jan Beulich
2016-03-17 10:45     ` Andrew Cooper [this message]
2016-03-17 11:00       ` Jan Beulich
2016-03-17 11:05         ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56EA8ACC.2080102@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=luto@amacapital.net \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [PATCH 2/2] xen/x86: Introduce a new VMASSIST for architectural behaviour of iopl' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).