Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the path

[Andy Lutomirski]

On Wed, Jun 14, 2017 at 11:25 AM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
> On Wed, Jun 14, 2017 at 2:02 PM, Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 14/06/17 18:40, Andy Lutomirski wrote:
>>> On Wed, Jun 14, 2017 at 5:40 AM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
>>>> Since tasks using IOPL are very rare, move the switching code to the slow
>>>> path for lower impact on normal tasks.
>>> I think that Andrew Cooper added a vmassist that we could opt in to
>>> that makes Xen PV IOPL switching work more or less just like native.
>>> We could maybe opt in to that and avoid needing this stuff at all on
>>> newer hypervisors.
>>
>> Indeed.
>>
>> HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_architectural_iopl);
>>
>> (if recognised) does two things.
>>
>> 1) virtual IOPL is picked up from EFLAGS in the iret frame, exactly like
>> native.
>> 2) The guest kernel is assumed to have virtual CPL0 for the purpose of
>> privilege calculations.
>>
>> Xen never runs with the real IOPL different to 0, or a PV guests could
>> disable interrupts with popf.  As a result, all IO port access does trap
>> to Xen for auditing.  What part 2) does is avoid having the awkward
>> double-step of Linux needing to set IOPL to 1 for kernel level IO access
>> to avoid faulting.
>>
>> The assist should be available in Xen 4.7 and later (or wherever vendors
>> have backported it to).
>>
>> ~Andrew
>
> Ok.  So do we keep the old code around to support older Xen
> hypervisors or just require the newer Xen for guest userspace IOPL
> support?  Part of the reason I am making these changes is to sync the
> 32-bit and 64-bit code in __switch_to(), to ultimately merge them.

I think we should keep the old code.

One way to structure this that might be nicer than using paravirt ops
would be to add a new bug X86_BUG_XEN_PV_IOPL that would only be set
on old hypervisors that don't have the assist.  Then the code could
look like:

if (static_cpu_has_bug(X86_XEN_PV_IOPL))
  xen_set_iopl(whatever);

and we wouldn't need the paravirt indirection at all.