Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
Michael S. Tsirkin writes: > On Thu, Jun 27, 2019 at 10:58:40PM -0300, Thiago Jung Bauermann wrote: >> >> Michael S. Tsirkin writes: >> >> > On Mon, Jun 03, 2019 at 10:13:59PM -0300, Thiago Jung Bauermann wrote: >> >> >> >> >> >> Michael S. Tsirkin writes: >> >> >> >> > On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote: >> >> >> I rephrased it in terms of address translation. What do you think of >> >> >> this version? The flag name is slightly different too: >> >> >> >> >> >> >> >> >> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same >> >> >> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set, >> >> >> with the exception that address translation is guaranteed to be >> >> >> unnecessary when accessing memory addresses supplied to the device >> >> >> by the driver. Which is to say, the device will always use physical >> >> >> addresses matching addresses used by the driver (typically meaning >> >> >> physical addresses used by the CPU) and not translated further. >> >> >> This >> >> >> flag should be set by the guest if offered, but to allow for >> >> >> backward-compatibility device implementations allow for it to be >> >> >> left unset by the guest. It is an error to set both this flag and >> >> >> VIRTIO_F_ACCESS_PLATFORM. >> >> > >> >> > >> >> > >> >> > >> >> > OK so VIRTIO_F_ACCESS_PLATFORM is designed to allow unpriveledged >> >> > drivers. This is why devices fail when it's not negotiated. >> >> >> >> Just to clarify, what do you mean by unprivileged drivers? Is it drivers >> >> implemented in guest userspace such as with VFIO? Or unprivileged in >> >> some other sense such as needing to use bounce buffers for some reason? >> > >> > I had drivers in guest userspace in mind. >> >> Great. Thanks for clarifying. >> >> I don't think this flag would work for guest userspace drivers. Should I >> add a note about that in the flag definition? > > I think you need to clarify access protection rules. Is it only > translation that is bypassed or is any platform-specific > protection mechanism bypassed too? It is only translation. In a secure guest, if the device tries to access a memory address that wasn't provided by the driver then the architecture will deny that access. If the device accesses addresses provided to it by the driver, then there's no protection mechanism or translation to get in the way. >> >> > This confuses me. >> >> > If driver is unpriveledged then what happens with this flag? >> >> > It can supply any address it wants. Will that corrupt kernel >> >> > memory? >> >> >> >> Not needing address translation doesn't necessarily mean that there's no >> >> IOMMU. On powerpc we don't use VIRTIO_F_ACCESS_PLATFORM but there's >> >> always an IOMMU present. And we also support VFIO drivers. The VFIO API >> >> for pseries (sPAPR section in Documentation/vfio.txt) has extra ioctls >> >> to program the IOMMU. >> >> >> >> For our use case, we don't need address translation because we set up an >> >> identity mapping in the IOMMU so that the device can use guest physical >> >> addresses. > > OK so I think I am beginning to see it in a different light. Right now the > specific > platform creates an identity mapping. That in turn means DMA API can be > fast - it does not need to do anything. What you are looking for is a > way to tell host it's an identity mapping - just as an optimization. > > Is that right? Almost. Theoretically it is just an optimization. But in practice the pseries boot firmware (SLOF) doesn't support IOMMU_PLATFORM so it's not possible to boot a guest from a device with that flag set. > So this is what I would call this option: > > VIRTIO_F_ACCESS_PLATFORM_IDENTITY_ADDRESS > > and the explanation should state that all device > addresses are translated by the platform to identical > addresses. > > In fact this option then becomes more, not less restrictive > than VIRTIO_F_ACCESS_PLATFORM - it's a promise > by guest to only create identity mappings, > and only before driver_ok is set. > This option then would always be negotiated together with > VIRTIO_F_ACCESS_PLATFORM. > > Host then must verify that > 1. full 1:1 mappings are created before driver_ok > or can we make sure this happens before features_ok? > that would be ideal as we could require that features_ok fails > 2. mappings are not modified between driver_ok and reset > i guess attempts to change them will fail - > possibly by causing a guest crash > or some other kind of platform-specific error I think VIRTIO_F_ACCESS_PLATFORM_IDENTITY_ADDRESS is good, but requiring it to be accompanied by ACCESS_PLATFORM can be a problem. One reason is SLOF as I mentioned above, another is that we would be requiring all guests running on the machine (secure guests or not, since we would use the same configuration for all guests) to support it. But ACCESS_PLATFORM is relatively recent so it's a bit early for that. For
Re: [PATCH v9 05/10] namei: O_BENEATH-style path resolution flags
On Sat, Jul 13, 2019 at 03:41:53AM +0100, Al Viro wrote:
> On Fri, Jul 12, 2019 at 04:00:26PM +0100, Al Viro wrote:
> > On Fri, Jul 12, 2019 at 02:25:53PM +0100, Al Viro wrote:
> >
> > > if (flags & LOOKUP_BENEATH) {
> > > nd->root = nd->path;
> > > if (!(flags & LOOKUP_RCU))
> > > path_get(&nd->root);
> > > else
> > > nd->root_seq = nd->seq;
> >
> > BTW, this assignment is needed for LOOKUP_RCU case. Without it
> > you are pretty much guaranteed that lazy pathwalk will fail,
> > when it comes to complete_walk().
> >
> > Speaking of which, what would happen if LOOKUP_ROOT/LOOKUP_BENEATH
> > combination would someday get passed?
>
> I don't understand what's going on with ->r_seq in there - your
> call of path_is_under() is after having (re-)sampled rename_lock,
> but if that was the only .. in there, who's going to recheck
> the value? For that matter, what's to guarantee that the thing
> won't get moved just as you are returning from handle_dots()?
>
> IOW, what does LOOKUP_IN_ROOT guarantee for caller (openat2())?
Sigh... Usual effects of trying to document things:
1) LOOKUP_NO_EVAL looks bogus. It had been introduced by commit 57d4657716ac
(audit: ignore fcaps on umount) and AFAICS it's crap. It is set in
ksys_umount() and nowhere else. It's ignored by everything except
filename_mountpoint(). The thing is, call graph for filename_mountpoint()
is
filename_mountpoint()
<- user_path_mountpoint_at()
<- ksys_umount()
<- kern_path_mountpoint()
<- autofs_dev_ioctl_ismountpoint()
<- find_autofs_mount()
<- autofs_dev_ioctl_open_mountpoint()
<- autofs_dev_ioctl_requester()
<- autofs_dev_ioctl_ismountpoint()
In other words, that flag is basically "was filename_mountpoint()
been called by umount(2) or has it come from an autofs ioctl?".
And looking at the rationale in that commit, autofs ioctls need
it just as much as umount(2) does. Why is it not set for those
as well? And why is it conditional at all?
1b) ... because audit_inode() wants LOOKUP_... as the last argument,
only to remap it into AUDIT_..., that's why. So audit needs something
guaranteed not to conflict with LOOKUP_PARENT (another flag getting
remapped). So why do we bother with remapping those, anyway? Let's look
at the callers:
fs/namei.c:933: audit_inode(nd->name, nd->stack[0].link.dentry, 0);
fs/namei.c:2353:audit_inode(name, path->dentry, flags &
LOOKUP_PARENT);
fs/namei.c:2394:audit_inode(name, parent->dentry,
LOOKUP_PARENT);
fs/namei.c:2721:audit_inode(name, path->dentry, flags &
LOOKUP_NO_EVAL);
fs/namei.c:3302:audit_inode(nd->name, dir, LOOKUP_PARENT);
fs/namei.c:3336:audit_inode(nd->name, file->f_path.dentry, 0);
fs/namei.c:3371:audit_inode(nd->name, path.dentry, 0);
fs/namei.c:3389:audit_inode(nd->name, nd->path.dentry, 0);
fs/namei.c:3490:audit_inode(nd->name, child, 0);
fs/namei.c:3509:audit_inode(nd->name, path.dentry, 0);
ipc/mqueue.c:788: audit_inode(name, dentry, 0);
In all but two of those we have a nice constant value - 0 or AUDIT_INODE_PARENT.
One of two exceptions is in filename_mountpoint(), and there we want
unconditional AUDIT_INODE_NOEVAL (see above). What of the other? It's
if (likely(!retval))
audit_inode(name, path->dentry, flags & LOOKUP_PARENT);
in filename_lookup(). And that is bogus as well. filename_lookupat() would
better *NOT* get LOOKUP_PARENT in flags. And it doesn't - not since
commit 8bcb77fabd7c (namei: split off filename_lookupat() with LOOKUP_PARENT)
back in 2015. In filename_parentat() introduced there we have
audit_inode(name, parent->dentry, LOOKUP_PARENT);
and at the same point the call in filename_lookupat() should've become
audit_inode(name, path->dentry, 0);
It hadn't; my fault. And after fixing that everything becomes nice and
unconditional - the last argument of audit_inode() is always an AUDIT_...
constant or zero. Moving AUDIT_... definitions outside of ifdef on
CONFIG_AUDITSYSCALL, getting rid of remapping in audit_inode() and
passing the right values in 3 callers that don't pass 0 and LOOKUP_NO_EVAL
can go to hell.
Any objections from audit folks?
2) comment in namei.h is seriously out of sync with reality. To quote:
* - follow links at the end
OK, that's LOOKUP_FOLLOW (1)
* - require a directory
... and LOOKUP_DIRECTORY (2)
* - ending slashes ok even for nonexistent files
... used to be about LOOKUP_CONTINUE (eight years dead now)
* - internal "there are more path components" flag
... LOOKUP_PARENT (16)
* - dentry cache is untrusted; force a real lookup
... LOOKUP_REVAL (32)
* - suppress terminal auto
Re: [RFC PATCH kernel] powerpc/xive: Drop deregistered irqs
On Sat, 2019-07-13 at 18:53 +1000, Alexey Kardashevskiy wrote: > > On 13/07/2019 09:47, Benjamin Herrenschmidt wrote: > > On Fri, 2019-07-12 at 19:37 +1000, Alexey Kardashevskiy wrote: > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/irq.c#n614 > > > > > > If so, then in order to do EOI, I'll need the desc which is gone, > > > or > > > I am missing the point? > > > > All you need is drop the local CPU priority. > > I know that, the question was how. I cannot use irq_chip in > arch/powerpc/kernel/irq.c and I do not want to add > ppc_md.enable_irqs(). Well, best is probably to do just that though, but call it something like ppc_md.orphan_irq() or something like that instead. Another option as you mention is to try to scrub queues, but that's trickier to do due to the lockless nature of the queue handling. Cheers, Ben.
Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
On Sun, Jul 14, 2019 at 07:45:15AM +0900, Masahiro Yamada wrote:
> On Sat, Jul 13, 2019 at 10:17 PM Segher Boessenkool
> wrote:
> > On Sat, Jul 13, 2019 at 07:47:44AM -0500, Segher Boessenkool wrote:
> > > On Sat, Jul 13, 2019 at 12:21:06PM +0900, Masahiro Yamada wrote:
> > > > The KBUILD_ARFLAGS addition in arch/powerpc/Makefile has never worked
> > > > in a useful way because it is always overridden by the following code
> > > > in the top Makefile:
> > > >
> > > > # use the deterministic mode of AR if available
> > > > KBUILD_ARFLAGS := $(call ar-option,D)
> > > >
> > > > The code in the top Makefile was added in 2011, by commit 40df759e2b9e
> > > > ("kbuild: Fix build with binutils <= 2.19").
> > > >
> > > > The KBUILD_ARFLAGS addition for ppc has always been dead code from the
> > > > beginning.
> > >
> > > That was added in 43c9127d94d6 to replace my 8995ac870273 from 2007.
> > >
> > > Is it no longer supported to build a 64-bit kernel with a toolchain
> > > that defaults to 32-bit, or the other way around? And with non-native
> > > toolchains (this one didn't run on Linux, even).
> >
> > It was an --enable-targets=all toolchain, somewhat common for crosses,
> > if that matters.
>
> I always use the same toolchain
> for compile-testing PPC32/64.
>
> I have never been hit by the issue you mention.
> Somebody would have reported it if it were still a problem.
But did you use --enable-targets=all?
The problem was empty archives IIRC. Not a problem anymore with thin
archives, maybe?
> Moreover, commit 43c9127d94d6
> translated the environment variable "GNUTARGET"
> into the command option "--target="
>
> My powerpc-linux-ar does not know it:
>
> powerpc-linux-ar: -t: No such file or directory
Yes, that is why I used the environment variable, all binutils work
with that. There was no --target option in GNU ar before 2.22.
Segher
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.3-1 tag
The pull request you sent on Sat, 13 Jul 2019 14:28:00 +1000: > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git > tags/powerpc-5.3-1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/192f0f8e9db7efe4ac98d47f5fa4334e43c1204d Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
On Sat, Jul 13, 2019 at 10:17 PM Segher Boessenkool
wrote:
>
> On Sat, Jul 13, 2019 at 07:47:44AM -0500, Segher Boessenkool wrote:
> > On Sat, Jul 13, 2019 at 12:21:06PM +0900, Masahiro Yamada wrote:
> > > The KBUILD_ARFLAGS addition in arch/powerpc/Makefile has never worked
> > > in a useful way because it is always overridden by the following code
> > > in the top Makefile:
> > >
> > > # use the deterministic mode of AR if available
> > > KBUILD_ARFLAGS := $(call ar-option,D)
> > >
> > > The code in the top Makefile was added in 2011, by commit 40df759e2b9e
> > > ("kbuild: Fix build with binutils <= 2.19").
> > >
> > > The KBUILD_ARFLAGS addition for ppc has always been dead code from the
> > > beginning.
> >
> > That was added in 43c9127d94d6 to replace my 8995ac870273 from 2007.
> >
> > Is it no longer supported to build a 64-bit kernel with a toolchain
> > that defaults to 32-bit, or the other way around? And with non-native
> > toolchains (this one didn't run on Linux, even).
>
> It was an --enable-targets=all toolchain, somewhat common for crosses,
> if that matters.
I always use the same toolchain
for compile-testing PPC32/64.
I have never been hit by the issue you mention.
Somebody would have reported it if it were still a problem.
Moreover, commit 43c9127d94d6
translated the environment variable "GNUTARGET"
into the command option "--target="
My powerpc-linux-ar does not know it:
powerpc-linux-ar: -t: No such file or directory
--
Best Regards
Masahiro Yamada
Re: [PATCH v4 6/8] KVM: PPC: Ultravisor: Restrict LDBAR access
On 7/1/19 3:46 AM, Ram Pai wrote: > On Mon, Jul 01, 2019 at 04:30:55PM +1000, Alexey Kardashevskiy wrote: >> >> On 01/07/2019 16:17, maddy wrote: >>> On 01/07/19 11:24 AM, Alexey Kardashevskiy wrote: On 29/06/2019 06:08, Claudio Carvalho wrote: > When the ultravisor firmware is available, it takes control over the > LDBAR register. In this case, thread-imc updates and save/restore > operations on the LDBAR register are handled by ultravisor. What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9 Processor" do not tell. >>> LDBAR is a per-thread SPR used by thread-imc pmu to dump the counter >>> data into memory. >>> LDBAR contains memory address along with few other configuration bits >>> (it is populated >>> by the thread-imc pmu driver). It is populated and enabled only when any >>> of the thread >>> imc pmu events are monitored. >> >> I was actually looking for a spec for this register, what is the >> document name? > Its not a architected register. Its documented in the Power9 > workbook. I also found some information about the LDBAR in arch/powerpc/perf/imc-pmu.c Claudio > > RP >
Re: [PATCH v4 3/8] KVM: PPC: Ultravisor: Add generic ultravisor call handler
On 7/11/19 9:57 AM, Michael Ellerman wrote: > Claudio Carvalho writes: >> From: Ram Pai >> >> Add the ucall() function, which can be used to make ultravisor calls >> with varied number of in and out arguments. Ultravisor calls can be made >> from the host or guests. >> >> This copies the implementation of plpar_hcall(). > .. with quite a few changes? > > This is one of the things I'd like to see in a Documentation file, so > that people can review the implementation vs the specification. I will document this (and other things) in a file under Documentation/powerpc. > >> Signed-off-by: Ram Pai >> [ Change ucall.S to not save CR, rename and move headers, build ucall.S >> if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some >> comments in the code ] > Why are we not saving CR? See previous comment about Documentation :) > >> Signed-off-by: Claudio Carvalho >> --- >> arch/powerpc/include/asm/ultravisor-api.h | 20 +++ >> arch/powerpc/include/asm/ultravisor.h | 20 +++ >> arch/powerpc/kernel/Makefile | 2 +- >> arch/powerpc/kernel/ucall.S | 30 +++ >> arch/powerpc/kernel/ultravisor.c | 4 +++ >> 5 files changed, 75 insertions(+), 1 deletion(-) >> create mode 100644 arch/powerpc/include/asm/ultravisor-api.h >> create mode 100644 arch/powerpc/kernel/ucall.S >> >> diff --git a/arch/powerpc/include/asm/ultravisor-api.h >> b/arch/powerpc/include/asm/ultravisor-api.h >> new file mode 100644 >> index ..49e766adabc7 >> --- /dev/null >> +++ b/arch/powerpc/include/asm/ultravisor-api.h >> @@ -0,0 +1,20 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Ultravisor API. >> + * >> + * Copyright 2019, IBM Corporation. >> + * >> + */ >> +#ifndef _ASM_POWERPC_ULTRAVISOR_API_H >> +#define _ASM_POWERPC_ULTRAVISOR_API_H >> + >> +#include >> + >> +/* Return codes */ >> +#define U_NOT_AVAILABLE H_NOT_AVAILABLE >> +#define U_SUCCESS H_SUCCESS >> +#define U_FUNCTION H_FUNCTION >> +#define U_PARAMETER H_PARAMETER > Is there any benefit in redefining these? > >> diff --git a/arch/powerpc/include/asm/ultravisor.h >> b/arch/powerpc/include/asm/ultravisor.h >> index e5009b0d84ea..a78a2dacfd0b 100644 >> --- a/arch/powerpc/include/asm/ultravisor.h >> +++ b/arch/powerpc/include/asm/ultravisor.h >> @@ -8,8 +8,28 @@ >> #ifndef _ASM_POWERPC_ULTRAVISOR_H >> #define _ASM_POWERPC_ULTRAVISOR_H >> >> +#include >> + >> +#if !defined(__ASSEMBLY__) > Just #ifndef is fine. > >> /* Internal functions */ > How is it internal? > >> extern int early_init_dt_scan_ultravisor(unsigned long node, const char >> *uname, >> int depth, void *data); >> >> +/* API functions */ >> +#define UCALL_BUFSIZE 4 > Please don't copy this design from the hcall code, it has led to bugs in > the past. > > See my (still unmerged) attempt to fix this for the hcall case: > https://patchwork.ozlabs.org/patch/683577/ > > Basically instead of asking callers nicely to define a certain sized > buffer, and them forgetting, define a proper type that has the right size. I will keep that in mind. For now I think we don't need that since the v5 will have ucall_norets() instead. > >> +/** >> + * ucall: Make a powerpc ultravisor call. >> + * @opcode: The ultravisor call to make. >> + * @retbuf: Buffer to store up to 4 return arguments in. >> + * >> + * This call supports up to 6 arguments and 4 return arguments. Use >> + * UCALL_BUFSIZE to size the return argument buffer. >> + */ >> +#if defined(CONFIG_PPC_POWERNV) > #ifdef > >> +long ucall(unsigned long opcode, unsigned long *retbuf, ...); >> +#endif >> + >> +#endif /* !__ASSEMBLY__ */ >> + >> #endif /* _ASM_POWERPC_ULTRAVISOR_H */ >> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile >> index f0caa302c8c0..f28baccc0a79 100644 >> --- a/arch/powerpc/kernel/Makefile >> +++ b/arch/powerpc/kernel/Makefile >> @@ -154,7 +154,7 @@ endif >> >> obj-$(CONFIG_EPAPR_PARAVIRT)+= epapr_paravirt.o epapr_hcalls.o >> obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o >> -obj-$(CONFIG_PPC_POWERNV) += ultravisor.o >> +obj-$(CONFIG_PPC_POWERNV) += ultravisor.o ucall.o > Same comment about being platforms/powernv ? >> diff --git a/arch/powerpc/kernel/ucall.S b/arch/powerpc/kernel/ucall.S >> new file mode 100644 >> index ..1678f6eb7230 >> --- /dev/null >> +++ b/arch/powerpc/kernel/ucall.S >> @@ -0,0 +1,30 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Generic code to perform an ultravisor call. >> + * >> + * Copyright 2019, IBM Corporation. >> + * >> + */ >> +#include >> + >> +/* >> + * This function is based on the plpar_hcall() > I don't think it meaningfully is any more. > >> + */ >> +_GLOBAL_TOC(ucall) > You don't need the TOC setup here (unless a later patch does?). > >> +std r4,STK_PARAM(R4)(r1)/* Save ret buffer */ >> +mr r4,r5 >> +
[PATCH V2] cpufreq: Make cpufreq_generic_init() return void
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.
change its return type to void and update all the callers.
Signed-off-by: Viresh Kumar
---
V1->V2:
- Fixed compilation warning with s3c64xx driver by replace %d with %ld
(Reported by buildbot).
drivers/cpufreq/bmips-cpufreq.c | 7 ++-
drivers/cpufreq/cpufreq.c | 4 +---
drivers/cpufreq/davinci-cpufreq.c | 3 ++-
drivers/cpufreq/imx6q-cpufreq.c | 6 ++
drivers/cpufreq/kirkwood-cpufreq.c | 3 ++-
drivers/cpufreq/loongson1-cpufreq.c | 8 +++-
drivers/cpufreq/loongson2_cpufreq.c | 3 ++-
drivers/cpufreq/maple-cpufreq.c | 3 ++-
drivers/cpufreq/omap-cpufreq.c | 15 +--
drivers/cpufreq/pasemi-cpufreq.c| 3 ++-
drivers/cpufreq/pmac32-cpufreq.c| 3 ++-
drivers/cpufreq/pmac64-cpufreq.c| 3 ++-
drivers/cpufreq/s3c2416-cpufreq.c | 9 ++---
drivers/cpufreq/s3c64xx-cpufreq.c | 15 +++
drivers/cpufreq/s5pv210-cpufreq.c | 3 ++-
drivers/cpufreq/sa1100-cpufreq.c| 3 ++-
drivers/cpufreq/sa1110-cpufreq.c| 3 ++-
drivers/cpufreq/spear-cpufreq.c | 3 ++-
drivers/cpufreq/tegra20-cpufreq.c | 8 +---
include/linux/cpufreq.h | 2 +-
20 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..2b187d802fe3 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -141,11 +141,8 @@ static int bmips_cpufreq_init(struct cpufreq_policy
*policy)
return ret;
}
- ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
- if (ret)
- bmips_cpufreq_exit(policy);
- else
- pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+ cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+ pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
return ret;
}
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
* - set policies transition latency
* - policy->cpus with all possible CPUs
*/
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
{
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
* share the clock and voltage and clock.
*/
cpumask_setall(policy->cpus);
-
- return 0;
}
EXPORT_SYMBOL_GPL(cpufreq_generic_init);
diff --git a/drivers/cpufreq/davinci-cpufreq.c
b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
* Setting the latency to 2000 us to accommodate addition of drivers
* to pre/post change notification list.
*/
- return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ return 0;
}
static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy
*policy, unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
- int ret;
-
policy->clk = clks[ARM].clk;
- ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+ cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
- return ret;
+ return 0;
}
static struct cpufreq_driver imx6q_cpufreq_driver = {
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c
b/drivers/cpufreq/kirkwood-cpufreq.c
index 7ab564c1f7ae..cb74bdc5baaa 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -85,7 +85,8 @@ static int kirkwood_cpufreq_target(struct cpufreq_policy
*policy,
/* Module init and exit code */
static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ return 0;
}
static struct cpufreq_driver kirkwood_cpufreq_driver = {
diff --git a/drivers/cpufreq/loongson1-cpufreq.c
b/drivers/cpufreq/loongson1-cpufreq.c
index 21c9ce8526c0..0ea88778882a 100644
---
Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
On Sat, Jul 13, 2019 at 07:47:44AM -0500, Segher Boessenkool wrote:
> On Sat, Jul 13, 2019 at 12:21:06PM +0900, Masahiro Yamada wrote:
> > The KBUILD_ARFLAGS addition in arch/powerpc/Makefile has never worked
> > in a useful way because it is always overridden by the following code
> > in the top Makefile:
> >
> > # use the deterministic mode of AR if available
> > KBUILD_ARFLAGS := $(call ar-option,D)
> >
> > The code in the top Makefile was added in 2011, by commit 40df759e2b9e
> > ("kbuild: Fix build with binutils <= 2.19").
> >
> > The KBUILD_ARFLAGS addition for ppc has always been dead code from the
> > beginning.
>
> That was added in 43c9127d94d6 to replace my 8995ac870273 from 2007.
>
> Is it no longer supported to build a 64-bit kernel with a toolchain
> that defaults to 32-bit, or the other way around? And with non-native
> toolchains (this one didn't run on Linux, even).
It was an --enable-targets=all toolchain, somewhat common for crosses,
if that matters.
Segher
Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
On Sat, Jul 13, 2019 at 12:21:06PM +0900, Masahiro Yamada wrote:
> The KBUILD_ARFLAGS addition in arch/powerpc/Makefile has never worked
> in a useful way because it is always overridden by the following code
> in the top Makefile:
>
> # use the deterministic mode of AR if available
> KBUILD_ARFLAGS := $(call ar-option,D)
>
> The code in the top Makefile was added in 2011, by commit 40df759e2b9e
> ("kbuild: Fix build with binutils <= 2.19").
>
> The KBUILD_ARFLAGS addition for ppc has always been dead code from the
> beginning.
That was added in 43c9127d94d6 to replace my 8995ac870273 from 2007.
Is it no longer supported to build a 64-bit kernel with a toolchain
that defaults to 32-bit, or the other way around? And with non-native
toolchains (this one didn't run on Linux, even).
Segher
Re: [RFC PATCH kernel] powerpc/xive: Drop deregistered irqs
On 13/07/2019 09:47, Benjamin Herrenschmidt wrote: > On Fri, 2019-07-12 at 19:37 +1000, Alexey Kardashevskiy wrote: >> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/irq.c#n614 >> >> If so, then in order to do EOI, I'll need the desc which is gone, or >> I am missing the point? > > All you need is drop the local CPU priority. I know that, the question was how. I cannot use irq_chip in arch/powerpc/kernel/irq.c and I do not want to add ppc_md.enable_irqs(). -- Alexey
Re: [PATCH v4 2/8] powerpc: Introduce FW_FEATURE_ULTRAVISOR
On 7/11/19 9:57 AM, Michael Ellerman wrote:
> Claudio Carvalho writes:
>> diff --git a/arch/powerpc/include/asm/ultravisor.h
>> b/arch/powerpc/include/asm/ultravisor.h
>> new file mode 100644
>> index ..e5009b0d84ea
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/ultravisor.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Ultravisor definitions
>> + *
>> + * Copyright 2019, IBM Corporation.
>> + *
>> + */
>> +#ifndef _ASM_POWERPC_ULTRAVISOR_H
>> +#define _ASM_POWERPC_ULTRAVISOR_H
>> +
>> +/* Internal functions */
>> +extern int early_init_dt_scan_ultravisor(unsigned long node, const char
>> *uname,
>> + int depth, void *data);
> Please don't use extern in new headers.
>
>> diff --git a/arch/powerpc/kernel/ultravisor.c
>> b/arch/powerpc/kernel/ultravisor.c
>> new file mode 100644
>> index ..dc6021f63c97
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ultravisor.c
> Is there a reason this (and other later files) aren't in platforms/powernv ?
Yes, there is.
https://www.spinics.net/lists/kvm-ppc/msg14998.html
We also need to do ucalls from a secure guest and its kernel may not have
CONFIG_PPC_POWERNV=y. I can make it clear in the commit message.
>
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Ultravisor high level interfaces
>> + *
>> + * Copyright 2019, IBM Corporation.
>> + *
>> + */
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +
>> +int __init early_init_dt_scan_ultravisor(unsigned long node, const char
>> *uname,
>> + int depth, void *data)
>> +{
>> +if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)
>> +return 0;
> I know you're following the example of OPAL, but this is not the best
> way to search for the ultravisor node.
>
> It makes the location and name of the node part of the ABI, when there's
> no need for it to be.
>
> If instead you just scan the tree for a node that is *compatible* with
> "ibm,ultravisor" (or whatever compatible string) then the node can be
> placed any where in the tree and have any name, which gives us the most
> flexibility in future to change the location of the device tree node.
I will do that in the next version.
Thanks,
Claudio
>
> cheers
>
Re: [PATCH 3/3] fs/core/vmcore: Move sev_active() reference to x86 arch code
On Fri, 12 Jul 2019 17:11:29 +0200 Christoph Hellwig wrote: > On Fri, Jul 12, 2019 at 04:51:53PM +0200, Halil Pasic wrote: > > Thank you very much! I will have another look, but it seems to me, > > without further measures taken, this would break protected virtualization > > support on s390. The effect of the che for s390 is that > > force_dma_unencrypted() will always return false instead calling into > > the platform code like it did before the patch, right? > > > > Should I send a Fixes: e67a5ed1f86f "dma-direct: Force unencrypted DMA > > under SME for certain DMA masks" (Tom Lendacky, 2019-07-10) patch that > > rectifies things for s390 or how do we want handle this? > > Yes, please do. I hadn't noticed the s390 support had landed in > mainline already. > Will do! I guess I should do the patch against the for-next branch of the dma-mapping tree. But that branch does not have the s390 support patches (yet?). To fix it I need both e67a5ed1f86f and 64e1f0c531d1 "s390/mm: force swiotlb for protected virtualization" (Halil Pasic, 2018-09-13). Or should I wait for e67a5ed1f86f landing in mainline? Regards, Halil
Re: [PATCH 3/3] fs/core/vmcore: Move sev_active() reference to x86 arch code
On Fri, 12 Jul 2019 16:08:12 +0200
Christoph Hellwig wrote:
> On Fri, Jul 12, 2019 at 03:09:12PM +0200, Halil Pasic wrote:
> > This is the implementation for the guys that don't
> > have ARCH_HAS_MEM_ENCRYPT.
> >
> > Means sev_active() may not be used in such code after this
> > patch. What about
> >
> > static inline bool force_dma_unencrypted(void)
> > {
> > return sev_active();
> > }
> >
> > in kernel/dma/direct.c?
>
> FYI, I have this pending in the dma-mapping tree:
>
> http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/e67a5ed1f86f4370991c601f2fcad9ebf9e1eebb
Thank you very much! I will have another look, but it seems to me,
without further measures taken, this would break protected virtualization
support on s390. The effect of the che for s390 is that
force_dma_unencrypted() will always return false instead calling into
the platform code like it did before the patch, right?
Should I send a Fixes: e67a5ed1f86f "dma-direct: Force unencrypted DMA
under SME for certain DMA masks" (Tom Lendacky, 2019-07-10) patch that
rectifies things for s390 or how do we want handle this?
Regards,
Halil
Re: [PATCH 3/3] fs/core/vmcore: Move sev_active() reference to x86 arch code
On Fri, 12 Jul 2019 02:36:31 -0300
Thiago Jung Bauermann wrote:
> Secure Encrypted Virtualization is an x86-specific feature, so it shouldn't
> appear in generic kernel code because it forces non-x86 architectures to
> define the sev_active() function, which doesn't make a lot of sense.
sev_active() might be just bad (too specific) name for a general
concept. s390 code defines it drives the right behavior in
kernel/dma/direct.c (which uses it).
>
> To solve this problem, add an x86 elfcorehdr_read() function to override
> the generic weak implementation. To do that, it's necessary to make
> read_from_oldmem() public so that it can be used outside of vmcore.c.
>
> Signed-off-by: Thiago Jung Bauermann
> ---
> arch/x86/kernel/crash_dump_64.c | 5 +
> fs/proc/vmcore.c| 8
> include/linux/crash_dump.h | 14 ++
> include/linux/mem_encrypt.h | 1 -
> 4 files changed, 23 insertions(+), 5 deletions(-)
Does not seem to apply to today's or yesterdays master.
>
> diff --git a/arch/x86/kernel/crash_dump_64.c b/arch/x86/kernel/crash_dump_64.c
> index 22369dd5de3b..045e82e8945b 100644
> --- a/arch/x86/kernel/crash_dump_64.c
> +++ b/arch/x86/kernel/crash_dump_64.c
> @@ -70,3 +70,8 @@ ssize_t copy_oldmem_page_encrypted(unsigned long pfn, char
> *buf, size_t csize,
> {
> return __copy_oldmem_page(pfn, buf, csize, offset, userbuf, true);
> }
> +
> +ssize_t elfcorehdr_read(char *buf, size_t count, u64 *ppos)
> +{
> + return read_from_oldmem(buf, count, ppos, 0, sev_active());
> +}
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 57957c91c6df..ca1f20bedd8c 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -100,9 +100,9 @@ static int pfn_is_ram(unsigned long pfn)
> }
>
> /* Reads a page from the oldmem device from given offset. */
> -static ssize_t read_from_oldmem(char *buf, size_t count,
> - u64 *ppos, int userbuf,
> - bool encrypted)
> +ssize_t read_from_oldmem(char *buf, size_t count,
> + u64 *ppos, int userbuf,
> + bool encrypted)
> {
> unsigned long pfn, offset;
> size_t nr_bytes;
> @@ -166,7 +166,7 @@ void __weak elfcorehdr_free(unsigned long long addr)
> */
> ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
> {
> - return read_from_oldmem(buf, count, ppos, 0, sev_active());
> + return read_from_oldmem(buf, count, ppos, 0, false);
> }
>
> /*
> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
> index f774c5eb9e3c..4664fc1871de 100644
> --- a/include/linux/crash_dump.h
> +++ b/include/linux/crash_dump.h
> @@ -115,4 +115,18 @@ static inline int vmcore_add_device_dump(struct
> vmcoredd_data *data)
> return -EOPNOTSUPP;
> }
> #endif /* CONFIG_PROC_VMCORE_DEVICE_DUMP */
> +
> +#ifdef CONFIG_PROC_VMCORE
> +ssize_t read_from_oldmem(char *buf, size_t count,
> + u64 *ppos, int userbuf,
> + bool encrypted);
> +#else
> +static inline ssize_t read_from_oldmem(char *buf, size_t count,
> +u64 *ppos, int userbuf,
> +bool encrypted)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif /* CONFIG_PROC_VMCORE */
> +
> #endif /* LINUX_CRASHDUMP_H */
> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h
> index f2e399fb626b..a3747fcae466 100644
> --- a/include/linux/mem_encrypt.h
> +++ b/include/linux/mem_encrypt.h
> @@ -21,7 +21,6 @@
>
> #else/* !CONFIG_ARCH_HAS_MEM_ENCRYPT */
>
> -static inline bool sev_active(void) { return false; }
This is the implementation for the guys that don't
have ARCH_HAS_MEM_ENCRYPT.
Means sev_active() may not be used in such code after this
patch. What about
static inline bool force_dma_unencrypted(void)
{
return sev_active();
}
in kernel/dma/direct.c?
Regards,
Halil
> static inline bool mem_encrypt_active(void) { return false; }
>
> #endif /* CONFIG_ARCH_HAS_MEM_ENCRYPT */
Re: [PATCH 3/3] fs/core/vmcore: Move sev_active() reference to x86 arch code
On Fri, Jul 12, 2019 at 05:42:49PM +0200, Halil Pasic wrote: > > Will do! I guess I should do the patch against the for-next branch of the > dma-mapping tree. But that branch does not have the s390 support patches > (yet?). > To fix it I need both e67a5ed1f86f and 64e1f0c531d1 "s390/mm: force > swiotlb for protected virtualization" (Halil Pasic, 2018-09-13). Or > should I wait for e67a5ed1f86f landing in mainline? I've rebased the dma-mapping for-next branch to latest mainline as of today that has both commits.
Re: [PATCH 2/3] DMA mapping: Move SME handling to x86-specific files
While this looks generally good to me, I think we want to split this into three patches: 1) update the swiotlb printk 2) removing the dma-mapping check and printk 3) clean up the mem_encrypt.h interface.
Re: [PATCH 1/3] x86,s390: Move ARCH_HAS_MEM_ENCRYPT definition to arch/Kconfig
On Sat, Jul 13, 2019 at 01:45:52AM -0300, Thiago Jung Bauermann wrote: > powerpc is also going to use this feature, so put it in a generic location. Looks good, even without a third arch using it we should never habe symbols defined under arch/$(ARCH) that are used in common code to start with. Reviewed-by: Christoph Hellwig
