Linux Virtualization - Jul 2019 - [PATCH v2] x86/paravirt: Drop {read,write}_cr8() hooks

On 15/07/2019 19:17, Nadav Amit wrote:
>> On Jul 15, 2019, at 8:16 AM, Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
>>
>> There is a lot of infrastructure for functionality which is used
>> exclusively in __{save,restore}_processor_state() on the suspend/resume
>> path.
>>
>> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored by
>> lapic_{suspend,resume}().  Saving and restoring cr8 independently of
the
>> rest of the Local APIC state isn't a clever thing to be doing.
>>
>> Delete the suspend/resume cr8 handling, which shrinks the size of
struct
>> saved_context, and allows for the removal of both PVOPS.
> I think removing the interface for CR8 writes is also good to avoid
> potential correctness issues, as the SDM says (10.8.6.1 "Interaction
of Task
> Priorities between CR8 and APIC?):
>
> "Operating software should implement either direct APIC TPR updates or
CR8
> style TPR updates but not mix them. Software can use a serializing
> instruction (for example, CPUID) to serialize updates between MOV CR8 and
> stores to the APIC.?
>
> And native_write_cr8() did not even issue a serializing instruction.
>
Given its location, the one write_cr8() is bounded by two serialising
operations, so is safe in practice.

However, I agree with the statement in the manual.? I could submit a v3
with an updated commit message, or let it be fixed on commit.? Whichever
is easiest.

~Andrew

On Mon, Jul 15, 2019 at 4:30 PM Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
>
> On 15/07/2019 19:17, Nadav Amit wrote:
> >> On Jul 15, 2019, at 8:16 AM, Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
> >>
> >> There is a lot of infrastructure for functionality which is used
> >> exclusively in __{save,restore}_processor_state() on the
suspend/resume
> >> path.
> >>
> >> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is
saved/restored by
> >> lapic_{suspend,resume}().  Saving and restoring cr8 independently
of the
> >> rest of the Local APIC state isn't a clever thing to be doing.
> >>
> >> Delete the suspend/resume cr8 handling, which shrinks the size of
struct
> >> saved_context, and allows for the removal of both PVOPS.
> > I think removing the interface for CR8 writes is also good to avoid
> > potential correctness issues, as the SDM says (10.8.6.1
"Interaction of Task
> > Priorities between CR8 and APIC?):
> >
> > "Operating software should implement either direct APIC TPR
updates or CR8
> > style TPR updates but not mix them. Software can use a serializing
> > instruction (for example, CPUID) to serialize updates between MOV CR8
and
> > stores to the APIC.?
> >
> > And native_write_cr8() did not even issue a serializing instruction.
> >
>
> Given its location, the one write_cr8() is bounded by two serialising
> operations, so is safe in practice.
>
> However, I agree with the statement in the manual.  I could submit a v3
> with an updated commit message, or let it be fixed on commit.  Whichever
> is easiest.
>
I don't see anything wrong with the message.  If we actually used CR8
for interrupt priorities, we wouldn't want it to serialize.  The bug
is that the code that did the write_cr8() should have had a comment as
to how it serialized against lapic_restore().  But that doesn't seem
worth mentioning in the message, since, as noted, the real problem was
that it nonsensically restored just TPR without restoring everything
else.

> On Jul 15, 2019, at 4:30 PM, Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
> 
> On 15/07/2019 19:17, Nadav Amit wrote:
>>> On Jul 15, 2019, at 8:16 AM, Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
>>> 
>>> There is a lot of infrastructure for functionality which is used
>>> exclusively in __{save,restore}_processor_state() on the
suspend/resume
>>> path.
>>> 
>>> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is saved/restored
by
>>> lapic_{suspend,resume}().  Saving and restoring cr8 independently
of the
>>> rest of the Local APIC state isn't a clever thing to be doing.
>>> 
>>> Delete the suspend/resume cr8 handling, which shrinks the size of
struct
>>> saved_context, and allows for the removal of both PVOPS.
>> I think removing the interface for CR8 writes is also good to avoid
>> potential correctness issues, as the SDM says (10.8.6.1
"Interaction of Task
>> Priorities between CR8 and APIC?):
>> 
>> "Operating software should implement either direct APIC TPR
updates or CR8
>> style TPR updates but not mix them. Software can use a serializing
>> instruction (for example, CPUID) to serialize updates between MOV CR8
and
>> stores to the APIC.?
>> 
>> And native_write_cr8() did not even issue a serializing instruction.
> 
> Given its location, the one write_cr8() is bounded by two serialising
> operations, so is safe in practice.
That?s what the ?potential? in "potential correctness issues? means :)

On 16/07/2019 01:05, Andy Lutomirski wrote:
> On Mon, Jul 15, 2019 at 4:30 PM Andrew Cooper <andrew.cooper3 at
citrix.com> wrote:
>> On 15/07/2019 19:17, Nadav Amit wrote:
>>>> On Jul 15, 2019, at 8:16 AM, Andrew Cooper <andrew.cooper3
at citrix.com> wrote:
>>>>
>>>> There is a lot of infrastructure for functionality which is
used
>>>> exclusively in __{save,restore}_processor_state() on the
suspend/resume
>>>> path.
>>>>
>>>> cr8 is an alias of APIC_TASKPRI, and APIC_TASKPRI is
saved/restored by
>>>> lapic_{suspend,resume}().  Saving and restoring cr8
independently of the
>>>> rest of the Local APIC state isn't a clever thing to be
doing.
>>>>
>>>> Delete the suspend/resume cr8 handling, which shrinks the size
of struct
>>>> saved_context, and allows for the removal of both PVOPS.
>>> I think removing the interface for CR8 writes is also good to avoid
>>> potential correctness issues, as the SDM says (10.8.6.1
"Interaction of Task
>>> Priorities between CR8 and APIC?):
>>>
>>> "Operating software should implement either direct APIC TPR
updates or CR8
>>> style TPR updates but not mix them. Software can use a serializing
>>> instruction (for example, CPUID) to serialize updates between MOV
CR8 and
>>> stores to the APIC.?
>>>
>>> And native_write_cr8() did not even issue a serializing
instruction.
>>>
>> Given its location, the one write_cr8() is bounded by two serialising
>> operations, so is safe in practice.
>>
>> However, I agree with the statement in the manual.  I could submit a v3
>> with an updated commit message, or let it be fixed on commit. 
Whichever
>> is easiest.
>>
> I don't see anything wrong with the message.  If we actually used CR8
> for interrupt priorities, we wouldn't want it to serialize.  The bug
> is that the code that did the write_cr8() should have had a comment as
> to how it serialized against lapic_restore().  But that doesn't seem
> worth mentioning in the message, since, as noted, the real problem was
> that it nonsensically restored just TPR without restoring everything
> else.
Fair enough, in which case I'm happy with v2 as it is.

~Andrew