2018-10-29 13:01:01

by Marc Dionne

[permalink] [raw]
Subject: Re: x86/paravirt: Use a single ops structure

On Tue, Oct 23, 2018 at 2:37 PM Linux Kernel Mailing List
<[email protected]> wrote:
>
> Commit: 5c83511bdb9832c86be20fb86b783356e2f58062
> Parent: 27876f3882fdd4acb3d3614a0133ecdc777fc292
> Refname: refs/heads/master
> Web: https://git.kernel.org/torvalds/c/5c83511bdb9832c86be20fb86b783356e2f58062
> Author: Juergen Gross <[email protected]>
> AuthorDate: Tue Aug 28 09:40:19 2018 +0200
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Mon Sep 3 16:50:35 2018 +0200
>
> x86/paravirt: Use a single ops structure
>
> Instead of using six globally visible paravirt ops structures combine
> them in a single structure, keeping the original structures as
> sub-structures.
>
> This avoids the need to assemble struct paravirt_patch_template at
> runtime on the stack each time apply_paravirt() is being called (i.e.
> when loading a module).

The above commit replaces pv_lock_ops, which was EXPORT_SYMBOL, with
something that is part of pv_ops, which is EXPORT_SYMBOL_GPL. When
CONFIG_PARAVIRT_SPINLOCKS is set, this has the side effect of making
spin_lock() unusable by out of tree modules, which will likely be an
issue for many, if not most of them.

Marc


2018-10-29 14:27:03

by Juergen Gross

[permalink] [raw]
Subject: Re: x86/paravirt: Use a single ops structure

On 29/10/2018 13:58, Marc Dionne wrote:
> On Tue, Oct 23, 2018 at 2:37 PM Linux Kernel Mailing List
> <[email protected]> wrote:
>>
>> Commit: 5c83511bdb9832c86be20fb86b783356e2f58062
>> Parent: 27876f3882fdd4acb3d3614a0133ecdc777fc292
>> Refname: refs/heads/master
>> Web: https://git.kernel.org/torvalds/c/5c83511bdb9832c86be20fb86b783356e2f58062
>> Author: Juergen Gross <[email protected]>
>> AuthorDate: Tue Aug 28 09:40:19 2018 +0200
>> Committer: Thomas Gleixner <[email protected]>
>> CommitDate: Mon Sep 3 16:50:35 2018 +0200
>>
>> x86/paravirt: Use a single ops structure
>>
>> Instead of using six globally visible paravirt ops structures combine
>> them in a single structure, keeping the original structures as
>> sub-structures.
>>
>> This avoids the need to assemble struct paravirt_patch_template at
>> runtime on the stack each time apply_paravirt() is being called (i.e.
>> when loading a module).
>
> The above commit replaces pv_lock_ops, which was EXPORT_SYMBOL, with
> something that is part of pv_ops, which is EXPORT_SYMBOL_GPL. When
> CONFIG_PARAVIRT_SPINLOCKS is set, this has the side effect of making
> spin_lock() unusable by out of tree modules, which will likely be an
> issue for many, if not most of them.

Thanks for noticing!

Sending a patch soon...


Juergen


2018-10-30 13:35:32

by Marc Dionne

[permalink] [raw]
Subject: Re: x86/paravirt: Use a single ops structure

On Mon, Oct 29, 2018 at 11:25 AM Juergen Gross <[email protected]> wrote:
>
> On 29/10/2018 13:58, Marc Dionne wrote:
> > On Tue, Oct 23, 2018 at 2:37 PM Linux Kernel Mailing List
> > <[email protected]> wrote:
> >>
> >> Commit: 5c83511bdb9832c86be20fb86b783356e2f58062
> >> Parent: 27876f3882fdd4acb3d3614a0133ecdc777fc292
> >> Refname: refs/heads/master
> >> Web: https://git.kernel.org/torvalds/c/5c83511bdb9832c86be20fb86b783356e2f58062
> >> Author: Juergen Gross <[email protected]>
> >> AuthorDate: Tue Aug 28 09:40:19 2018 +0200
> >> Committer: Thomas Gleixner <[email protected]>
> >> CommitDate: Mon Sep 3 16:50:35 2018 +0200
> >>
> >> x86/paravirt: Use a single ops structure
> >>
> >> Instead of using six globally visible paravirt ops structures combine
> >> them in a single structure, keeping the original structures as
> >> sub-structures.
> >>
> >> This avoids the need to assemble struct paravirt_patch_template at
> >> runtime on the stack each time apply_paravirt() is being called (i.e.
> >> when loading a module).
> >
> > The above commit replaces pv_lock_ops, which was EXPORT_SYMBOL, with
> > something that is part of pv_ops, which is EXPORT_SYMBOL_GPL. When
> > CONFIG_PARAVIRT_SPINLOCKS is set, this has the side effect of making
> > spin_lock() unusable by out of tree modules, which will likely be an
> > issue for many, if not most of them.
>
> Thanks for noticing!
>
> Sending a patch soon...
>
>
> Juergen

Thanks for the quick patch. Does this also need fixing for arm/arm64?
I can't easily verify but it seems like it would have the same issue.

Marc

2018-10-30 13:51:22

by Juergen Gross

[permalink] [raw]
Subject: Re: x86/paravirt: Use a single ops structure

On 30/10/2018 14:33, Marc Dionne wrote:
> On Mon, Oct 29, 2018 at 11:25 AM Juergen Gross <[email protected]> wrote:
>>
>> On 29/10/2018 13:58, Marc Dionne wrote:
>>> On Tue, Oct 23, 2018 at 2:37 PM Linux Kernel Mailing List
>>> <[email protected]> wrote:
>>>>
>>>> Commit: 5c83511bdb9832c86be20fb86b783356e2f58062
>>>> Parent: 27876f3882fdd4acb3d3614a0133ecdc777fc292
>>>> Refname: refs/heads/master
>>>> Web: https://git.kernel.org/torvalds/c/5c83511bdb9832c86be20fb86b783356e2f58062
>>>> Author: Juergen Gross <[email protected]>
>>>> AuthorDate: Tue Aug 28 09:40:19 2018 +0200
>>>> Committer: Thomas Gleixner <[email protected]>
>>>> CommitDate: Mon Sep 3 16:50:35 2018 +0200
>>>>
>>>> x86/paravirt: Use a single ops structure
>>>>
>>>> Instead of using six globally visible paravirt ops structures combine
>>>> them in a single structure, keeping the original structures as
>>>> sub-structures.
>>>>
>>>> This avoids the need to assemble struct paravirt_patch_template at
>>>> runtime on the stack each time apply_paravirt() is being called (i.e.
>>>> when loading a module).
>>>
>>> The above commit replaces pv_lock_ops, which was EXPORT_SYMBOL, with
>>> something that is part of pv_ops, which is EXPORT_SYMBOL_GPL. When
>>> CONFIG_PARAVIRT_SPINLOCKS is set, this has the side effect of making
>>> spin_lock() unusable by out of tree modules, which will likely be an
>>> issue for many, if not most of them.
>>
>> Thanks for noticing!
>>
>> Sending a patch soon...
>>
>>
>> Juergen
>
> Thanks for the quick patch. Does this also need fixing for arm/arm64?
> I can't easily verify but it seems like it would have the same issue.

pv_lock_ops was x86 only.


Juergen