linux-kernel - Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <36a22b0a-e745-4f6f-ca9a-fe549aad930e@oracle.com>
Date:   Wed, 8 Nov 2017 13:01:42 +0000
From:   Joao Martins <joao.m.martins@...cle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Radim Krcmar <rkrcmar@...hat.com>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH v7 2/5] x86/pvclock: add setter for pvclock_pvti_cpu0_va

On 11/08/2017 11:06 AM, Thomas Gleixner wrote:
> On Tue, 7 Nov 2017, Joao Martins wrote:
>> On 11/06/2017 04:09 PM, Paolo Bonzini wrote:
>>> On 19/10/2017 15:39, Joao Martins wrote:
>>>> Right now there is only a pvclock_pvti_cpu0_va() which is defined
>>>> on kvmclock since:
>>>>
>>>> commit dac16fba6fc5
>>>> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap")
>>>>
>>>> The only user of this interface so far is kvm. This commit adds a
>>>> setter function for the pvti page and moves pvclock_pvti_cpu0_va
>>>> to pvclock, which is a more generic place to have it; and would
>>>> allow other PV clocksources to use it, such as Xen.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@...cle.com>
>>>> Acked-by: Andy Lutomirski <luto@...nel.org>
>>>
>>> Acked-by: Paolo Bonzini <pbonzini@...hat.com>
>>>
>>> IOW, the Xen folks are free to pick up the whole series. :)
>>>
>> Thank you!
>>
>> I guess only x86 maintainers Ack is left - any comments?
> 
> The only nit-pick I have are the convoluted function names:
> 
>     pvclock_set_pvti_cpu0_va() pvclock_pvti_cpu0_va()
> 
> What on earth does that mean?
>
Those two functions respectively set and get in pvclock common code the address
of a page for vCPU 0 containing time info (pvti, which is periodically updated
by hypervisor). This region is guest memory and registered with hypervisor by
guest PV clocksource and set in pvclock if certain conditions are met (i.e.
PVCLOCK_TSC_STABLE_BIT is supported by hypervisor), and the getter is afterwards
used by vdso and ptp_kvm.

FWIW I merely followed the current style/code of the existent function but there
could be a better name like "pvclock_set_data() pvclock_get_data()". Albeit the
current names are more explicit on what we should expect to set or return from
the functions.

> Aside of that can you please make it at least symetric, i.e. _set_ and
> _get_ ?
> 
OK - Provided this is changing an exported symbol (pvclock_pvti_cpu0_va in use
by ptp_kvm) and a non-functional change would you want me to address in a
separate patch or it is OK to have in this one?

> Other than that:
> 
>       Acked-by: Thomas Gleixner <tglx@...utronix.de>
> 
Thanks!