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

On Mon, Jul 15, 2019 at 6:23 AM Juergen Gross <jgross at suse.com>
wrote:
>
> On 15.07.19 15:00, Andrew Cooper 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
> > independently by lapic_{suspend,resume}().
>
> Aren't those called only with CONFIG_PM set?
>

Unless I'm missing something, we only build any of the restore code
(including the write_cr8() call) if CONFIG_PM_SLEEP is set, and that
selects CONFIG_PM, so we should be fine, I think.

On 15.07.19 16:26, Andy Lutomirski wrote:
> On Mon, Jul 15, 2019 at 6:23 AM Juergen Gross <jgross at suse.com>
wrote:
>>
>> On 15.07.19 15:00, Andrew Cooper 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
>>> independently by lapic_{suspend,resume}().
>>
>> Aren't those called only with CONFIG_PM set?
>>
> 
> 
> Unless I'm missing something, we only build any of the restore code
> (including the write_cr8() call) if CONFIG_PM_SLEEP is set, and that
> selects CONFIG_PM, so we should be fine, I think.
> 
Okay, in that case I'd suggest to remove "cr8" from struct
saved_context
as it won't be used any longer.


Juergen

On 15/07/2019 15:52, Juergen Gross wrote:
> On 15.07.19 16:26, Andy Lutomirski wrote:
>> On Mon, Jul 15, 2019 at 6:23 AM Juergen Gross <jgross at
suse.com> wrote:
>>>
>>> On 15.07.19 15:00, Andrew Cooper 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
>>>> independently by lapic_{suspend,resume}().
>>>
>>> Aren't those called only with CONFIG_PM set?
>>>
>>
>>
>> Unless I'm missing something, we only build any of the restore code
>> (including the write_cr8() call) if CONFIG_PM_SLEEP is set, and that
>> selects CONFIG_PM, so we should be fine, I think.
>>
>
> Okay, in that case I'd suggest to remove "cr8" from struct
saved_context
> as it won't be used any longer.
To be honest, saving and restoring of cr8 without the rest of the Local
APIC state is conceptually broken to begin with.

If there are any bugs revealed by this, then the correct fixes are
elsewhere.

You're right about the saved_context.? I'll submit a v2 with an even
larger negative diffstat.

~Andrew