| Message ID | 20190906153948.2160342-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: xen: unexport HYPERVISOR_platform_op function | expand |
On 06/09/2019 16:39, Arnd Bergmann wrote: > HYPERVISOR_platform_op() is an inline function and should not > be exported. Since commit 15bfc2348d54 ("modpost: check for > static EXPORT_SYMBOL* functions"), this causes a warning: > > WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > Remove the extraneous export. > > Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Something is wonky. That symbol is (/ really ought to be) in the hypercall page and most definitely not inline. Which tree is that changeset from? I can't find the SHA. I hate to open a separate can of worms, but why are they tagged GPL? The Xen hypercall ABI, like the Linux syscall ABI, are specifically not GPL. Xen has as something very similar to (and probably derived from) the Linux-syscall-note exception. ~Andrew
On 06.09.2019 17:55, Andrew Cooper wrote: > On 06/09/2019 16:39, Arnd Bergmann wrote: >> HYPERVISOR_platform_op() is an inline function and should not >> be exported. Since commit 15bfc2348d54 ("modpost: check for >> static EXPORT_SYMBOL* functions"), this causes a warning: >> >> WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL >> >> Remove the extraneous export. >> >> Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Something is wonky. That symbol is (/ really ought to be) in the > hypercall page and most definitely not inline. It's HYPERVISOR_platform_op_raw that's in the hypercall page afaics. Jan
On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 06/09/2019 16:39, Arnd Bergmann wrote: > > HYPERVISOR_platform_op() is an inline function and should not > > be exported. Since commit 15bfc2348d54 ("modpost: check for > > static EXPORT_SYMBOL* functions"), this causes a warning: > > > > WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > > > Remove the extraneous export. > > > > Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Something is wonky. That symbol is (/ really ought to be) in the > hypercall page and most definitely not inline. > > Which tree is that changeset from? I can't find the SHA. This is from linux-next, I think from the kbuild tree. Arnd
On 06/09/2019 17:00, Arnd Bergmann wrote: > On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 06/09/2019 16:39, Arnd Bergmann wrote: >>> HYPERVISOR_platform_op() is an inline function and should not >>> be exported. Since commit 15bfc2348d54 ("modpost: check for >>> static EXPORT_SYMBOL* functions"), this causes a warning: >>> >>> WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL >>> >>> Remove the extraneous export. >>> >>> Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Something is wonky. That symbol is (/ really ought to be) in the >> hypercall page and most definitely not inline. >> >> Which tree is that changeset from? I can't find the SHA. > This is from linux-next, I think from the kbuild tree. Thanks. Julien/Stefano: Why are any of these hypercalls out-of-line? ARM doesn't use the hypercall page, and there is no argument translation (not even in arm32 as there are no 5-argument hypercalls declared). They'd surely be easier to implement with a few static inlines and some common code, than to try and replicate the x86 side hypercall_page interface ? ~Andrew
Hi Andrew, On 9/6/19 6:20 PM, Andrew Cooper wrote: > On 06/09/2019 17:00, Arnd Bergmann wrote: >> On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> On 06/09/2019 16:39, Arnd Bergmann wrote: >>>> HYPERVISOR_platform_op() is an inline function and should not >>>> be exported. Since commit 15bfc2348d54 ("modpost: check for >>>> static EXPORT_SYMBOL* functions"), this causes a warning: >>>> >>>> WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL >>>> >>>> Remove the extraneous export. >>>> >>>> Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") >>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> Something is wonky. That symbol is (/ really ought to be) in the >>> hypercall page and most definitely not inline. >>> >>> Which tree is that changeset from? I can't find the SHA. >> This is from linux-next, I think from the kbuild tree. > > Thanks. > > Julien/Stefano: Why are any of these hypercalls out-of-line? ARM > doesn't use the hypercall page, and there is no argument translation > (not even in arm32 as there are no 5-argument hypercalls declared). I am not sure how the hypercall page makes things different. You still have to store the arguments in the correct register so... > > They'd surely be easier to implement with a few static inlines and some > common code, than to try and replicate the x86 side hypercall_page > interface ? ... I don't think they will be easier to implement with a few static inlines. The implementation will likely end up to be similar to arch/x86/asm/xen/hypercall.h. Furthermore, one of the downside of per-arch static inline is it is more difficult to ensure the prototype match for all the architectures. Although, it might be possible to make them common by only requesting per-arch to implement HYPERCALL_N(...). So I think the code is better as it is. While looking at the code, I also realized that the implementation of HYPERCALL_dm_op might be incorrect for Arm32. Similarly do privcmd call, I think dm_op call should enable user access as they will be used by userspace. We don't use dm_op on Arm so far, hence why I think this was unnoticed. I will see if I can reproduce it and send a patch. Cheers,
Hi Julien, On Sat, Sep 07, 2019 at 11:05:45AM +0100, Julien Grall wrote: > On 9/6/19 6:20 PM, Andrew Cooper wrote: > > On 06/09/2019 17:00, Arnd Bergmann wrote: > > > On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > On 06/09/2019 16:39, Arnd Bergmann wrote: > > > > > HYPERVISOR_platform_op() is an inline function and should not > > > > > be exported. Since commit 15bfc2348d54 ("modpost: check for > > > > > static EXPORT_SYMBOL* functions"), this causes a warning: > > > > > > > > > > WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > > > > > > > > > Remove the extraneous export. > > > > > > > > > > Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > Something is wonky. That symbol is (/ really ought to be) in the > > > > hypercall page and most definitely not inline. > > > > > > > > Which tree is that changeset from? I can't find the SHA. > > > This is from linux-next, I think from the kbuild tree. > > > > Thanks. > > > > Julien/Stefano: Why are any of these hypercalls out-of-line? ARM > > doesn't use the hypercall page, and there is no argument translation > > (not even in arm32 as there are no 5-argument hypercalls declared). > > I am not sure how the hypercall page makes things different. You still have > to store the arguments in the correct register so... > > > > > They'd surely be easier to implement with a few static inlines and some > > common code, than to try and replicate the x86 side hypercall_page > > interface ? > > ... I don't think they will be easier to implement with a few static > inlines. The implementation will likely end up to be similar to > arch/x86/asm/xen/hypercall.h. > > Furthermore, one of the downside of per-arch static inline is it is more > difficult to ensure the prototype match for all the architectures. Although, > it might be possible to make them common by only requesting per-arch to > implement HYPERCALL_N(...). > > So I think the code is better as it is. > > While looking at the code, I also realized that the implementation of > HYPERCALL_dm_op might be incorrect for Arm32. Similarly do privcmd call, I > think dm_op call should enable user access as they will be used by > userspace. > > We don't use dm_op on Arm so far, hence why I think this was unnoticed. I > will see if I can reproduce it and send a patch. I'm seeing this when building arm64 defconfig v5.4-rc1: | [mark@lakrids:~/src/linux]% usekorg 8.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j56 -s | arch/arm64/Makefile:62: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL I couldn't see a follow-up; do you have a patch for this? Thanks, Mark.
On 01/10/2019 15:33, Mark Rutland wrote: > Hi Julien, Hi Mark, > > On Sat, Sep 07, 2019 at 11:05:45AM +0100, Julien Grall wrote: >> On 9/6/19 6:20 PM, Andrew Cooper wrote: >>> On 06/09/2019 17:00, Arnd Bergmann wrote: >>>> On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>>> On 06/09/2019 16:39, Arnd Bergmann wrote: >>>>>> HYPERVISOR_platform_op() is an inline function and should not >>>>>> be exported. Since commit 15bfc2348d54 ("modpost: check for >>>>>> static EXPORT_SYMBOL* functions"), this causes a warning: >>>>>> >>>>>> WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL >>>>>> >>>>>> Remove the extraneous export. >>>>>> >>>>>> Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") >>>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>>>> Something is wonky. That symbol is (/ really ought to be) in the >>>>> hypercall page and most definitely not inline. >>>>> >>>>> Which tree is that changeset from? I can't find the SHA. >>>> This is from linux-next, I think from the kbuild tree. >>> >>> Thanks. >>> >>> Julien/Stefano: Why are any of these hypercalls out-of-line? ARM >>> doesn't use the hypercall page, and there is no argument translation >>> (not even in arm32 as there are no 5-argument hypercalls declared). >> >> I am not sure how the hypercall page makes things different. You still have >> to store the arguments in the correct register so... >> >>> >>> They'd surely be easier to implement with a few static inlines and some >>> common code, than to try and replicate the x86 side hypercall_page >>> interface ? >> >> ... I don't think they will be easier to implement with a few static >> inlines. The implementation will likely end up to be similar to >> arch/x86/asm/xen/hypercall.h. >> >> Furthermore, one of the downside of per-arch static inline is it is more >> difficult to ensure the prototype match for all the architectures. Although, >> it might be possible to make them common by only requesting per-arch to >> implement HYPERCALL_N(...). >> >> So I think the code is better as it is. >> >> While looking at the code, I also realized that the implementation of >> HYPERCALL_dm_op might be incorrect for Arm32. Similarly do privcmd call, I >> think dm_op call should enable user access as they will be used by >> userspace. >> >> We don't use dm_op on Arm so far, hence why I think this was unnoticed. I >> will see if I can reproduce it and send a patch. > > I'm seeing this when building arm64 defconfig v5.4-rc1: > > | [mark@lakrids:~/src/linux]% usekorg 8.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j56 -s > | arch/arm64/Makefile:62: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built > | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > I couldn't see a follow-up; do you have a patch for this? The first e-mail of the thread should contain a patch to address the warning (see [1]). But it is still waiting on an Ack from Stefano so it can get merged. Cheers, [1] https://patchwork.kernel.org/patch/11135601/
On Tue, Oct 01, 2019 at 03:39:41PM +0100, Julien Grall wrote: > On 01/10/2019 15:33, Mark Rutland wrote: > > On Sat, Sep 07, 2019 at 11:05:45AM +0100, Julien Grall wrote: > > > On 9/6/19 6:20 PM, Andrew Cooper wrote: > > > > On 06/09/2019 17:00, Arnd Bergmann wrote: > > > > > On Fri, Sep 6, 2019 at 5:55 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > > > On 06/09/2019 16:39, Arnd Bergmann wrote: > > > > > > > HYPERVISOR_platform_op() is an inline function and should not > > > > > > > be exported. Since commit 15bfc2348d54 ("modpost: check for > > > > > > > static EXPORT_SYMBOL* functions"), this causes a warning: > > > > > > > > > > > > > > WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > > > > > > > > > > > > > Remove the extraneous export. > > > > > > > > > > > > > > Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") > > > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> [...] > > > While looking at the code, I also realized that the implementation of > > > HYPERCALL_dm_op might be incorrect for Arm32. Similarly do privcmd call, I > > > think dm_op call should enable user access as they will be used by > > > userspace. > > > > > > We don't use dm_op on Arm so far, hence why I think this was unnoticed. I > > > will see if I can reproduce it and send a patch. > > > > I'm seeing this when building arm64 defconfig v5.4-rc1: > > > > | [mark@lakrids:~/src/linux]% usekorg 8.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j56 -s > > | arch/arm64/Makefile:62: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built > > | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > | WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > > > I couldn't see a follow-up; do you have a patch for this? > > The first e-mail of the thread should contain a patch to address the warning > (see [1]). But it is still waiting on an Ack from Stefano so it can get > merged. Ah, sorry. I misunderstood what you were planning to send a patch for, and assumed you were going to propose an alternative to Arnd's patch. Stefano, do you see any problem with Arnd's patch? If not, it would be good to get this merged soon. Thanks, Mark. > > Cheers, > > [1] https://patchwork.kernel.org/patch/11135601/ > > -- > Julien Grall
On Fri, 6 Sep 2019, Arnd Bergmann wrote: > HYPERVISOR_platform_op() is an inline function and should not > be exported. Since commit 15bfc2348d54 ("modpost: check for > static EXPORT_SYMBOL* functions"), this causes a warning: > > WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL > > Remove the extraneous export. > > Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/arm/xen/enlighten.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 1e57692552d9..845c528acf24 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -437,7 +437,6 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op); > -EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_multicall); > EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist); > EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op); Hi Arnd, Thank you for the patch. HYPERVISOR_platform_op() is an inline function, the underlying function that should be exported is HYPERVISOR_platform_op_raw. So, instead of removing HYPERVISOR_platform_op, we should change it to HYPERVISOR_platform_op_raw. For convenience, and for testing I cooked up a patch. Arnd, if you are happy with it (in the sense that it solves your problem) we'll check it in the xentip tree, unless you would like to get it in your tree? Cheers, Stefano --- From: Stefano Stabellini <stefano.stabellini@xilinx.com> HYPERVISOR_platform_op() is an inline function and should not be exported. Since commit 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions"), this causes a warning: WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL Instead, export the underlying function called by the static inline: HYPERVISOR_platform_op_raw. Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 1e57692552d9..522c97d43ef8 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op); EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op); EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op); EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op); -EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op); +EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op_raw); EXPORT_SYMBOL_GPL(HYPERVISOR_multicall); EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist); EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op);
On Tue, Oct 1, 2019 at 7:38 PM Stefano Stabellini <sstabellini@kernel.org> wrote: > Thank you for the patch. HYPERVISOR_platform_op() is an inline function, > the underlying function that should be exported is > HYPERVISOR_platform_op_raw. So, instead of removing > HYPERVISOR_platform_op, we should change it to > HYPERVISOR_platform_op_raw. Ok, that makes sense. > For convenience, and for testing I cooked up a patch. Arnd, if you are > happy with it (in the sense that it solves your problem) we'll check it > in the xentip tree, unless you would like to get it in your tree? > Please merge it through your tree. > @@ -437,7 +437,7 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op); > -EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op); > +EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op_raw); > EXPORT_SYMBOL_GPL(HYPERVISOR_multicall); > EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist); > EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op); Note that there are obviously no callers from any loadable modules in the kernel, all users are in built-in code at the moment. As an API definition it still makes sense though. Arnd
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 1e57692552d9..845c528acf24 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -437,7 +437,6 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op); EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op); EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op); EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op); -EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op); EXPORT_SYMBOL_GPL(HYPERVISOR_multicall); EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist); EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op);
HYPERVISOR_platform_op() is an inline function and should not be exported. Since commit 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions"), this causes a warning: WARNING: "HYPERVISOR_platform_op" [vmlinux] is a static EXPORT_SYMBOL_GPL Remove the extraneous export. Fixes: 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL* functions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/arm/xen/enlighten.c | 1 - 1 file changed, 1 deletion(-)