* [PATCH 2/2] powerpc/rtas: allow rescheduling while changing cpu states
From: Nathan Lynch @ 2019-07-18 19:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20190718192940.16103-1-nathanl@linux.ibm.com>
rtas_cpu_state_change_mask() potentially operates on scores of cpus,
so explicitly allow rescheduling in the loop body.
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
arch/powerpc/kernel/rtas.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index fbefd9ff6dab..396fb2f35c01 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -20,6 +20,7 @@
#include <linux/capability.h>
#include <linux/delay.h>
#include <linux/cpu.h>
+#include <linux/sched.h>
#include <linux/smp.h>
#include <linux/completion.h>
#include <linux/cpumask.h>
@@ -902,6 +903,7 @@ static int rtas_cpu_state_change_mask(enum rtas_cpu_state state,
cpumask_clear_cpu(cpu, cpus);
}
}
+ cond_resched();
}
return ret;
--
2.20.1
^ permalink raw reply related
* [PATCH 1/2] powerpc/rtas: use device model APIs and serialization during LPM
From: Nathan Lynch @ 2019-07-18 19:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20190718192940.16103-1-nathanl@linux.ibm.com>
During LPAR migration, cpu hotplug and migration operations can
interleave like so:
cd /sys/devices/system/cpu/cpu7/ | drmgr -m -c pmig -p pre \
echo 0 > online | -s 0xd7a884f83d830e6d -t 19 \
echo 1 > online | -n -d 1 5
---------------------------------+-------------------------------------------
online_store() { |
device_offline() { |
cpu_subsys_offline() { |
cpu_down(7); |
} |
dev->offline = true; |
} | migration_store() {
} | rtas_ibm_suspend_me() {
| rtas_online_cpus_mask() {
| cpu_up(7);
| }
| cpu_hotplug_disable();
| on_each_cpu(rtas_percpu_suspend_me());
| cpu_hotplug_enable();
online_store() { |
device_online() { |
cpu_subsys_online() { |
cpu_up(7); |
} |
dev->offline = false; |
} | rtas_offline_cpus_mask() {
} | rtas_cpu_state_change_mask() {
| cpu_down(7);
| }
| }
| }
| }
This leaves cpu7 in a state where the driver core considers the cpu
device online, but in all other respects it is offline and
unused. Attempts to online the cpu via sysfs appear to succeed but the
driver core actually does not pass the request to the lower-level
cpuhp support code. This makes the cpu unusable until the system is
rebooted.
Instead of directly calling cpu_up/cpu_down, the migration code should
use the higher-level device core APIs to maintain consistent state and
serialize operations.
Fixes: 120496ac2d2d ("powerpc: Bring all threads online prior to migration/hibernation")
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
arch/powerpc/kernel/rtas.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 9b4d2a2ffb4f..fbefd9ff6dab 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -875,15 +875,17 @@ static int rtas_cpu_state_change_mask(enum rtas_cpu_state state,
return 0;
for_each_cpu(cpu, cpus) {
+ struct device *dev = get_cpu_device(cpu);
+
switch (state) {
case DOWN:
- cpuret = cpu_down(cpu);
+ cpuret = device_offline(dev);
break;
case UP:
- cpuret = cpu_up(cpu);
+ cpuret = device_online(dev);
break;
}
- if (cpuret) {
+ if (cpuret < 0) {
pr_debug("%s: cpu_%s for cpu#%d returned %d.\n",
__func__,
((state == UP) ? "up" : "down"),
@@ -972,6 +974,8 @@ int rtas_ibm_suspend_me(u64 handle)
data.token = rtas_token("ibm,suspend-me");
data.complete = &done;
+ lock_device_hotplug();
+
/* All present CPUs must be online */
cpumask_andnot(offline_mask, cpu_present_mask, cpu_online_mask);
cpuret = rtas_online_cpus_mask(offline_mask);
@@ -1011,6 +1015,7 @@ int rtas_ibm_suspend_me(u64 handle)
__func__);
out:
+ unlock_device_hotplug();
free_cpumask_var(offline_mask);
return atomic_read(&data.error);
}
--
2.20.1
^ permalink raw reply related
* [PATCH] scsi: ibmvscsi: remove casting dma_alloc_coherent
From: Vasyl Gomonovych @ 2019-07-18 19:31 UTC (permalink / raw)
To: tyreld, benh, paulus, mpe, jejb, martin.petersen, linux-scsi,
linuxppc-dev
Cc: Vasyl Gomonovych, linux-kernel
Fix allocation style
Generated by: alloc_cast.cocci
Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 7f66a7783209..7e9b3e409851 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -715,8 +715,7 @@ static int map_sg_data(struct scsi_cmnd *cmd,
/* get indirect table */
if (!evt_struct->ext_list) {
- evt_struct->ext_list = (struct srp_direct_buf *)
- dma_alloc_coherent(dev,
+ evt_struct->ext_list = dma_alloc_coherent(dev,
SG_ALL * sizeof(struct srp_direct_buf),
&evt_struct->ext_list_token, 0);
if (!evt_struct->ext_list) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 0/6] Remove x86-specific code from generic headers
From: Thiago Jung Bauermann @ 2019-07-18 19:44 UTC (permalink / raw)
To: Lendacky, Thomas
Cc: linux-s390@vger.kernel.org, Mike Anderson, Konrad Rzeszutek Wilk,
Robin Murphy, x86@kernel.org, Ram Pai,
linux-kernel@vger.kernel.org, Alexey Dobriyan, Halil Pasic,
iommu@lists.linux-foundation.org, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, linux-fsdevel@vger.kernel.org, Thomas Gleixner,
linuxppc-dev@lists.ozlabs.org, Christoph Hellwig,
Marek Szyprowski
In-Reply-To: <680bb92e-66eb-8959-88a5-3447a6a282c8@amd.com>
Lendacky, Thomas <Thomas.Lendacky@amd.com> writes:
> On 7/17/19 10:28 PM, Thiago Jung Bauermann wrote:
>> Hello,
>>
>> This version is mostly about splitting up patch 2/3 into three separate
>> patches, as suggested by Christoph Hellwig. Two other changes are a fix in
>> patch 1 which wasn't selecting ARCH_HAS_MEM_ENCRYPT for s390 spotted by
>> Janani and removal of sme_active and sev_active symbol exports as suggested
>> by Christoph Hellwig.
>>
>> These patches are applied on top of today's dma-mapping/for-next.
>>
>> I don't have a way to test SME, SEV, nor s390's PEF so the patches have only
>> been build tested.
>
> I'll try and get this tested quickly to be sure everything works for SME
> and SEV.
Thanks! And thanks for reviewing the patches.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
From: Shawn Anastasio @ 2019-07-18 19:46 UTC (permalink / raw)
To: Christoph Hellwig, Oliver O'Halloran
Cc: Alexey Kardashevskiy, Sam Bobroff, iommu, linuxppc-dev,
Marek Szyprowski
In-Reply-To: <20190718095200.GA25744@lst.de>
On 7/18/19 4:52 AM, Christoph Hellwig wrote:
> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote:
>> On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote:
>>>> Other than m68k, mips, and arm64, everybody else that doesn't have
>>>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so
>>>> I assume this behavior is acceptable on those architectures.
>>>
>>> It might be acceptable, but there's no reason to use pgport_noncached
>>> if the platform supports cache-coherent DMA.
>>>
>>> Christoph (+cc) made the change so maybe he saw something we're missing.
>>
>> I always found the forcing of noncached access even for coherent
>> devices a little odd, but this was inherited from the previous
>> implementation, which surprised me a bit as the different attributes
>> are usually problematic even on x86. Let me dig into the history a
>> bit more, but I suspect the righ fix is to default to cached mappings
>> for coherent devices.
>
> Ok, some history:
>
> The generic dma mmap implementation, which we are effectively still
> using today was added by:
>
> commit 64ccc9c033c6089b2d426dad3c56477ab066c999
> Author: Marek Szyprowski <m.szyprowski@samsung.com>
> Date: Thu Jun 14 13:03:04 2012 +0200
>
> common: dma-mapping: add support for generic dma_mmap_* calls
>
> and unconditionally uses pgprot_noncached in dma_common_mmap, which is
> then used as the fallback by dma_mmap_attrs if no ->mmap method is
> present. At that point we already had the powerpc implementation
> that only uses pgprot_noncached for non-coherent mappings, and
> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE
> is set and otherwise pgprot_dmacoherent, which seems to be uncached.
> Arm did support coherent platforms at that time, but they might have
> been an afterthought and not handled properly.
>
> So it migt have been that we were all wrong for that time and might
> have to fix it up.
Personally, I'm not a huge fan of an implicit default for something
inherently architecture-dependent like this at all. What I'd like to
see is a mechanism that forces architecture code to explicitly
opt in to the default pgprot settings if they don't provide an
implementation of arch_dma_mmap_pgprot. This could perhaps be done
by reversing ARCH_HAS_DMA_MMAP_PGPROT to something like
ARCH_USE_DEFAULT_DMA_MMAP_PGPROT.
This way as more systems are moved to use the common mmap code instead
of their ops->mmap, the people doing the refactoring have to make an
explicit decision about the pgprot settings to use. Such a configuration
would have likely prevented this situation with powerpc from happening.
That being said, if the default behavior doesn't make sense in the
general case it should probably be fixed as well.
Curious to hear some thoughts on this.
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/rtas: use device model APIs and serialization during LPM
From: Nathan Lynch @ 2019-07-18 19:46 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20190718192940.16103-2-nathanl@linux.ibm.com>
Nathan Lynch <nathanl@linux.ibm.com> writes:
> During LPAR migration, cpu hotplug and migration operations can
> interleave like so:
>
> cd /sys/devices/system/cpu/cpu7/ | drmgr -m -c pmig -p pre \
> echo 0 > online | -s 0xd7a884f83d830e6d -t 19 \
> echo 1 > online | -n -d 1 5
> ---------------------------------+-------------------------------------------
> online_store() { |
> device_offline() { |
> cpu_subsys_offline() { |
> cpu_down(7); |
> } |
> dev->offline = true; |
> } | migration_store() {
> } | rtas_ibm_suspend_me() {
> | rtas_online_cpus_mask() {
> | cpu_up(7);
> | }
> | cpu_hotplug_disable();
> | on_each_cpu(rtas_percpu_suspend_me());
> | cpu_hotplug_enable();
> online_store() { |
> device_online() { |
> cpu_subsys_online() { |
> cpu_up(7); |
> } |
> dev->offline = false; |
> } | rtas_offline_cpus_mask() {
> } | rtas_cpu_state_change_mask() {
> | cpu_down(7);
> | }
> | }
> | }
> | }
Actually I think this isn't a correct depiction of the race. I'll
rewrite and resend.
^ permalink raw reply
* Re: [PATCH 2/3] DMA mapping: Move SME handling to x86-specific files
From: Thiago Jung Bauermann @ 2019-07-18 19:47 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-s390, Mike Anderson, Konrad Rzeszutek Wilk, Robin Murphy,
x86, Ram Pai, linux-kernel, Alexey Dobriyan, Halil Pasic, iommu,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-fsdevel,
linuxppc-dev, Christoph Hellwig, Marek Szyprowski
In-Reply-To: <alpine.DEB.2.21.1907121806160.1788@nanos.tec.linutronix.de>
Thomas Gleixner <tglx@linutronix.de> writes:
> On Fri, 12 Jul 2019, Thiago Jung Bauermann wrote:
>> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h
>> index b310a9c18113..f2e399fb626b 100644
>> --- a/include/linux/mem_encrypt.h
>> +++ b/include/linux/mem_encrypt.h
>> @@ -21,23 +21,11 @@
>>
>> #else /* !CONFIG_ARCH_HAS_MEM_ENCRYPT */
>>
>> -#define sme_me_mask 0ULL
>> -
>> -static inline bool sme_active(void) { return false; }
>> static inline bool sev_active(void) { return false; }
>
> You want to move out sev_active as well, the only relevant thing is
> mem_encrypt_active(). Everything SME/SEV is an architecture detail.
I'm sure you saw it. I addressed sev_active in a separate patch.
Thanks for reviewing this series!
>> +static inline bool mem_encrypt_active(void) { return false; }
>
> Thanks,
>
> tglx
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
From: Segher Boessenkool @ 2019-07-18 19:58 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anshuman Khandual, Mike Anderson, Ram Pai, linux-kernel,
Claudio Carvalho, Paul Mackerras, linuxppc-dev, Christoph Hellwig,
Thiago Jung Bauermann
In-Reply-To: <70f8097f-7222-fe18-78b4-9372c21bfc9d@ozlabs.ru>
(Sorry to hijack your reply).
On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote:
> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
> >From: Ram Pai <linuxram@us.ibm.com>
> >+static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
> >+{
> >+ register uint64_t func asm("r3") = UV_ESM;
> >+ register uint64_t arg1 asm("r4") = (uint64_t)kbase;
> >+ register uint64_t arg2 asm("r5") = (uint64_t)fdt;
>
> What does UV do with kbase and fdt precisely? Few words in the commit
> log will do.
>
> >+
> >+ asm volatile("sc 2\n"
> >+ : "=r"(func)
> >+ : "0"(func), "r"(arg1), "r"(arg2)
> >+ :);
> >+
> >+ return (int)func;
>
> And why "func"? Is it "function"? Weird name. Thanks,
Maybe the three vars should just be called "r3", "r4", and "r5" --
r3 is used as return value as well, so "func" isn't a great name for it.
Some other comments about this inline asm:
The "\n" makes the generated asm look funny and has no other function.
Instead of using backreferences you can use a "+" constraint, "inout".
Empty clobber list is strange.
Casts to the return type, like most other casts, are an invitation to
bugs and not actually useful.
So this can be written
static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
{
register uint64_t r3 asm("r3") = UV_ESM;
register uint64_t r4 asm("r4") = kbase;
register uint64_t r4 asm("r5") = fdt;
asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5));
return r3;
}
(and it probably should use u64 instead of both uint64_t and unsigned long?)
Segher
^ permalink raw reply
* Re: [PATCH v2 04/13] powerpc/pseries/svm: Add helpers for UV_SHARE_PAGE and UV_UNSHARE_PAGE
From: Thiago Jung Bauermann @ 2019-07-18 20:12 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anshuman Khandual, Mike Anderson, Ram Pai, linux-kernel,
Claudio Carvalho, Paul Mackerras, linuxppc-dev, Christoph Hellwig
In-Reply-To: <4fcc84ae-b93a-b5f1-fba4-b0e2af7b727c@ozlabs.ru>
Hello Alexey,
Thanks for your review!
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
>> From: Ram Pai <linuxram@us.ibm.com>
>>
>> These functions are used when the guest wants to grant the hypervisor
>> access to certain pages.
>>
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/ultravisor-api.h | 2 ++
>> arch/powerpc/include/asm/ultravisor.h | 15 +++++++++++++++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h
>> index fe9a0d8d7673..c7513bbadf57 100644
>> --- a/arch/powerpc/include/asm/ultravisor-api.h
>> +++ b/arch/powerpc/include/asm/ultravisor-api.h
>> @@ -25,6 +25,8 @@
>> #define UV_UNREGISTER_MEM_SLOT 0xF124
>> #define UV_PAGE_IN 0xF128
>> #define UV_PAGE_OUT 0xF12C
>> +#define UV_SHARE_PAGE 0xF130
>> +#define UV_UNSHARE_PAGE 0xF134
>> #define UV_PAGE_INVAL 0xF138
>> #define UV_SVM_TERMINATE 0xF13C
>> diff --git a/arch/powerpc/include/asm/ultravisor.h
>> b/arch/powerpc/include/asm/ultravisor.h
>> index f5dc5af739b8..f7418b663a0e 100644
>> --- a/arch/powerpc/include/asm/ultravisor.h
>> +++ b/arch/powerpc/include/asm/ultravisor.h
>> @@ -91,6 +91,21 @@ static inline int uv_svm_terminate(u64 lpid)
>> return ucall(UV_SVM_TERMINATE, retbuf, lpid);
>> }
>> +
>> +static inline int uv_share_page(u64 pfn, u64 npages)
>> +{
>> + unsigned long retbuf[UCALL_BUFSIZE];
>> +
>> + return ucall(UV_SHARE_PAGE, retbuf, pfn, npages);
>
>
> What is in that retbuf? Can you pass NULL instead?
I think so, that buffer isn't used actually. Claudio is working on a
ucall_norets() which doesn't take the buffer and I can switch to that.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
From: Segher Boessenkool @ 2019-07-18 20:46 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Stephen Rothwell, Linux Kernel Mailing List, Nicholas Piggin,
Paul Mackerras, linuxppc-dev
In-Reply-To: <CAK7LNAR7jkq1fAi_=xgsANCkgP2AAej9Yv7RZB3B_cpD7C_71Q@mail.gmail.com>
Hi!
On Thu, Jul 18, 2019 at 11:19:58AM +0900, Masahiro Yamada wrote:
> On Thu, Jul 18, 2019 at 1:46 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> Kbuild always uses thin archives as far as vmlinux is concerned.
>
> But, there are some other call-sites.
>
> masahiro@pug:~/ref/linux$ git grep '$(AR)' -- :^Documentation :^tools
> arch/powerpc/boot/Makefile: BOOTAR := $(AR)
> arch/unicore32/lib/Makefile: $(Q)$(AR) p $(GNU_LIBC_A) $(notdir $@) > $@
> arch/unicore32/lib/Makefile: $(Q)$(AR) p $(GNU_LIBGCC_A) $(notdir $@) > $@
> lib/raid6/test/Makefile: $(AR) cq $@ $^
> scripts/Kbuild.include:ar-option = $(call try-run, $(AR) rc$(1)
> "$$TMP",$(1),$(2))
> scripts/Makefile.build: cmd_ar_builtin = rm -f $@; $(AR)
> rcSTP$(KBUILD_ARFLAGS) $@ $(real-prereqs)
> scripts/Makefile.lib: cmd_ar = rm -f $@; $(AR)
> rcsTP$(KBUILD_ARFLAGS) $@ $(real-prereqs)
>
> Probably, you are interested in arch/powerpc/boot/Makefile.
That one seems fine actually. The raid6 one I don't know.
My original commit message was
Without this, some versions of GNU ar fail to create
an archive index if the object files it is packing
together are of a different object format than ar's
default format (for example, binutils compiled to
default to 64-bit, with 32-bit objects).
but I cannot reproduce the problem anymore. Shortly after my patch the
thin archive code happened to binutils, and that overhauled some other
things, which might have fixed it already?
> > Yes, I know. This isn't about built-in.[oa], it is about *other*
> > archives we at least *used to* create. If we *know* we do not anymore,
> > then this workaround can of course be removed (and good riddance).
>
> If it is not about built-in.[oa],
> which archive are you talking about?
>
> Can you pin-point the one?
No, not anymore. Lost in the mists of time, I guess? I think we'll
just have to file it as "it seems to work fine now".
Thank you (and everyone else) for the time looking at this!
Segher
^ permalink raw reply
* Re: [PATCH v4 4/8] KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a PATE
From: Claudio Carvalho @ 2019-07-18 21:25 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: Ryan Grimm, Madhavan Srinivasan, Michael Anderson, Ram Pai,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <87ims8g24r.fsf@concordia.ellerman.id.au>
On 7/11/19 9:57 AM, Michael Ellerman wrote:
>
>>
>> static pmd_t *get_pmd_from_cache(struct mm_struct *mm)
>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> index 8904aa1243d8..da6a6b76a040 100644
>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -656,8 +656,10 @@ void radix__early_init_mmu_secondary(void)
>> lpcr = mfspr(SPRN_LPCR);
>> mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>>
>> - mtspr(SPRN_PTCR,
>> - __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
>> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> + mtspr(SPRN_PTCR, __pa(partition_tb) |
>> + (PATB_SIZE_SHIFT - 12));
>> +
>> radix_init_amor();
>> }
>>
>> @@ -673,7 +675,8 @@ void radix__mmu_cleanup_all(void)
>> if (!firmware_has_feature(FW_FEATURE_LPAR)) {
>> lpcr = mfspr(SPRN_LPCR);
>> mtspr(SPRN_LPCR, lpcr & ~LPCR_UPRT);
>> - mtspr(SPRN_PTCR, 0);
>> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> + mtspr(SPRN_PTCR, 0);
>> powernv_set_nmmu_ptcr(0);
>> radix__flush_tlb_all();
>> }
> There's four of these case where we skip touching the PTCR, which is
> right on the borderline of warranting an accessor. I guess we can do it
> as a cleanup later.
I agree.
Since the kernel doesn't need to access a big number of ultravisor
privileged registers, maybe we can define mtspr_<reg> and mfspr_<reg>
inline functions that in ultravisor.h that skip touching the register if an
ultravisor is present and and the register is ultravisor privileged. Thus,
we don't need to replicate comments and that also would make it easier for
developers to know what are the ultravisor privileged registers.
Something like this:
--- a/arch/powerpc/include/asm/ultravisor.h
+++ b/arch/powerpc/include/asm/ultravisor.h
@@ -10,10 +10,21 @@
#include <asm/ultravisor-api.h>
#include <asm/asm-prototypes.h>
+#include <asm/reg.h>
int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data);
+static inline void mtspr_ptcr(unsigned long val)
+{
+ /*
+ * If the ultravisor firmware is present, it maintains the partition
+ * table. PTCR becomes ultravisor privileged only for writing.
+ */
+ if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ mtspr(SPRN_PTCR, val);
+}
+
static inline int uv_register_pate(u64 lpid, u64 dw0, u64 dw1)
{
return ucall_norets(UV_WRITE_PATE, lpid, dw0, dw1);
diff --git a/arch/powerpc/mm/book3s64/pgtable.c
b/arch/powerpc/mm/book3s64/pgtable.c
index e1bbc48e730f..25156f9dfde8 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -220,7 +220,7 @@ void __init mmu_partition_table_init(void)
* 64 K size.
*/
ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
- mtspr(SPRN_PTCR, ptcr);
+ mtspr_ptcr(ptcr);
powernv_set_nmmu_ptcr(ptcr);
}
What do you think?
An alternative could be to change the mtspr() and mfspr() macros as we
proposed in the v1, but access to non-ultravisor privileged registers would
be performance impacted because we always would need to check if the
register is one of the few ultravisor registers that the kernel needs to
access.
Thanks,
Claudio
> cheers
>
^ permalink raw reply related
* Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
From: Thiago Jung Bauermann @ 2019-07-18 21:28 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Anshuman Khandual, Alexey Kardashevskiy, Mike Anderson, Ram Pai,
linux-kernel, Claudio Carvalho, Paul Mackerras, linuxppc-dev,
Christoph Hellwig
In-Reply-To: <20190718195850.GU20882@gate.crashing.org>
Hello Segher,
Thanks for your review and suggestions!
Segher Boessenkool <segher@kernel.crashing.org> writes:
> (Sorry to hijack your reply).
>
> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote:
>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
>> >From: Ram Pai <linuxram@us.ibm.com>
>> >+static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
>> >+{
>> >+ register uint64_t func asm("r3") = UV_ESM;
>> >+ register uint64_t arg1 asm("r4") = (uint64_t)kbase;
>> >+ register uint64_t arg2 asm("r5") = (uint64_t)fdt;
>>
>> What does UV do with kbase and fdt precisely? Few words in the commit
>> log will do.
>>
>> >+
>> >+ asm volatile("sc 2\n"
>> >+ : "=r"(func)
>> >+ : "0"(func), "r"(arg1), "r"(arg2)
>> >+ :);
>> >+
>> >+ return (int)func;
>>
>> And why "func"? Is it "function"? Weird name. Thanks,
Yes, I believe func is for function. Perhaps ucall would be clearer
if the variable wasn't reused for the return value as Segher points out.
> Maybe the three vars should just be called "r3", "r4", and "r5" --
> r3 is used as return value as well, so "func" isn't a great name for it.
Yes, that does seem simpler.
> Some other comments about this inline asm:
>
> The "\n" makes the generated asm look funny and has no other function.
> Instead of using backreferences you can use a "+" constraint, "inout".
> Empty clobber list is strange.
> Casts to the return type, like most other casts, are an invitation to
> bugs and not actually useful.
>
> So this can be written
>
> static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
> {
> register uint64_t r3 asm("r3") = UV_ESM;
> register uint64_t r4 asm("r4") = kbase;
> register uint64_t r4 asm("r5") = fdt;
>
> asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5));
>
> return r3;
> }
I'll adopt your version, it is cleaner inded. Thanks for providing it!
> (and it probably should use u64 instead of both uint64_t and unsigned long?)
Almost all of prom_init.c uses unsigned long, with u64 in just a few
places. uint64_t isn't used anywhere else in the file. I'll switch to
unsigned long everywhere, since this feature is only for 64 bit.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v9 08/10] open: openat2(2) syscall
From: Arnd Bergmann @ 2019-07-18 21:29 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, Linux-sh list, Alexei Starovoitov,
Linux Kernel Mailing List, David Howells,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai, Linux ARM,
linux-mips, linux-xtensa, Kees Cook, Jann Horn, linuxppc-dev,
linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan, David Drysdale,
Christian Brauner, J. Bruce Fields, Parisc List, Linux API,
Chanho Min, Jeff Layton, Oleg Nesterov, Eric Biederman, alpha,
Linux FS-devel Mailing List, Andrew Morton, Linus Torvalds,
containers
In-Reply-To: <20190718161231.xcno272nvqpln3wj@yavin>
On Thu, Jul 18, 2019 at 6:12 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-07-18, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sat, Jul 6, 2019 at 5:00 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > In fact, that seems similar enough to the existing openat() that I think
> > you could also just add the fifth argument to the existing call when
> > a newly defined flag is set, similarly to how we only use the 'mode'
> > argument when O_CREAT or O_TMPFILE are set.
>
> I considered doing this (and even had a preliminary version of it), but
> I discovered that I was not in favour of this idea -- once I started to
> write tests using it -- for a few reasons:
>
> 1. It doesn't really allow for clean extension for a future 6th
> argument (because you are using up O_* flags to signify "use the
> next argument", and O_* flags don't give -EINVAL if they're
> unknown). Now, yes you can do the on-start runtime check that
> everyone does -- but I've never really liked having to do it.
>
> Having reserved padding for later extensions (that is actually
> checked and gives -EINVAL) matches more modern syscall designs.
>
> 2. I really was hoping that the variadic openat(2) could be done away
> using this union setup (Linus said he didn't like it, and suggested
> using something like 'struct stat' as an argument for openat(2) --
> though personally I am not sure I would personally like to use an
> interface like that).
>
> 3. In order to avoid wasting a syscall argument for mode/mask you need
> to either have something like your suggested mode_mask (which makes
> the syscall arguments less consistent) or have some sort of
> mode-like argument that is treated specially (which is really awful
> on multiple levels -- this one I also tried and even wrote my
> original tests using). And in both cases, the shims for
> open{,at}(2) are somewhat less clean.
These are all good reasons, thanks for providing the background.
> All of that being said, I'd be happy to switch to whatever you think
> makes the most sense. As long as it's possible to get an O_PATH with
> RESOLVE_IN_ROOT set, I'm happy.
I don't feel I should be in charge of making the decision. I'd still
prefer avoiding the indirect argument structure because
4. it's inconsistent with most other syscalls
5. you get the same problem with seccomp and strace that
clone3() has -- these and others only track the register
arguments by default.
6. copying the structure adds a small overhead compared to
passing registers
7. the calling conventions may be inconvenient for a user space
library, so you end up with different prototypes for the low-level
syscall and the libc abstraction.
I don't see any of the above seven points as a showstopper
either way, so I hope someone else has a strong opinion
and can make the decision easier for you.
In the meantime just keep what you have, so you don't have
to change it multiple times.
Arnd
^ permalink raw reply
* Re: Crash in kvmppc_xive_release()
From: Cédric Le Goater @ 2019-07-18 21:51 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, Greg Kurz, Satheesh Rajendran,
Paul Mackerras
In-Reply-To: <3c152700-81de-9b34-e8a7-70b341a92197@kaod.org>
On 18/07/2019 15:14, Cédric Le Goater wrote:
> On 18/07/2019 14:49, Michael Ellerman wrote:
>> Anyone else seen this?
>>
>> This is running ~176 VMs on a Power9 (1 per thread), host crashes:
>
> This is beyond the underlying limits of XIVE.
>
> As we allocate 2K vCPUs per VM, that is 16K EQs for interrupt events. The overall
> EQ count is 1M. I let you calculate what is our max number of VMs ...
>
>> [ 66.403750][ T6423] xive: OPAL failed to allocate VCPUs order 11, err -10
>
> Hence, the OPAL XIVE driver fails which is good but ...
>
>> [188523.080935670,4] Spent 1783 msecs in OPAL call 135!
>> [ 66.484965][ T6250] BUG: Kernel NULL pointer dereference at 0x000042e8
>> [ 66.485558][ T6250] Faulting instruction address: 0xc008000011a33fcc
>> [ 66.485990][ T6250] Oops: Kernel access of bad area, sig: 7 [#1]
>> [ 66.486405][ T6250] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
>> [ 66.486967][ T6250] Modules linked in: kvm_hv kvm
>> [ 66.487275][ T6250] CPU: 107 PID: 6250 Comm: qemu-system-ppc Not tainted 5.2.0-rc2-gcc9x-gf5a9e488d623 #1
>> [ 66.487902][ T6250] NIP: c008000011a33fcc LR: c008000011a33fc4 CTR: c0000000005d5970
>> [ 66.488383][ T6250] REGS: c000001fabebb900 TRAP: 0300 Not tainted (5.2.0-rc2-gcc9x-gf5a9e488d623)
>> [ 66.488933][ T6250] MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24028224 XER: 00000000
>> [ 66.489724][ T6250] CFAR: c0000000005d6a4c DAR: 00000000000042e8 DSISR: 00080000 IRQMASK: 0
>> [ 66.489724][ T6250] GPR00: c008000011a33fc4 c000001fabebbb90 c008000011a5a200 c000000001399928
>> [ 66.489724][ T6250] GPR04: 0000000000000001 c00000000047b8d0 0000000000000000 0000000000000001
>> [ 66.489724][ T6250] GPR08: 0000000000000000 0000000000000000 c000001fa8c42f00 c008000011a3af20
>> [ 66.489724][ T6250] GPR12: 0000000000008000 c0002023ff65a880 000000013a1b4000 0000000000000002
>> [ 66.489724][ T6250] GPR16: 0000000010000000 0000000000000002 0000000000000001 000000012b194cc0
>> [ 66.489724][ T6250] GPR20: 00007fffb1645250 0000000000000001 0000000000000031 0000000000000000
>> [ 66.489724][ T6250] GPR24: 00007fffb16408d8 c000001ffafb62e0 c000001f78699360 c000001ff35d0620
>> [ 66.489724][ T6250] GPR28: c000001ed0ed0000 c000001ecd900000 0000000000000000 c000001ed0ed0000
>> [ 66.495211][ T6250] NIP [c008000011a33fcc] kvmppc_xive_release+0x54/0x1b0 [kvm]
>> [ 66.495642][ T6250] LR [c008000011a33fc4] kvmppc_xive_release+0x4c/0x1b0 [kvm]
>> [ 66.496101][ T6250] Call Trace:
>> [ 66.496314][ T6250] [c000001fabebbb90] [c008000011a33fc4] kvmppc_xive_release+0x4c/0x1b0 [kvm] (unreliable)
>> [ 66.496893][ T6250] [c000001fabebbbf0] [c008000011a18d54] kvm_device_release+0xac/0xf0 [kvm]
>> [ 66.497399][ T6250] [c000001fabebbc30] [c000000000442f8c] __fput+0xec/0x310
>> [ 66.497815][ T6250] [c000001fabebbc90] [c000000000145f94] task_work_run+0x114/0x170
>> [ 66.498296][ T6250] [c000001fabebbce0] [c000000000115274] do_exit+0x454/0xee0
>> [ 66.498743][ T6250] [c000001fabebbdc0] [c000000000115dd0] do_group_exit+0x60/0xe0
>> [ 66.499201][ T6250] [c000001fabebbe00] [c000000000115e74] sys_exit_group+0x24/0x40
>> [ 66.499747][ T6250] [c000001fabebbe20] [c00000000000b83c] system_call+0x5c/0x70
>> [ 66.500261][ T6250] Instruction dump:
>> [ 66.500484][ T6250] fbe1fff8 fba1ffe8 fbc1fff0 7c7c1b78 f8010010 f821ffa1 eba30010 e87d0010
>> [ 66.501006][ T6250] ebdd0000 48006f61 e8410018 39200000 <eb7e42ea> 913e42e8 48007f3d e8410018
>> [ 66.501529][ T6250] ---[ end trace c021a6ca03594ec3 ]---
>> [ 66.513119][ T6150] xive: OPAL failed to allocate VCPUs order 11, err -10
>
>
> ... the rollback code in case of such error must be bogus. It was never tested
> clearly :/
Here is a fix. Could you give it a try on your system ?
Thanks,
C.
From b6f728ca19a9540c8bf4f5a56991c4e3dab4cf56 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Thu, 18 Jul 2019 22:15:31 +0200
Subject: [PATCH] KVM: PPC: Book3S HV: XIVE: fix rollback when
kvmppc_xive_create fails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The XIVE device structure is now allocated in kvmppc_xive_get_device()
and kfree'd in kvmppc_core_destroy_vm(). In case of an OPAL error when
allocating the XIVE VPs, the kfree() call in kvmppc_xive_*create()
will result in a double free and corrupt the host memory.
Fixes: 5422e95103cf ("KVM: PPC: Book3S HV: XIVE: Replace the 'destroy' method by a 'release' method")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/kvm/book3s_xive.c | 4 +---
arch/powerpc/kvm/book3s_xive_native.c | 4 ++--
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index 6ca0d7376a9f..e3ba67095895 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -1986,10 +1986,8 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
xive->single_escalation = xive_native_has_single_escalation();
- if (ret) {
- kfree(xive);
+ if (ret)
return ret;
- }
return 0;
}
diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
index c7c7e3d4c031..45b0c143280c 100644
--- a/arch/powerpc/kvm/book3s_xive_native.c
+++ b/arch/powerpc/kvm/book3s_xive_native.c
@@ -1090,9 +1090,9 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
xive->ops = &kvmppc_xive_native_ops;
if (ret)
- kfree(xive);
+ return ret;
- return ret;
+ return 0;
}
/*
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
From: Alexey Kardashevskiy @ 2019-07-19 0:09 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Anshuman Khandual, Mike Anderson, Ram Pai, linux-kernel,
Claudio Carvalho, Paul Mackerras, linuxppc-dev, Christoph Hellwig
In-Reply-To: <875znz3ud7.fsf@morokweng.localdomain>
On 19/07/2019 07:28, Thiago Jung Bauermann wrote:
>
> Hello Segher,
>
> Thanks for your review and suggestions!
>
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>
>> (Sorry to hijack your reply).
>>
>> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote:
>>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
>>>> From: Ram Pai <linuxram@us.ibm.com>
>>>> +static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
>>>> +{
>>>> + register uint64_t func asm("r3") = UV_ESM;
>>>> + register uint64_t arg1 asm("r4") = (uint64_t)kbase;
>>>> + register uint64_t arg2 asm("r5") = (uint64_t)fdt;
>>>
>>> What does UV do with kbase and fdt precisely? Few words in the commit
>>> log will do.
What about this one? :)
>>>
>>>> +
>>>> + asm volatile("sc 2\n"
>>>> + : "=r"(func)
>>>> + : "0"(func), "r"(arg1), "r"(arg2)
>>>> + :);
>>>> +
>>>> + return (int)func;
>>>
>>> And why "func"? Is it "function"? Weird name. Thanks,
>
> Yes, I believe func is for function. Perhaps ucall would be clearer
> if the variable wasn't reused for the return value as Segher points out.
>
>> Maybe the three vars should just be called "r3", "r4", and "r5" --
>> r3 is used as return value as well, so "func" isn't a great name for it.
>
> Yes, that does seem simpler.
>
>> Some other comments about this inline asm:
>>
>> The "\n" makes the generated asm look funny and has no other function.
>> Instead of using backreferences you can use a "+" constraint, "inout".
>> Empty clobber list is strange.
>> Casts to the return type, like most other casts, are an invitation to
>> bugs and not actually useful.
>>
>> So this can be written
>>
>> static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
>> {
>> register uint64_t r3 asm("r3") = UV_ESM;
>> register uint64_t r4 asm("r4") = kbase;
>> register uint64_t r4 asm("r5") = fdt;
>>
>> asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5));
>>
>> return r3;
>> }
>
> I'll adopt your version, it is cleaner inded. Thanks for providing it!
>
>> (and it probably should use u64 instead of both uint64_t and unsigned long?)
>
> Almost all of prom_init.c uses unsigned long, with u64 in just a few
> places. uint64_t isn't used anywhere else in the file. I'll switch to
> unsigned long everywhere, since this feature is only for 64 bit.
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 03/13] powerpc/prom_init: Add the ESM call to prom_init
From: Thiago Jung Bauermann @ 2019-07-19 0:48 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anshuman Khandual, Mike Anderson, Ram Pai, linux-kernel,
Claudio Carvalho, Paul Mackerras, linuxppc-dev, Christoph Hellwig
In-Reply-To: <f4cba627-8c40-ce95-0ede-b01edf3546dc@ozlabs.ru>
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 19/07/2019 07:28, Thiago Jung Bauermann wrote:
>>
>> Hello Segher,
>>
>> Thanks for your review and suggestions!
>>
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>>
>>> (Sorry to hijack your reply).
>>>
>>> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote:
>>>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote:
>>>>> From: Ram Pai <linuxram@us.ibm.com>
>>>>> +static int enter_secure_mode(unsigned long kbase, unsigned long fdt)
>>>>> +{
>>>>> + register uint64_t func asm("r3") = UV_ESM;
>>>>> + register uint64_t arg1 asm("r4") = (uint64_t)kbase;
>>>>> + register uint64_t arg2 asm("r5") = (uint64_t)fdt;
>>>>
>>>> What does UV do with kbase and fdt precisely? Few words in the commit
>>>> log will do.
>
>
> What about this one? :)
Sorry, I don't have an elaborate answer yet. The non-elaborate answer is
that the ultravisor uses the kbase and fdt as part of integrity checking
of the secure guest.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v9 08/10] open: openat2(2) syscall
From: Dmitry V. Levin @ 2019-07-19 1:59 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k, Al Viro,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190706145737.5299-9-cyphar@cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 785 bytes --]
On Sun, Jul 07, 2019 at 12:57:35AM +1000, Aleksa Sarai wrote:
[...]
> +/**
> + * Arguments for how openat2(2) should open the target path. If @extra is zero,
> + * then openat2(2) is identical to openat(2).
> + *
> + * @flags: O_* flags (unknown flags ignored).
What was the rationale for implementing this semantics?
Ignoring unknown flags makes potential extension of this new interface
problematic. This has bitten us many times already, so ...
> + * @mode: O_CREAT file mode (ignored otherwise).
> + * @upgrade_mask: restrict how the O_PATH may be re-opened (ignored otherwise).
> + * @resolve: RESOLVE_* flags (-EINVAL on unknown flags).
... could you consider implementing this (-EINVAL on unknown flags) semantics
for @flags as well, please?
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v9 08/10] open: openat2(2) syscall
From: Dmitry V. Levin @ 2019-07-19 2:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-ia64, Linux-sh list, Alexei Starovoitov,
Linux Kernel Mailing List, David Howells,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai, Linux ARM,
linux-mips, linux-xtensa, Kees Cook, Jann Horn, linuxppc-dev,
Aleksa Sarai, Al Viro, Andy Lutomirski, Shuah Khan,
David Drysdale, Christian Brauner, J. Bruce Fields, Parisc List,
linux-m68k, Linux API, Chanho Min, Jeff Layton, Oleg Nesterov,
Eric Biederman, alpha, Linux FS-devel Mailing List, Andrew Morton,
Linus Torvalds, containers
In-Reply-To: <CAK8P3a3MiYK4bJiA3G_m5H-TpfN5__--b+=szsJBhG7_it+NQg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
On Thu, Jul 18, 2019 at 11:29:50PM +0200, Arnd Bergmann wrote:
[...]
> 5. you get the same problem with seccomp and strace that
> clone3() has -- these and others only track the register
> arguments by default.
Just for the record, this is definitely not the case for strace:
it decodes arrays, structures, netlink messages, and so on by default.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v9 08/10] open: openat2(2) syscall
From: Aleksa Sarai @ 2019-07-19 2:19 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k, Al Viro,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190719015933.GA18022@altlinux.org>
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
On 2019-07-19, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Sun, Jul 07, 2019 at 12:57:35AM +1000, Aleksa Sarai wrote:
> [...]
> > +/**
> > + * Arguments for how openat2(2) should open the target path. If @extra is zero,
> > + * then openat2(2) is identical to openat(2).
> > + *
> > + * @flags: O_* flags (unknown flags ignored).
>
> What was the rationale for implementing this semantics?
> Ignoring unknown flags makes potential extension of this new interface
> problematic. This has bitten us many times already, so ...
I am mirroring the semantics of open(2) and openat(2).
To be clear, I am in favour of doing it -- and it would definitely be
possible to implement it with -EINVAL (you would just mask off
~VALID_OPEN_FLAGS for the older syscalls). But Linus' response to my
point about (the lack of) -EINVAL for unknown open(2) flags gave me the
impression he would be against this idea (though I might be
misunderstanding the point he was making).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v4 4/8] KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a PATE
From: Michael Ellerman @ 2019-07-19 2:25 UTC (permalink / raw)
To: Claudio Carvalho, linuxppc-dev
Cc: Ryan Grimm, Madhavan Srinivasan, Michael Anderson, Ram Pai,
kvm-ppc, Bharata B Rao, Ryan Grimm, Sukadev Bhattiprolu,
Thiago Bauermann, Anshuman Khandual
In-Reply-To: <6688060f-3744-cae5-635e-f1ee3ff48c19@linux.ibm.com>
Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> On 7/11/19 9:57 AM, Michael Ellerman wrote:
>>> static pmd_t *get_pmd_from_cache(struct mm_struct *mm)
>>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>>> index 8904aa1243d8..da6a6b76a040 100644
>>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>>> @@ -656,8 +656,10 @@ void radix__early_init_mmu_secondary(void)
>>> lpcr = mfspr(SPRN_LPCR);
>>> mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>>>
>>> - mtspr(SPRN_PTCR,
>>> - __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
>>> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>>> + mtspr(SPRN_PTCR, __pa(partition_tb) |
>>> + (PATB_SIZE_SHIFT - 12));
>>> +
>>> radix_init_amor();
>>> }
>>>
>>> @@ -673,7 +675,8 @@ void radix__mmu_cleanup_all(void)
>>> if (!firmware_has_feature(FW_FEATURE_LPAR)) {
>>> lpcr = mfspr(SPRN_LPCR);
>>> mtspr(SPRN_LPCR, lpcr & ~LPCR_UPRT);
>>> - mtspr(SPRN_PTCR, 0);
>>> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>>> + mtspr(SPRN_PTCR, 0);
>>> powernv_set_nmmu_ptcr(0);
>>> radix__flush_tlb_all();
>>> }
>> There's four of these case where we skip touching the PTCR, which is
>> right on the borderline of warranting an accessor. I guess we can do it
>> as a cleanup later.
>
> I agree.
>
> Since the kernel doesn't need to access a big number of ultravisor
> privileged registers, maybe we can define mtspr_<reg> and mfspr_<reg>
> inline functions that in ultravisor.h that skip touching the register if an
> ultravisor is present and and the register is ultravisor privileged. Thus,
> we don't need to replicate comments and that also would make it easier for
> developers to know what are the ultravisor privileged registers.
>
> Something like this:
>
> --- a/arch/powerpc/include/asm/ultravisor.h
> +++ b/arch/powerpc/include/asm/ultravisor.h
> @@ -10,10 +10,21 @@
>
> #include <asm/ultravisor-api.h>
> #include <asm/asm-prototypes.h>
> +#include <asm/reg.h>
>
> int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
> int depth, void *data);
>
> +static inline void mtspr_ptcr(unsigned long val)
> +{
> + /*
> + * If the ultravisor firmware is present, it maintains the partition
> + * table. PTCR becomes ultravisor privileged only for writing.
> + */
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + mtspr(SPRN_PTCR, val);
> +}
+
> static inline int uv_register_pate(u64 lpid, u64 dw0, u64 dw1)
> {
> return ucall_norets(UV_WRITE_PATE, lpid, dw0, dw1);
> diff --git a/arch/powerpc/mm/book3s64/pgtable.c
> b/arch/powerpc/mm/book3s64/pgtable.c
> index e1bbc48e730f..25156f9dfde8 100644
> --- a/arch/powerpc/mm/book3s64/pgtable.c
> +++ b/arch/powerpc/mm/book3s64/pgtable.c
> @@ -220,7 +220,7 @@ void __init mmu_partition_table_init(void)
> * 64 K size.
> */
> ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
> - mtspr(SPRN_PTCR, ptcr);
> + mtspr_ptcr(ptcr);
> powernv_set_nmmu_ptcr(ptcr);
> }
>
> What do you think?
I don't think that's actually clearer.
If the logic was always:
if (ultravisor)
do_ucall()
else
mtspr()
Then a wrapper called eg. set_ptcr() would make sense.
But because in some cases you do a ucall and some you don't, I don't
think it helps to hide that in an accessor like above.
That is confusing to a reader who sees all this code to setup a value
and then the write to PTCR does nothing.
And in fact you didn't explain why it's OK for those cases to not do the
write at all.
> An alternative could be to change the mtspr() and mfspr() macros as we
> proposed in the v1, but access to non-ultravisor privileged registers would
> be performance impacted because we always would need to check if the
> register is one of the few ultravisor registers that the kernel needs to
> access.
Yeah that and it would be very confusing to a reader who sees:
ptcr = ...;
mtspr(SPRN_PTCR, ptcr);
...
And then they discover the mtspr does *nothing* when the Ultravisor is
enabled.
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc: slightly improve cache helpers
From: Nathan Chancellor @ 2019-07-19 3:24 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190709064952.GA40851@archlinux-threadripper>
On Mon, Jul 08, 2019 at 11:49:52PM -0700, Nathan Chancellor wrote:
> On Tue, Jul 09, 2019 at 07:04:43AM +0200, Christophe Leroy wrote:
> > Is that a Clang bug ?
>
> No idea, it happens with clang-8 and clang-9 though (pretty sure there
> were fixes for PowerPC in clang-8 so something before it probably won't
> work but I haven't tried).
>
> >
> > Do you have a disassembly of the code both with and without this patch in
> > order to compare ?
>
> I can give you whatever disassembly you want (or I can upload the raw
> files if that is easier).
>
> Cheers,
> Nathan
Hi Christophe and Segher,
What disassembly/files did you need to start taking a look at this? I
can upload/send whatever you need.
If it is easier, we have a self contained clang build script available
to make it easier to reproduce this on your side (does assume an x86_64
host):
https://github.com/ClangBuiltLinux/tc-build
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
From: Michael Ellerman @ 2019-07-19 3:39 UTC (permalink / raw)
To: Segher Boessenkool, Masahiro Yamada
Cc: Stephen Rothwell, linuxppc-dev, Paul Mackerras,
Linux Kernel Mailing List, Nicholas Piggin
In-Reply-To: <20190718204631.GV20882@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Jul 18, 2019 at 11:19:58AM +0900, Masahiro Yamada wrote:
>> On Thu, Jul 18, 2019 at 1:46 AM Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>> Kbuild always uses thin archives as far as vmlinux is concerned.
>>
>> But, there are some other call-sites.
>>
>> masahiro@pug:~/ref/linux$ git grep '$(AR)' -- :^Documentation :^tools
>> arch/powerpc/boot/Makefile: BOOTAR := $(AR)
>> arch/unicore32/lib/Makefile: $(Q)$(AR) p $(GNU_LIBC_A) $(notdir $@) > $@
>> arch/unicore32/lib/Makefile: $(Q)$(AR) p $(GNU_LIBGCC_A) $(notdir $@) > $@
>> lib/raid6/test/Makefile: $(AR) cq $@ $^
>> scripts/Kbuild.include:ar-option = $(call try-run, $(AR) rc$(1)
>> "$$TMP",$(1),$(2))
>> scripts/Makefile.build: cmd_ar_builtin = rm -f $@; $(AR)
>> rcSTP$(KBUILD_ARFLAGS) $@ $(real-prereqs)
>> scripts/Makefile.lib: cmd_ar = rm -f $@; $(AR)
>> rcsTP$(KBUILD_ARFLAGS) $@ $(real-prereqs)
>>
>> Probably, you are interested in arch/powerpc/boot/Makefile.
>
> That one seems fine actually. The raid6 one I don't know.
>
>
> My original commit message was
>
> Without this, some versions of GNU ar fail to create
> an archive index if the object files it is packing
> together are of a different object format than ar's
> default format (for example, binutils compiled to
> default to 64-bit, with 32-bit objects).
>
> but I cannot reproduce the problem anymore. Shortly after my patch the
> thin archive code happened to binutils, and that overhauled some other
> things, which might have fixed it already?
>
>> > Yes, I know. This isn't about built-in.[oa], it is about *other*
>> > archives we at least *used to* create. If we *know* we do not anymore,
>> > then this workaround can of course be removed (and good riddance).
>>
>> If it is not about built-in.[oa],
>> which archive are you talking about?
>>
>> Can you pin-point the one?
>
> No, not anymore. Lost in the mists of time, I guess? I think we'll
> just have to file it as "it seems to work fine now".
Yeah I think so. If someone finds a case it breaks we can fix it then.
> Thank you (and everyone else) for the time looking at this!
Likewise.
cheers
^ permalink raw reply
* [PATCH AUTOSEL 5.2 060/171] powerpc/pseries/mobility: prevent cpu hotplug during DT update
From: Sasha Levin @ 2019-07-19 3:54 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Nathan Lynch, Gautham R . Shenoy, linuxppc-dev, Sasha Levin
In-Reply-To: <20190719035643.14300-1-sashal@kernel.org>
From: Nathan Lynch <nathanl@linux.ibm.com>
[ Upstream commit e59a175faa8df9d674247946f2a5a9c29c835725 ]
CPU online/offline code paths are sensitive to parts of the device
tree (various cpu node properties, cache nodes) that can be changed as
a result of a migration.
Prevent CPU hotplug while the device tree potentially is inconsistent.
Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/powerpc/platforms/pseries/mobility.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 0c48c8964783..50e7aee3c7f3 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -6,6 +6,7 @@
* Copyright (C) 2010 IBM Corporation
*/
+#include <linux/cpu.h>
#include <linux/kernel.h>
#include <linux/kobject.h>
#include <linux/smp.h>
@@ -335,11 +336,19 @@ void post_mobility_fixup(void)
if (rc)
printk(KERN_ERR "Post-mobility activate-fw failed: %d\n", rc);
+ /*
+ * We don't want CPUs to go online/offline while the device
+ * tree is being updated.
+ */
+ cpus_read_lock();
+
rc = pseries_devicetree_update(MIGRATION_SCOPE);
if (rc)
printk(KERN_ERR "Post-mobility device tree update "
"failed: %d\n", rc);
+ cpus_read_unlock();
+
/* Possibly switch to a new RFI flush type */
pseries_setup_rfi_flush();
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.2 077/171] powerpc/pci/of: Fix OF flags parsing for 64bit BARs
From: Sasha Levin @ 2019-07-19 3:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Sasha Levin, Shawn Anastasio, Alexey Kardashevskiy, Sam Bobroff,
Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20190719035643.14300-1-sashal@kernel.org>
From: Alexey Kardashevskiy <aik@ozlabs.ru>
[ Upstream commit df5be5be8735ef2ae80d5ae1f2453cd81a035c4b ]
When the firmware does PCI BAR resource allocation, it passes the assigned
addresses and flags (prefetch/64bit/...) via the "reg" property of
a PCI device device tree node so the kernel does not need to do
resource allocation.
The flags are stored in resource::flags - the lower byte stores
PCI_BASE_ADDRESS_SPACE/etc bits and the other bytes are IORESOURCE_IO/etc.
Some flags from PCI_BASE_ADDRESS_xxx and IORESOURCE_xxx are duplicated,
such as PCI_BASE_ADDRESS_MEM_PREFETCH/PCI_BASE_ADDRESS_MEM_TYPE_64/etc.
When parsing the "reg" property, we copy the prefetch flag but we skip
on PCI_BASE_ADDRESS_MEM_TYPE_64 which leaves the flags out of sync.
The missing IORESOURCE_MEM_64 flag comes into play under 2 conditions:
1. we remove PCI_PROBE_ONLY for pseries (by hacking pSeries_setup_arch()
or by passing "/chosen/linux,pci-probe-only");
2. we request resource alignment (by passing pci=resource_alignment=
via the kernel cmd line to request PAGE_SIZE alignment or defining
ppc_md.pcibios_default_alignment which returns anything but 0). Note that
the alignment requests are ignored if PCI_PROBE_ONLY is enabled.
With 1) and 2), the generic PCI code in the kernel unconditionally
decides to:
- reassign the BARs in pci_specified_resource_alignment() (works fine)
- write new BARs to the device - this fails for 64bit BARs as the generic
code looks at IORESOURCE_MEM_64 (not set) and writes only lower 32bits
of the BAR and leaves the upper 32bit unmodified which breaks BAR mapping
in the hypervisor.
This fixes the issue by copying the flag. This is useful if we want to
enforce certain BAR alignment per platform as handling subpage sized BARs
is proven to cause problems with hotplug (SLOF already aligns BARs to 64k).
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Shawn Anastasio <shawn@anastas.io>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/powerpc/kernel/pci_of_scan.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index 24522aa37665..c63c53b37e8e 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -42,6 +42,8 @@ unsigned int pci_parse_of_flags(u32 addr0, int bridge)
if (addr0 & 0x02000000) {
flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
+ if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+ flags |= IORESOURCE_MEM_64;
flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
if (addr0 & 0x40000000)
flags |= IORESOURCE_PREFETCH
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.2 092/171] powerpc/cacheflush: fix variable set but not used
From: Sasha Levin @ 2019-07-19 3:55 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Sasha Levin, Qian Cai, linuxppc-dev
In-Reply-To: <20190719035643.14300-1-sashal@kernel.org>
From: Qian Cai <cai@lca.pw>
[ Upstream commit 04db3ede40ae4fc23a5c4237254c4a53bbe4c1f2 ]
The powerpc's flush_cache_vmap() is defined as a macro and never use
both of its arguments, so it will generate a compilation warning,
lib/ioremap.c: In function 'ioremap_page_range':
lib/ioremap.c:203:16: warning: variable 'start' set but not used
[-Wunused-but-set-variable]
Fix it by making it an inline function.
Signed-off-by: Qian Cai <cai@lca.pw>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/powerpc/include/asm/cacheflush.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
index 74d60cfe8ce5..fd318f7c3eed 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -29,9 +29,12 @@
* not expect this type of fault. flush_cache_vmap is not exactly the right
* place to put this, but it seems to work well enough.
*/
-#define flush_cache_vmap(start, end) do { asm volatile("ptesync" ::: "memory"); } while (0)
+static inline void flush_cache_vmap(unsigned long start, unsigned long end)
+{
+ asm volatile("ptesync" ::: "memory");
+}
#else
-#define flush_cache_vmap(start, end) do { } while (0)
+static inline void flush_cache_vmap(unsigned long start, unsigned long end) { }
#endif
#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 1
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox