Re: [Xen-devel] [PATCH v2 1/2] x86: Meltdown band-aid against malicious 64-bit PV guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/01/18 12:33, Jan Beulich wrote:
>>
>>>>>> On 15.01.18 at 19:23, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>  can we collect these together into macros, rather than
>>>> opencoding?  We seem to have 3 distinct variations.
>>> I had considered that (following the model you use in the SP2
>>> series), but decided against it not the least because of the
>>> dependent but placement-wise separated code additions to
>>> restore original values. Plus again this might be a hindrance of
>>> backporting to really old Xen (which then typically will also be
>>> built on really old tool chains) - as you certainly recall old gas
>>> had quite a few issues with macro handling.
>> There is nothing special in these macros though?  I found the SP2-style
>> far easier to backport because they are a single slot-in line.
> I've just found the patch here needing a change in register use
> in 4.7 - such would be a little harder with pre-cooked macros,
> especially when they don't allow to specify all registers to be
> used (including ones for temporaries).
>
>> Anyway, I'm not overly fussed, but I have a
> Unfinished sentence?

Oops.  "I have a feeling that the duplication will lead to subtle bugs
when we need to change things, and inevitably miss one."

>
>>>>> @@ -585,6 +692,17 @@ ENTRY(double_fault)
>>>>>          movl  $TRAP_double_fault,4(%rsp)
>>>>>          /* Set AC to reduce chance of further SMAP faults */
>>>>>          SAVE_ALL STAC
>>>>> +
>>>>> +        GET_STACK_END(bx)
>>>>> +        mov   STACK_CPUINFO_FIELD(xen_cr3)(%rbx), %rbx
>>>>> +        test  %rbx, %rbx
>>>>> +        jz    .Ldblf_cr3_okay
>>>>> +        jns   .Ldblf_cr3_load
>>>>> +        neg   %rbx
>>>>> +.Ldblf_cr3_load:
>>>>> +        write_cr3 rbx, rdi, rsi
>>>>> +.Ldblf_cr3_okay:
>>>>> +
>>>> It is moderately common for there to be cascade faults in #DF.  This
>>>> would be better if it were the general IST switch.
>>> Based on the issues I had with #DF occurring while debugging this,
>>> I've decided to keep the code here as simple as possible without
>>> being incorrect: There's no point looking at the incoming CR3.
>>> There's no point in trying to avoid nested faults (including
>>> subsequent #DF) restoring CR3. There's also no point in retaining
>>> the value for later restoring here, as we never return. In fact, as
>>> mentioned elsewhere, we should imo indeed consider unilaterally
>>> switching to idle_pg_table[] here.
>> Ok.
> "Ok" to which parts of this - keeping the code simple, switching to
> idle_pg_table[] (perhaps in a follow-up patch), or both?

Keeping the code as it is now, and adjusting in a follow-up.  Thinking
about things, idle_pg_table[], while nice, is incompatible with a more
complete isolation strategy.

~Andrew

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