* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-07-15 22:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <20190715163453-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin <mst@redhat.com> writes:
> On Mon, Jul 15, 2019 at 05:29:06PM -0300, Thiago Jung Bauermann wrote:
>>
>> Michael S. Tsirkin <mst@redhat.com> writes:
>>
>> > On Sun, Jul 14, 2019 at 02:51:18AM -0300, Thiago Jung Bauermann wrote:
>> >>
>> >>
>> >> Michael S. Tsirkin <mst@redhat.com> writes:
>> >>
>> >> > 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
>> >> instance, Ubuntu 16.04 LTS (which is still supported) doesn't know about
>> >> it and wouldn't be able to use the device.
>> >
>> > OK and your target is to enable use with kernel drivers within
>> > guests, right?
>>
>> Right.
>>
>> > My question is, we are defining a new flag here, I guess old guests
>> > then do not set it. How does it help old guests? Or maybe it's
>> > not designed to ...
>>
>> Indeed. The idea is that QEMU can offer the flag, old guests can reject
>> it (or even new guests can reject it, if they decide not to convert into
>> secure VMs) and the feature negotiation will succeed with the flag
>> unset.
>
> OK. And then what does QEMU do? Assume guest is not encrypted I guess?
There's nothing different that QEMU needs to do, with or without the
flag. the perspective of the host, a secure guest and a regular guest
work the same way with respect to virtio.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-07-15 22:16 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <8736j7neg8.fsf@morokweng.localdomain>
On Mon, Jul 15, 2019 at 07:03:03PM -0300, Thiago Jung Bauermann wrote:
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Mon, Jul 15, 2019 at 05:29:06PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Michael S. Tsirkin <mst@redhat.com> writes:
> >>
> >> > On Sun, Jul 14, 2019 at 02:51:18AM -0300, Thiago Jung Bauermann wrote:
> >> >>
> >> >>
> >> >> Michael S. Tsirkin <mst@redhat.com> writes:
> >> >>
> >> >> > 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
> >> >> instance, Ubuntu 16.04 LTS (which is still supported) doesn't know about
> >> >> it and wouldn't be able to use the device.
> >> >
> >> > OK and your target is to enable use with kernel drivers within
> >> > guests, right?
> >>
> >> Right.
> >>
> >> > My question is, we are defining a new flag here, I guess old guests
> >> > then do not set it. How does it help old guests? Or maybe it's
> >> > not designed to ...
> >>
> >> Indeed. The idea is that QEMU can offer the flag, old guests can reject
> >> it (or even new guests can reject it, if they decide not to convert into
> >> secure VMs) and the feature negotiation will succeed with the flag
> >> unset.
> >
> > OK. And then what does QEMU do? Assume guest is not encrypted I guess?
>
> There's nothing different that QEMU needs to do, with or without the
> flag. the perspective of the host, a secure guest and a regular guest
> work the same way with respect to virtio.
OK. So now let's get back to implementation. What will
Linux guest driver do? It can't activate DMA API blindly since that
will assume translation also works, right?
Or do we somehow limit it to just a specific platform?
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-07-15 23:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <20190715181449-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin <mst@redhat.com> writes:
> On Mon, Jul 15, 2019 at 07:03:03PM -0300, Thiago Jung Bauermann wrote:
>>
>> Michael S. Tsirkin <mst@redhat.com> writes:
>>
>> > On Mon, Jul 15, 2019 at 05:29:06PM -0300, Thiago Jung Bauermann wrote:
>> >>
>> >> Michael S. Tsirkin <mst@redhat.com> writes:
>> >>
>> >> > On Sun, Jul 14, 2019 at 02:51:18AM -0300, Thiago Jung Bauermann wrote:
>> >> >>
>> >> >>
>> >> >> Michael S. Tsirkin <mst@redhat.com> writes:
>> >> >>
>> >> >> > 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
>> >> >> instance, Ubuntu 16.04 LTS (which is still supported) doesn't know about
>> >> >> it and wouldn't be able to use the device.
>> >> >
>> >> > OK and your target is to enable use with kernel drivers within
>> >> > guests, right?
>> >>
>> >> Right.
>> >>
>> >> > My question is, we are defining a new flag here, I guess old guests
>> >> > then do not set it. How does it help old guests? Or maybe it's
>> >> > not designed to ...
>> >>
>> >> Indeed. The idea is that QEMU can offer the flag, old guests can reject
>> >> it (or even new guests can reject it, if they decide not to convert into
>> >> secure VMs) and the feature negotiation will succeed with the flag
>> >> unset.
>> >
>> > OK. And then what does QEMU do? Assume guest is not encrypted I guess?
>>
>> There's nothing different that QEMU needs to do, with or without the
>> flag. the perspective of the host, a secure guest and a regular guest
>> work the same way with respect to virtio.
>
> OK. So now let's get back to implementation. What will
> Linux guest driver do? It can't activate DMA API blindly since that
> will assume translation also works, right?
It can on pseries, because we always have a 1:1 window mapping the whole
guest memory.
> Or do we somehow limit it to just a specific platform?
Yes, we want to accept the new flag only on secure pseries guests.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Benjamin Herrenschmidt @ 2019-07-15 23:24 UTC (permalink / raw)
To: Thiago Jung Bauermann, Michael S. Tsirkin
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <8736j7neg8.fsf@morokweng.localdomain>
On Mon, 2019-07-15 at 19:03 -0300, Thiago Jung Bauermann wrote:
> > > Indeed. The idea is that QEMU can offer the flag, old guests can
> > > reject
> > > it (or even new guests can reject it, if they decide not to
> > > convert into
> > > secure VMs) and the feature negotiation will succeed with the
> > > flag
> > > unset.
> >
> > OK. And then what does QEMU do? Assume guest is not encrypted I
> > guess?
>
> There's nothing different that QEMU needs to do, with or without the
> flag. the perspective of the host, a secure guest and a regular guest
> work the same way with respect to virtio.
This is *precisely* why I was against adding a flag and touch the
protocol negociation with qemu in the first place, back when I cared
about that stuff...
Guys, this has gone in circles over and over again.
This has nothing to do with qemu. Qemu doesn't need to know about this.
It's entirely guest local. This is why the one-liner in virtio was a
far better and simpler solution.
This is something the guest does to itself (with the participation of a
ultravisor but that's not something qemu cares about at this stage, at
least not as far as virtio is concerned).
Basically, the guest "hides" its memory from the host using a HW secure
memory facility. As a result, it needs to ensure that all of its DMA
pages are bounced through insecure pages that aren't hidden. That's it,
it's all guest side. Qemu shouldn't have to care about it at all.
Cheers,
Ben.
^ permalink raw reply
* [PATCH] PPC: Set reserved PCR bits
From: Jordan Niethe @ 2019-07-16 2:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, alistair, sjitindarsingh, Jordan Niethe
Currently the reserved bits of the Processor Compatibility Register
(PCR) are cleared as per the Programming Note in Section 1.3.3 of the
ISA. An update is planned for the ISA so that PCR reserved bits should
be set. Set the reserved bits of the PCR as required.
Acked-by: Alistair Popple <alistair@popple.id.au>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/include/asm/reg.h | 3 +++
arch/powerpc/kernel/cpu_setup_power.S | 6 ++++++
arch/powerpc/kernel/dt_cpu_ftrs.c | 3 ++-
arch/powerpc/kvm/book3s_hv.c | 11 +++++++----
arch/powerpc/kvm/book3s_hv_nested.c | 6 +++---
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 10 ++++++----
6 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 10caa145f98b..2e0815994f4d 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -475,6 +475,7 @@
#define PCR_VEC_DIS (1ul << (63-0)) /* Vec. disable (bit NA since POWER8) */
#define PCR_VSX_DIS (1ul << (63-1)) /* VSX disable (bit NA since POWER8) */
#define PCR_TM_DIS (1ul << (63-2)) /* Trans. memory disable (POWER8) */
+#define PCR_HIGH_BITS (PCR_VEC_DIS | PCR_VSX_DIS | PCR_TM_DIS)
/*
* These bits are used in the function kvmppc_set_arch_compat() to specify and
* determine both the compatibility level which we want to emulate and the
@@ -483,6 +484,8 @@
#define PCR_ARCH_207 0x8 /* Architecture 2.07 */
#define PCR_ARCH_206 0x4 /* Architecture 2.06 */
#define PCR_ARCH_205 0x2 /* Architecture 2.05 */
+#define PCR_LOW_BITS (PCR_ARCH_207 | PCR_ARCH_206 | PCR_ARCH_205)
+#define PCR_MASK ~(PCR_HIGH_BITS | PCR_LOW_BITS) /* PCR Reserved Bits */
#define SPRN_HEIR 0x153 /* Hypervisor Emulated Instruction Register */
#define SPRN_TLBINDEXR 0x154 /* P7 TLB control register */
#define SPRN_TLBVPNR 0x155 /* P7 TLB control register */
diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S
index 3239a9fe6c1c..a460298c7ddb 100644
--- a/arch/powerpc/kernel/cpu_setup_power.S
+++ b/arch/powerpc/kernel/cpu_setup_power.S
@@ -23,6 +23,7 @@ _GLOBAL(__setup_cpu_power7)
beqlr
li r0,0
mtspr SPRN_LPID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
li r4,(LPCR_LPES1 >> LPCR_LPES_SH)
@@ -37,6 +38,7 @@ _GLOBAL(__restore_cpu_power7)
beqlr
li r0,0
mtspr SPRN_LPID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
li r4,(LPCR_LPES1 >> LPCR_LPES_SH)
@@ -54,6 +56,7 @@ _GLOBAL(__setup_cpu_power8)
beqlr
li r0,0
mtspr SPRN_LPID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
ori r3, r3, LPCR_PECEDH
@@ -76,6 +79,7 @@ _GLOBAL(__restore_cpu_power8)
beqlr
li r0,0
mtspr SPRN_LPID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
ori r3, r3, LPCR_PECEDH
@@ -98,6 +102,7 @@ _GLOBAL(__setup_cpu_power9)
mtspr SPRN_PSSCR,r0
mtspr SPRN_LPID,r0
mtspr SPRN_PID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
LOAD_REG_IMMEDIATE(r4, LPCR_PECEDH | LPCR_PECE_HVEE | LPCR_HVICE | LPCR_HEIC)
@@ -123,6 +128,7 @@ _GLOBAL(__restore_cpu_power9)
mtspr SPRN_PSSCR,r0
mtspr SPRN_LPID,r0
mtspr SPRN_PID,r0
+ LOAD_REG_IMMEDIATE(r0, PCR_MASK)
mtspr SPRN_PCR,r0
mfspr r3,SPRN_LPCR
LOAD_REG_IMMEDIATE(r4, LPCR_PECEDH | LPCR_PECE_HVEE | LPCR_HVICE | LPCR_HEIC)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 0e4c7c1f5c3e..7f2858e3e56a 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -102,7 +102,7 @@ static void __restore_cpu_cpufeatures(void)
if (hv_mode) {
mtspr(SPRN_LPID, 0);
mtspr(SPRN_HFSCR, system_registers.hfscr);
- mtspr(SPRN_PCR, 0);
+ mtspr(SPRN_PCR, PCR_MASK);
}
mtspr(SPRN_FSCR, system_registers.fscr);
@@ -145,6 +145,7 @@ static void __init cpufeatures_setup_cpu(void)
mtspr(SPRN_HFSCR, 0);
}
mtspr(SPRN_FSCR, 0);
+ mtspr(SPRN_PCR, PCR_MASK);
/*
* LPCR does not get cleared, to match behaviour with secondaries
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 76b1801aa44a..fb1debaa5a7c 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -401,8 +401,11 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
spin_lock(&vc->lock);
vc->arch_compat = arch_compat;
- /* Set all PCR bits for which guest_pcr_bit <= bit < host_pcr_bit */
- vc->pcr = host_pcr_bit - guest_pcr_bit;
+ /*
+ * Set all PCR bits for which guest_pcr_bit <= bit < host_pcr_bit
+ * Also set all reserved PCR bits
+ */
+ vc->pcr = (host_pcr_bit - guest_pcr_bit) | PCR_MASK;
spin_unlock(&vc->lock);
return 0;
@@ -3398,7 +3401,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
}
if (vc->pcr)
- mtspr(SPRN_PCR, vc->pcr);
+ mtspr(SPRN_PCR, vc->pcr | PCR_MASK);
mtspr(SPRN_DPDES, vc->dpdes);
mtspr(SPRN_VTB, vc->vtb);
@@ -3478,7 +3481,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
vc->vtb = mfspr(SPRN_VTB);
mtspr(SPRN_DPDES, 0);
if (vc->pcr)
- mtspr(SPRN_PCR, 0);
+ mtspr(SPRN_PCR, PCR_MASK);
if (vc->tb_offset_applied) {
u64 new_tb = mftb() - vc->tb_offset_applied;
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index 735e0ac6f5b2..e916522c5c39 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -29,7 +29,7 @@ void kvmhv_save_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
{
struct kvmppc_vcore *vc = vcpu->arch.vcore;
- hr->pcr = vc->pcr;
+ hr->pcr = vc->pcr | PCR_MASK;
hr->dpdes = vc->dpdes;
hr->hfscr = vcpu->arch.hfscr;
hr->tb_offset = vc->tb_offset;
@@ -65,7 +65,7 @@ static void byteswap_hv_regs(struct hv_guest_state *hr)
hr->lpid = swab32(hr->lpid);
hr->vcpu_token = swab32(hr->vcpu_token);
hr->lpcr = swab64(hr->lpcr);
- hr->pcr = swab64(hr->pcr);
+ hr->pcr = swab64(hr->pcr) | PCR_MASK;
hr->amor = swab64(hr->amor);
hr->dpdes = swab64(hr->dpdes);
hr->hfscr = swab64(hr->hfscr);
@@ -148,7 +148,7 @@ static void restore_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
{
struct kvmppc_vcore *vc = vcpu->arch.vcore;
- vc->pcr = hr->pcr;
+ vc->pcr = hr->pcr | PCR_MASK;
vc->dpdes = hr->dpdes;
vcpu->arch.hfscr = hr->hfscr;
vcpu->arch.dawr = hr->dawr0;
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 337e64468d78..f21dee7a1757 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -643,8 +643,10 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
/* Load guest PCR value to select appropriate compat mode */
37: ld r7, VCORE_PCR(r5)
- cmpdi r7, 0
+ LOAD_REG_IMMEDIATE(r6, PCR_MASK)
+ cmpld r7, r6
beq 38f
+ or r7, r7, r6
mtspr SPRN_PCR, r7
38:
@@ -1884,10 +1886,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
/* Reset PCR */
ld r0, VCORE_PCR(r5)
- cmpdi r0, 0
+ LOAD_REG_IMMEDIATE(r6, PCR_MASK)
+ cmpld r0, r6
beq 18f
- li r0, 0
- mtspr SPRN_PCR, r0
+ mtspr SPRN_PCR, r6
18:
/* Signal secondary CPUs to continue */
stb r0,VCORE_IN_GUEST(r5)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] powerpc/nvdimm: Pick the nearby online node if the device node is not online
From: Aneesh Kumar K.V @ 2019-07-16 3:43 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: linuxppc-dev, Oliver O'Halloran
In-Reply-To: <20190711145654.17589-1-aneesh.kumar@linux.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> This is similar to what ACPI does. Nvdimm layer doesn't bring the SCM device
> numa node online. Hence we need to make sure we always use an online node
> as ndr_desc.numa_node. Otherwise this result in kernel crashes. The target
> node is used by dax/kmem and that will bring up the numa node online correctly.
>
> Without this patch, we do hit kernel crash as below because we try to access
> uninitialized NODE_DATA in different code paths.
>
> cpu 0x0: Vector: 300 (Data Access) at [c0000000fac53170]
> pc: c0000000004bbc50: ___slab_alloc+0x120/0xca0
> lr: c0000000004bc834: __slab_alloc+0x64/0xc0
> sp: c0000000fac53400
> msr: 8000000002009033
> dar: 73e8
> dsisr: 80000
> current = 0xc0000000fabb6d80
> paca = 0xc000000003870000 irqmask: 0x03 irq_happened: 0x01
> pid = 7, comm = kworker/u16:0
> Linux version 5.2.0-06234-g76bd729b2644 (kvaneesh@ltc-boston123) (gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)) #135 SMP Thu Jul 11 05:36:30 CDT 2019
> enter ? for help
> [link register ] c0000000004bc834 __slab_alloc+0x64/0xc0
> [c0000000fac53400] c0000000fac53480 (unreliable)
> [c0000000fac53500] c0000000004bc818 __slab_alloc+0x48/0xc0
> [c0000000fac53560] c0000000004c30a0 __kmalloc_node_track_caller+0x3c0/0x6b0
> [c0000000fac535d0] c000000000cfafe4 devm_kmalloc+0x74/0xc0
> [c0000000fac53600] c000000000d69434 nd_region_activate+0x144/0x560
> [c0000000fac536d0] c000000000d6b19c nd_region_probe+0x17c/0x370
> [c0000000fac537b0] c000000000d6349c nvdimm_bus_probe+0x10c/0x230
> [c0000000fac53840] c000000000cf3cc4 really_probe+0x254/0x4e0
> [c0000000fac538d0] c000000000cf429c driver_probe_device+0x16c/0x1e0
> [c0000000fac53950] c000000000cf0b44 bus_for_each_drv+0x94/0x130
> [c0000000fac539b0] c000000000cf392c __device_attach+0xdc/0x200
> [c0000000fac53a50] c000000000cf231c bus_probe_device+0x4c/0xf0
> [c0000000fac53a90] c000000000ced268 device_add+0x528/0x810
> [c0000000fac53b60] c000000000d62a58 nd_async_device_register+0x28/0xa0
> [c0000000fac53bd0] c0000000001ccb8c async_run_entry_fn+0xcc/0x1f0
> [c0000000fac53c50] c0000000001bcd9c process_one_work+0x46c/0x860
> [c0000000fac53d20] c0000000001bd4f4 worker_thread+0x364/0x5f0
> [c0000000fac53db0] c0000000001c7260 kthread+0x1b0/0x1c0
> [c0000000fac53e20] c00000000000b954 ret_from_kernel_thread+0x5c/0x68
>
> With the patch we get
>
> # numactl -H
> available: 2 nodes (0-1)
> node 0 cpus:
> node 0 size: 0 MB
> node 0 free: 0 MB
> node 1 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 1 size: 130865 MB
> node 1 free: 129130 MB
> node distances:
> node 0 1
> 0: 10 20
> 1: 20 10
> # cat /sys/bus/nd/devices/region0/numa_node
> 0
> # dmesg | grep papr_scm
> [ 91.332305] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Region registered with target node 2 and online node 0
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 30 +++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index c8ec670ee924..4abb0ecda30a 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -255,12 +255,32 @@ static const struct attribute_group *papr_scm_dimm_groups[] = {
> NULL,
> };
>
> +static inline int papr_scm_node(int node)
> +{
> + int min_dist = INT_MAX, dist;
> + int nid, min_node;
> +
> + if (node_online(node))
> + return node;
We should handle NUMA_NO_NODE here.
modified arch/powerpc/platforms/pseries/papr_scm.c
@@ -260,7 +260,7 @@ static inline int papr_scm_node(int node)
int min_dist = INT_MAX, dist;
int nid, min_node;
- if (node_online(node))
+ if ((node == NUMA_NO_NODE) || node_online(node))
return node;
min_node = first_online_node;
Will send an updated patch.
> +
> + min_node = first_online_node;
> + for_each_online_node(nid) {
> + dist = node_distance(node, nid);
> + if (dist < min_dist) {
> + min_dist = dist;
> + min_node = nid;
> + }
> + }
> + return min_node;
> +}
> +
> static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> {
> struct device *dev = &p->pdev->dev;
> struct nd_mapping_desc mapping;
> struct nd_region_desc ndr_desc;
> unsigned long dimm_flags;
> + int target_nid, online_nid;
>
> p->bus_desc.ndctl = papr_scm_ndctl;
> p->bus_desc.module = THIS_MODULE;
> @@ -299,8 +319,11 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>
> memset(&ndr_desc, 0, sizeof(ndr_desc));
> ndr_desc.attr_groups = region_attr_groups;
> - ndr_desc.numa_node = dev_to_node(&p->pdev->dev);
> - ndr_desc.target_node = ndr_desc.numa_node;
> + target_nid = dev_to_node(&p->pdev->dev);
> + online_nid = papr_scm_node(target_nid);
> + set_dev_node(&p->pdev->dev, online_nid);
> + ndr_desc.numa_node = online_nid;
> + ndr_desc.target_node = target_nid;
> ndr_desc.res = &p->res;
> ndr_desc.of_node = p->dn;
> ndr_desc.provider_data = p;
> @@ -318,6 +341,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> ndr_desc.res, p->dn);
> goto err;
> }
> + if (target_nid != online_nid)
> + dev_info(dev, "Region registered with target node %d and online node %d",
> + target_nid, online_nid);
>
> return 0;
>
> --
> 2.21.0
^ permalink raw reply
* Re: [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges
From: Sam Bobroff @ 2019-07-16 3:53 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <20190715085612.8802-2-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]
On Mon, Jul 15, 2019 at 06:56:08PM +1000, Oliver O'Halloran wrote:
> At the point where we start inserting ranges into the EEH address cache the
> binding between pci_dev and eeh_dev has already been set up. Instead of
> consulting the pci_dn tree we can retrieve the eeh_dev directly using
> pci_dev_to_eeh_dev().
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
In fact the binding between pci_dev and eeh_dev is set up right before
the calls to eeh_addr_cache_insert_dev() (see eeh_addr_cache_build() and
eeh_add_device_tree_late()) so this looks clearly correct to me.
A few simple tests of EEH recovery still succeed as well.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/eeh_cache.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
> index 3204723..908ba69 100644
> --- a/arch/powerpc/kernel/eeh_cache.c
> +++ b/arch/powerpc/kernel/eeh_cache.c
> @@ -156,18 +156,10 @@ eeh_addr_cache_insert(struct pci_dev *dev, resource_size_t alo,
>
> static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
> {
> - struct pci_dn *pdn;
> struct eeh_dev *edev;
> int i;
>
> - pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> - if (!pdn) {
> - pr_warn("PCI: no pci dn found for dev=%s\n",
> - pci_name(dev));
> - return;
> - }
> -
> - edev = pdn_to_eeh_dev(pdn);
> + edev = pci_dev_to_eeh_dev(dev);
> if (!edev) {
> pr_warn("PCI: no EEH dev found for %s\n",
> pci_name(dev));
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment
From: Sam Bobroff @ 2019-07-16 3:54 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <20190715085612.8802-3-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 970 bytes --]
On Mon, Jul 15, 2019 at 06:56:09PM +1000, Oliver O'Halloran wrote:
> The EEH_ATTR_SHOW() helper is used to display fields from struct eeh_dev
> not struct pci_dn.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Good.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/eeh_sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 3fa04dd..6a2c2886f 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -30,7 +30,7 @@
> /**
> * EEH_SHOW_ATTR -- Create sysfs entry for eeh statistic
> * @_name: name of file in sysfs directory
> - * @_memb: name of member in struct pci_dn to access
> + * @_memb: name of member in struct eeh_dev to access
> * @_format: printf format for display
> *
> * All of the attributes look very similar, so just
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties
From: Sam Bobroff @ 2019-07-16 3:54 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <20190715085612.8802-4-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]
On Mon, Jul 15, 2019 at 06:56:10PM +1000, Oliver O'Halloran wrote:
> There are several EEH sysfs properties that only exists when the
> "ibm,is-open-sriov-pf" property appears in the device tree node of the PCI
> device. This used on pseries to indicate to the guest that the hypervisor
> allows the guest to configure the SR-IOV capability. Doing this requires
> some handshaking between the guest, hypervisor and userspace when a VF is
> EEH frozen which is why these properties exist.
>
> This is all dead code on non-pseries platforms so wrap it in an #ifdef
> CONFIG_PPC_PSERIES to make the dependency clearer.
Looks good and a test compile without pSeries platform support
configured seemed to work, so:
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> arch/powerpc/kernel/eeh_sysfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 6a2c2886f..3adf8cd 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -91,7 +91,7 @@ static ssize_t eeh_pe_state_store(struct device *dev,
>
> static DEVICE_ATTR_RW(eeh_pe_state);
>
> -#ifdef CONFIG_PCI_IOV
> +#if defined(CONFIG_PCI_IOV) && defined(CONFIG_PPC_PSERIES)
> static ssize_t eeh_notify_resume_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -148,7 +148,7 @@ static void eeh_notify_resume_remove(struct pci_dev *pdev)
> #else
> static inline int eeh_notify_resume_add(struct pci_dev *pdev) { return 0; }
> static inline void eeh_notify_resume_remove(struct pci_dev *pdev) { }
> -#endif /* CONFIG_PCI_IOV */
> +#endif /* CONFIG_PCI_IOV && CONFIG PPC_PSERIES*/
>
> void eeh_sysfs_add_device(struct pci_dev *pdev)
> {
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup.
From: Sam Bobroff @ 2019-07-16 3:55 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <20190715085612.8802-5-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
On Mon, Jul 15, 2019 at 06:56:11PM +1000, Oliver O'Halloran wrote:
> In eeh_notify_resume_show() the pci_dn for the device is looked up once in
> the declaration block and then once after checking for a NULL eeh_dev.
> Remove the second lookup since it's pointless.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/eeh_sysfs.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 3adf8cd..c4cc8fc 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -102,7 +102,6 @@ static ssize_t eeh_notify_resume_show(struct device *dev,
> if (!edev || !edev->pe)
> return -ENODEV;
>
> - pdn = pci_get_pdn(pdev);
> return sprintf(buf, "%d\n", pdn->last_allow_rc);
> }
>
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner
From: Sam Bobroff @ 2019-07-16 4:00 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <20190715085612.8802-6-oohall@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]
On Mon, Jul 15, 2019 at 06:56:12PM +1000, Oliver O'Halloran wrote:
> The eeh_sysfs_remove_device() function is supposed to clear the
> EEH_DEV_SYSFS flag since it indicates the EEH sysfs entries have been added
> for a pci_dev.
>
> When the sysfs files are removed eeh_remove_device() the eeh_dev and the
> pci_dev have already been de-associated. This then causes the
> pci_dev_to_eeh_dev() call in eeh_sysfs_remove_device() to return NULL so
> the flag can't be cleared from the still-live eeh_dev. This problem is
> worked around in the caller by clearing the flag manually. However, this
> behaviour doesn't make a whole lot of sense, so this patch fixes it by:
>
> a) Re-ordering eeh_remove_device() so that eeh_sysfs_remove_device() is
> called before de-associating the pci_dev and eeh_dev.
>
> b) Making eeh_sysfs_remove_device() emit a warning if there's no
> corresponding eeh_dev for a pci_dev. The paths where the sysfs
> files are only reachable if EEH was setup for the device
> for the device in the first place so hitting this warning
> indicates a programming error.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
Good cleanup, although it looks like "for the device" got duplicated in
the last part of the commit message.
Simple EEH tests still succeeed.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>
> + if (!(edev->pe->state & EEH_PE_KEEP))
> + eeh_rmv_from_parent_pe(edev);
> + else
> + edev->mode |= EEH_DEV_DISCONNECTED;
> ---
> arch/powerpc/kernel/eeh.c | 30 +++++++++++++++++-------------
> arch/powerpc/kernel/eeh_sysfs.c | 15 ++++++++-------
> 2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index f192d57..6e24896 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1203,7 +1203,6 @@ void eeh_add_device_late(struct pci_dev *dev)
> eeh_rmv_from_parent_pe(edev);
> eeh_addr_cache_rmv_dev(edev->pdev);
> eeh_sysfs_remove_device(edev->pdev);
> - edev->mode &= ~EEH_DEV_SYSFS;
>
> /*
> * We definitely should have the PCI device removed
> @@ -1306,17 +1305,11 @@ void eeh_remove_device(struct pci_dev *dev)
> edev->pdev = NULL;
>
> /*
> - * The flag "in_error" is used to trace EEH devices for VFs
> - * in error state or not. It's set in eeh_report_error(). If
> - * it's not set, eeh_report_{reset,resume}() won't be called
> - * for the VF EEH device.
> + * eeh_sysfs_remove_device() uses pci_dev_to_eeh_dev() so we need to
> + * remove the sysfs files before clearing dev.archdata.edev
> */
> - edev->in_error = false;
> - dev->dev.archdata.edev = NULL;
> - if (!(edev->pe->state & EEH_PE_KEEP))
> - eeh_rmv_from_parent_pe(edev);
> - else
> - edev->mode |= EEH_DEV_DISCONNECTED;
> + if (edev->mode & EEH_DEV_SYSFS)
> + eeh_sysfs_remove_device(dev);
>
> /*
> * We're removing from the PCI subsystem, that means
> @@ -1327,8 +1320,19 @@ void eeh_remove_device(struct pci_dev *dev)
> edev->mode |= EEH_DEV_NO_HANDLER;
>
> eeh_addr_cache_rmv_dev(dev);
> - eeh_sysfs_remove_device(dev);
> - edev->mode &= ~EEH_DEV_SYSFS;
> +
> + /*
> + * The flag "in_error" is used to trace EEH devices for VFs
> + * in error state or not. It's set in eeh_report_error(). If
> + * it's not set, eeh_report_{reset,resume}() won't be called
> + * for the VF EEH device.
> + */
> + edev->in_error = false;
> + dev->dev.archdata.edev = NULL;
> + if (!(edev->pe->state & EEH_PE_KEEP))
> + eeh_rmv_from_parent_pe(edev);
> + else
> + edev->mode |= EEH_DEV_DISCONNECTED;
> }
>
> int eeh_unfreeze_pe(struct eeh_pe *pe)
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index c4cc8fc..5614fd83 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -175,22 +175,23 @@ void eeh_sysfs_remove_device(struct pci_dev *pdev)
> {
> struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>
> + if (!edev) {
> + WARN_ON(eeh_enabled());
> + return;
> + }
> +
> + edev->mode &= ~EEH_DEV_SYSFS;
> +
> /*
> * The parent directory might have been removed. We needn't
> * continue for that case.
> */
> - if (!pdev->dev.kobj.sd) {
> - if (edev)
> - edev->mode &= ~EEH_DEV_SYSFS;
> + if (!pdev->dev.kobj.sd)
> return;
> - }
>
> device_remove_file(&pdev->dev, &dev_attr_eeh_mode);
> device_remove_file(&pdev->dev, &dev_attr_eeh_pe_config_addr);
> device_remove_file(&pdev->dev, &dev_attr_eeh_pe_state);
>
> eeh_notify_resume_remove(pdev);
> -
> - if (edev)
> - edev->mode &= ~EEH_DEV_SYSFS;
> }
> --
> 2.9.5
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH V3] cpufreq: Make cpufreq_generic_init() return void
From: Viresh Kumar @ 2019-07-16 4:06 UTC (permalink / raw)
To: Rafael Wysocki, Markus Mayer, bcm-kernel-feedback-list, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keguang Zhang, Jiaxun Yang, Kevin Hilman,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kukjin Kim, Krzysztof Kozlowski, Thierry Reding, Jonathan Hunter
Cc: linux-samsung-soc, Vincent Guittot, linux-pm, Viresh Kumar,
linux-mips, linux-kernel, linux-tegra, linux-omap, linuxppc-dev,
linux-arm-kernel
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 <viresh.kumar@linaro.org>
---
V2->V3:
- Update bmips cpufreq driver to avoid "warning: 'ret' may be used
uninitialized".
- Build bot reported this issue almost after 4 days of posting this
patch, I was expecting this a lot earlier :)
drivers/cpufreq/bmips-cpufreq.c | 17 ++++++-----------
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, 46 insertions(+), 71 deletions(-)
diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..f7c23fa468f0 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -131,23 +131,18 @@ static int bmips_cpufreq_exit(struct cpufreq_policy *policy)
static int bmips_cpufreq_init(struct cpufreq_policy *policy)
{
struct cpufreq_frequency_table *freq_table;
- int ret;
freq_table = bmips_cpufreq_get_freq_table(policy);
if (IS_ERR(freq_table)) {
- ret = PTR_ERR(freq_table);
- pr_err("%s: couldn't determine frequency table (%d).\n",
- BMIPS_CPUFREQ_NAME, ret);
- return ret;
+ pr_err("%s: couldn't determine frequency table (%ld).\n",
+ BMIPS_CPUFREQ_NAME, PTR_ERR(freq_table));
+ return PTR_ERR(freq_table);
}
- 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;
+ return 0;
}
static struct cpufreq_driver bmips_cpufreq_driver = {
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
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1-cpufreq.c
@@ -81,7 +81,7 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
struct device *cpu_dev = get_cpu_device(policy->cpu);
struct cpufreq_frequency_table *freq_tbl;
unsigned int pll_freq, freq;
- int steps, i, ret;
+ int steps, i;
pll_freq = clk_get_rate(cpufreq->pll_clk) / 1000;
@@ -103,11 +103,9 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
freq_tbl[i].frequency = CPUFREQ_TABLE_END;
policy->clk = cpufreq->clk;
- ret = cpufreq_generic_init(policy, freq_tbl, 0);
- if (ret)
- kfree(freq_tbl);
+ cpufreq_generic_init(policy, freq_tbl, 0);
- return ret;
+ return 0;
}
static int ls1x_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/loongson2_cpufreq.c b/drivers/cpufreq/loongson2_cpufreq.c
index da344696beed..890813e0bb76 100644
--- a/drivers/cpufreq/loongson2_cpufreq.c
+++ b/drivers/cpufreq/loongson2_cpufreq.c
@@ -95,7 +95,8 @@ static int loongson2_cpufreq_cpu_init(struct cpufreq_policy *policy)
}
policy->clk = cpuclk;
- return cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ return 0;
}
static int loongson2_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/maple-cpufreq.c b/drivers/cpufreq/maple-cpufreq.c
index f5220b3d4ec5..28d346062166 100644
--- a/drivers/cpufreq/maple-cpufreq.c
+++ b/drivers/cpufreq/maple-cpufreq.c
@@ -140,7 +140,8 @@ static unsigned int maple_cpufreq_get_speed(unsigned int cpu)
static int maple_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ return 0;
}
static struct cpufreq_driver maple_cpufreq_driver = {
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 29643f06a3c3..8d14b42a8c6f 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -122,23 +122,18 @@ static int omap_cpu_init(struct cpufreq_policy *policy)
dev_err(mpu_dev,
"%s: cpu%d: failed creating freq table[%d]\n",
__func__, policy->cpu, result);
- goto fail;
+ clk_put(policy->clk);
+ return result;
}
}
atomic_inc_return(&freq_table_users);
/* FIXME: what's the actual transition time? */
- result = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (!result) {
- dev_pm_opp_of_register_em(policy->cpus);
- return 0;
- }
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
+ dev_pm_opp_of_register_em(policy->cpus);
- freq_table_free();
-fail:
- clk_put(policy->clk);
- return result;
+ return 0;
}
static int omap_cpu_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 6b1e4abe3248..93f39a1d4c3d 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -196,7 +196,8 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
policy->cur = pas_freqs[cur_astate].frequency;
ppc_proc_freq = policy->cur * 1000ul;
- return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ return 0;
out_unmap_sdcpwr:
iounmap(sdcpwr_mapbase);
diff --git a/drivers/cpufreq/pmac32-cpufreq.c b/drivers/cpufreq/pmac32-cpufreq.c
index 650104d729f3..73621bc11976 100644
--- a/drivers/cpufreq/pmac32-cpufreq.c
+++ b/drivers/cpufreq/pmac32-cpufreq.c
@@ -372,7 +372,8 @@ static int pmac_cpufreq_target( struct cpufreq_policy *policy,
static int pmac_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ return 0;
}
static u32 read_gpio(struct device_node *np)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 1af3492a000d..d7542a106e6b 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -321,7 +321,8 @@ static unsigned int g5_cpufreq_get_speed(unsigned int cpu)
static int g5_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ return 0;
}
static struct cpufreq_driver g5_cpufreq_driver = {
diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
index f7ff1ed7fef1..106910351c41 100644
--- a/drivers/cpufreq/s3c2416-cpufreq.c
+++ b/drivers/cpufreq/s3c2416-cpufreq.c
@@ -447,21 +447,16 @@ static int s3c2416_cpufreq_driver_init(struct cpufreq_policy *policy)
/* Datasheet says PLL stabalisation time must be at least 300us,
* so but add some fudge. (reference in LOCKCON0 register description)
*/
- ret = cpufreq_generic_init(policy, s3c_freq->freq_table,
+ cpufreq_generic_init(policy, s3c_freq->freq_table,
(500 * 1000) + s3c_freq->regulator_latency);
- if (ret)
- goto err_freq_table;
-
register_reboot_notifier(&s3c2416_cpufreq_reboot_notifier);
return 0;
-err_freq_table:
#ifdef CONFIG_ARM_S3C2416_CPUFREQ_VCORESCALE
- regulator_put(s3c_freq->vddarm);
err_vddarm:
-#endif
clk_put(s3c_freq->armclk);
+#endif
err_armclk:
clk_put(s3c_freq->hclk);
err_hclk:
diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index 37df2d892eb0..af0c00dabb22 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -144,7 +144,6 @@ static void s3c64xx_cpufreq_config_regulator(void)
static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
{
- int ret;
struct cpufreq_frequency_table *freq;
if (policy->cpu != 0)
@@ -165,8 +164,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
#ifdef CONFIG_REGULATOR
vddarm = regulator_get(NULL, "vddarm");
if (IS_ERR(vddarm)) {
- ret = PTR_ERR(vddarm);
- pr_err("Failed to obtain VDDARM: %d\n", ret);
+ pr_err("Failed to obtain VDDARM: %ld\n", PTR_ERR(vddarm));
pr_err("Only frequency scaling available\n");
vddarm = NULL;
} else {
@@ -196,16 +194,9 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
* the PLLs, which we don't currently) is ~300us worst case,
* but add some fudge.
*/
- ret = cpufreq_generic_init(policy, s3c64xx_freq_table,
+ cpufreq_generic_init(policy, s3c64xx_freq_table,
(500 * 1000) + regulator_latency);
- if (ret != 0) {
- pr_err("Failed to configure frequency table: %d\n",
- ret);
- regulator_put(vddarm);
- clk_put(policy->clk);
- }
-
- return ret;
+ return 0;
}
static struct cpufreq_driver s3c64xx_cpufreq_driver = {
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index e5cb17d4be7b..5d10030f2560 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -541,7 +541,8 @@ static int s5pv210_cpu_init(struct cpufreq_policy *policy)
s5pv210_dram_conf[1].freq = clk_get_rate(dmc1_clk);
policy->suspend_freq = SLEEP_FREQ;
- return cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ return 0;
out_dmc1:
clk_put(dmc0_clk);
diff --git a/drivers/cpufreq/sa1100-cpufreq.c b/drivers/cpufreq/sa1100-cpufreq.c
index ab5cab93e638..5c075ef6adc0 100644
--- a/drivers/cpufreq/sa1100-cpufreq.c
+++ b/drivers/cpufreq/sa1100-cpufreq.c
@@ -181,7 +181,8 @@ static int sa1100_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1100_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
static struct cpufreq_driver sa1100_driver __refdata = {
diff --git a/drivers/cpufreq/sa1110-cpufreq.c b/drivers/cpufreq/sa1110-cpufreq.c
index dab54e051c0e..d9d04d935b3a 100644
--- a/drivers/cpufreq/sa1110-cpufreq.c
+++ b/drivers/cpufreq/sa1110-cpufreq.c
@@ -303,7 +303,8 @@ static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1110_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
/* sa1110_driver needs __refdata because it must remain after init registers
diff --git a/drivers/cpufreq/spear-cpufreq.c b/drivers/cpufreq/spear-cpufreq.c
index 4074e2615522..73bd8dc47074 100644
--- a/drivers/cpufreq/spear-cpufreq.c
+++ b/drivers/cpufreq/spear-cpufreq.c
@@ -153,8 +153,9 @@ static int spear_cpufreq_target(struct cpufreq_policy *policy,
static int spear_cpufreq_init(struct cpufreq_policy *policy)
{
policy->clk = spear_cpufreq.clk;
- return cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
+ cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
spear_cpufreq.transition_latency);
+ return 0;
}
static struct cpufreq_driver spear_cpufreq_driver = {
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 3c32cc7b0671..f84ecd22f488 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -118,17 +118,11 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
static int tegra_cpu_init(struct cpufreq_policy *policy)
{
struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
- int ret;
clk_prepare_enable(cpufreq->cpu_clk);
/* FIXME: what's the actual transition time? */
- ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (ret) {
- clk_disable_unprepare(cpufreq->cpu_clk);
- return ret;
- }
-
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
policy->clk = cpufreq->cpu_clk;
policy->suspend_freq = freq_table[0].frequency;
return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d757a56a74dc..536a049d7ecc 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -992,7 +992,7 @@ extern struct freq_attr *cpufreq_generic_attr[];
int cpufreq_table_validate_and_sort(struct cpufreq_policy *policy);
unsigned int cpufreq_generic_get(unsigned int cpu);
-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);
#endif /* _LINUX_CPUFREQ_H */
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* Re: [PATCH] PPC: Set reserved PCR bits
From: Joel Stanley @ 2019-07-16 5:46 UTC (permalink / raw)
To: Jordan Niethe
Cc: Alistair Popple, Michael Neuling, linuxppc-dev,
Suraj Jitindar Singh
In-Reply-To: <20190716025400.4475-1-jniethe5@gmail.com>
On Tue, 16 Jul 2019 at 02:55, Jordan Niethe <jniethe5@gmail.com> wrote:
>
> Currently the reserved bits of the Processor Compatibility Register
> (PCR) are cleared as per the Programming Note in Section 1.3.3 of the
> ISA. An update is planned for the ISA so that PCR reserved bits should
> be set. Set the reserved bits of the PCR as required.
>
> Acked-by: Alistair Popple <alistair@popple.id.au>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Tested-by: Joel Stanley <joel@jms.id.au>
I gave a powernv_defconfig build a spin in a qemu powernv machine.
Cheers,
Joel
^ permalink raw reply
* [PATCH v2] powerpc/mm/radix: Fix PTE/PMD fragment count for early page table mappings
From: Bharata B Rao @ 2019-07-16 6:32 UTC (permalink / raw)
To: linuxppc-dev; +Cc: sraithal, aneesh.kumar, npiggin, Bharata B Rao
We could potentially hit the following BUG_ON when memory hotplugged
before reboot is unplugged after reboot:
kernel BUG at arch/powerpc/mm/pgtable-frag.c:113!
remove_pagetable+0x594/0x6a0
(unreliable)
remove_pagetable+0x94/0x6a0
vmemmap_free+0x394/0x410
sparse_remove_one_section+0x26c/0x2e8
__remove_pages+0x428/0x540
arch_remove_memory+0xd0/0x170
__remove_memory+0xd4/0x1a0
dlpar_remove_lmb+0xbc/0x110
dlpar_memory+0xa80/0xd20
handle_dlpar_errorlog+0xa8/0x160
pseries_hp_work_fn+0x2c/0x60
process_one_work+0x46c/0x860
worker_thread+0x364/0x5e0
kthread+0x1b0/0x1c0
ret_from_kernel_thread+0x5c/0x68
This occurs because, during reboot-after-hotplug, the hotplugged
memory range gets initialized as regular memory and page
tables are setup using memblock allocator. This means that we
wouldn't have initialized the PMD or PTE fragment count for
those PMD or PTE pages.
This BUG_ON was hitting quite regularly until we changed the vmemmap
mapping size from 64K to 2M via the commit
'89a3496e0664 ("powerpc/mm/radix: Use the right page size for
vmemmap mapping")'
After this commit, the bug still exists but it is hard to hit now
because vmemmap array is now mapped at PMD level (level 3) and we would
have to unplug a huge amount of memory to free one PMD table.
A point to note here is that we encounter this bug only while removing
the mappings for vmemmap and not when unmapping the memory that we
are unplugging. This is because for the memory that is to be unplugged
we split the mapping into smaller 2M mappings and hence PMD and PTE
fragments will be initialized properly as it is no longer early mapping.
Fixing this includes 3 parts:
- Re-walk the init_mm page tables from mem_init() and initialize
the PMD and PTE fragment counts appropriately. So PMD and PTE
table pages allocated during early allocation will have a
fragment count of 1.
- Convert the pages from memblock pages to regular pages so that
they can be freed back to buddy allocator seamlessly. However
we do this for only PMD and PTE pages and not for PUD pages.
PUD pages are freed using kmem_cache_free() and we need to
identify memblock pages and free them differently.
- When we do early memblock based allocation of PMD and PUD pages,
allocate in PAGE_SIZE granularity so that we are sure the
complete page is used as pagetable page. PAGE_SIZE allocations will
have an implication on the amount of memory used for page tables,
an example of which is shown below:
Since we now do PAGE_SIZE allocations for both PUD table and
PMD table (Note that PTE table allocation is already of PAGE_SIZE),
we end up allocating more memory for the same amount of system RAM.
Here is an example of how much more we end up allocating for
page tables in case of 64T system RAM:
1. Mapping system RAM
With each PGD entry spanning 512G, 64TB RAM would need 128 entries
and hence 128 PUD tables. We use 1G mapping at PUD level (level 2)
With default PUD_TABLE_SIZE(4K), 128*4K=512K (8 64K pages)
With PAGE_SIZE(64K) allocations, 128*64K=8192K (128 64K pages)
2. Mapping struct pages (memmap)
64T RAM would need 64G for memmap with struct page size being 64B.
Since memmap array is mapped using 64K mappings, we would need
64 PUD entries or 64 PMD tables (level 3) in total.
With default PMD_TABLE_SIZE(4K), 64*4K=256K (4 64K pages)
With PAGE_SIZE(64K) allocations, 64*64K=4096K (64 64K pages)
There is no change in PTE table (level 4) allocation requirement as
early page table allocation is already using PAGE_SIZE PTE tables.
So essentially with this change we would use 180 64K pages
more for 64T system.
Reported-by: Srikanth Aithal <sraithal@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
v1: https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg152511.html
Changes in v2:
- Replaced [pgd,pud,pmd]_huge() with [pgd,pud,pmd]_is_leaf()
- Updated commit description
arch/powerpc/include/asm/book3s/64/pgalloc.h | 7 +-
arch/powerpc/include/asm/book3s/64/radix.h | 1 +
arch/powerpc/include/asm/sparsemem.h | 1 +
arch/powerpc/mm/book3s64/pgtable.c | 15 +++-
arch/powerpc/mm/book3s64/radix_pgtable.c | 79 +++++++++++++++++++-
arch/powerpc/mm/mem.c | 5 ++
6 files changed, 104 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgalloc.h b/arch/powerpc/include/asm/book3s/64/pgalloc.h
index d45e4449619f..282f6620c514 100644
--- a/arch/powerpc/include/asm/book3s/64/pgalloc.h
+++ b/arch/powerpc/include/asm/book3s/64/pgalloc.h
@@ -114,7 +114,12 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
static inline void pud_free(struct mm_struct *mm, pud_t *pud)
{
- kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), pud);
+ struct page *page = virt_to_page(pud);
+
+ if (PageReserved(page))
+ free_reserved_page(page);
+ else
+ kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), pud);
}
static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
index e04a839cb5b9..a0b43e122cde 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h
@@ -288,6 +288,7 @@ static inline unsigned long radix__get_tree_size(void)
#ifdef CONFIG_MEMORY_HOTPLUG
int radix__create_section_mapping(unsigned long start, unsigned long end, int nid);
int radix__remove_section_mapping(unsigned long start, unsigned long end);
+void radix__fixup_pgtable_fragments(void);
#endif /* CONFIG_MEMORY_HOTPLUG */
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
index 3192d454a733..e662f9232d35 100644
--- a/arch/powerpc/include/asm/sparsemem.h
+++ b/arch/powerpc/include/asm/sparsemem.h
@@ -15,6 +15,7 @@
#ifdef CONFIG_MEMORY_HOTPLUG
extern int create_section_mapping(unsigned long start, unsigned long end, int nid);
extern int remove_section_mapping(unsigned long start, unsigned long end);
+void fixup_pgtable_fragments(void);
#ifdef CONFIG_PPC_BOOK3S_64
extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index 85bc81abd286..e45105a53e7e 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -190,6 +190,13 @@ int __meminit remove_section_mapping(unsigned long start, unsigned long end)
return hash__remove_section_mapping(start, end);
}
+
+void fixup_pgtable_fragments(void)
+{
+ if (radix_enabled())
+ radix__fixup_pgtable_fragments();
+}
+
#endif /* CONFIG_MEMORY_HOTPLUG */
void __init mmu_partition_table_init(void)
@@ -330,6 +337,8 @@ void pmd_fragment_free(unsigned long *pmd)
static inline void pgtable_free(void *table, int index)
{
+ struct page *page;
+
switch (index) {
case PTE_INDEX:
pte_fragment_free(table, 0);
@@ -338,7 +347,11 @@ static inline void pgtable_free(void *table, int index)
pmd_fragment_free(table);
break;
case PUD_INDEX:
- kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), table);
+ page = virt_to_page(table);
+ if (PageReserved(page))
+ free_reserved_page(page);
+ else
+ kmem_cache_free(PGT_CACHE(PUD_CACHE_INDEX), table);
break;
#if defined(CONFIG_PPC_4K_PAGES) && defined(CONFIG_HUGETLB_PAGE)
/* 16M hugepd directory at pud level */
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index e92c6472a20c..09184c81e394 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -37,6 +37,81 @@
unsigned int mmu_pid_bits;
unsigned int mmu_base_pid;
+/*
+ * Since we know that this page is a memblock-allocated page,
+ * convert it into a normal page in addition to fixing the fragment
+ * count.
+ */
+static void fix_fragment_count(struct page *page)
+{
+ ClearPageReserved(page);
+ init_page_count(page);
+ adjust_managed_page_count(page, 1);
+ atomic_inc(&page->pt_frag_refcount);
+}
+
+static void fixup_pte_fragments(pmd_t *pmd)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PMD; i++, pmd++) {
+ pte_t *pte;
+
+ if (pmd_none(*pmd))
+ continue;
+ if (pmd_is_leaf(*pmd))
+ continue;
+
+ pte = pte_offset_kernel(pmd, 0);
+ if (!pte_none(*pte))
+ fix_fragment_count(virt_to_page(pte));
+ }
+}
+
+static void fixup_pmd_fragments(pud_t *pud)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PUD; i++, pud++) {
+ pmd_t *pmd;
+
+ if (pud_none(*pud))
+ continue;
+ if (pud_is_leaf(*pud))
+ continue;
+
+ pmd = pmd_offset(pud, 0);
+ if (!pmd_none(*pmd))
+ fix_fragment_count(virt_to_page(pmd));
+ fixup_pte_fragments(pmd);
+ }
+}
+
+/*
+ * Walk the init_mm page tables and fixup the PMD and PTE fragment
+ * counts. This allows the PUD, PMD and PTE pages to be freed
+ * back to buddy allocator properly during memory unplug.
+ */
+void radix__fixup_pgtable_fragments(void)
+{
+ int i;
+ pgd_t *pgd = pgd_offset_k(0UL);
+
+ spin_lock(&init_mm.page_table_lock);
+ for (i = 0; i < PTRS_PER_PGD; i++, pgd++) {
+ pud_t *pud;
+
+ if (pgd_none(*pgd))
+ continue;
+ if (pgd_is_leaf(*pgd))
+ continue;
+
+ pud = pud_offset(pgd, 0);
+ fixup_pmd_fragments(pud);
+ }
+ spin_unlock(&init_mm.page_table_lock);
+}
+
static int native_register_process_table(unsigned long base, unsigned long pg_sz,
unsigned long table_size)
{
@@ -85,7 +160,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
pgdp = pgd_offset_k(ea);
if (pgd_none(*pgdp)) {
- pudp = early_alloc_pgtable(PUD_TABLE_SIZE, nid,
+ pudp = early_alloc_pgtable(PAGE_SIZE, nid,
region_start, region_end);
pgd_populate(&init_mm, pgdp, pudp);
}
@@ -95,7 +170,7 @@ static int early_map_kernel_page(unsigned long ea, unsigned long pa,
goto set_the_pte;
}
if (pud_none(*pudp)) {
- pmdp = early_alloc_pgtable(PMD_TABLE_SIZE, nid,
+ pmdp = early_alloc_pgtable(PAGE_SIZE, nid,
region_start, region_end);
pud_populate(&init_mm, pudp, pmdp);
}
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 096c87836e29..dac60f4c75a9 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -56,6 +56,10 @@
#include <mm/mmu_decl.h>
+void __weak fixup_pgtable_fragments(void)
+{
+}
+
#ifndef CPU_FTR_COHERENT_ICACHE
#define CPU_FTR_COHERENT_ICACHE 0 /* XXX for now */
#define CPU_FTR_NOEXECUTE 0
@@ -282,6 +286,7 @@ void __init mem_init(void)
set_max_mapnr(max_pfn);
memblock_free_all();
+ fixup_pgtable_fragments();
#ifdef CONFIG_HIGHMEM
{
unsigned long pfn, highmem_mapnr;
--
2.21.0
^ permalink raw reply related
* Re: [EXTERNAL] Re: [PATCH v2 3/6] powerpc/eeh: Improve debug messages around device addition
From: Sam Bobroff @ 2019-07-16 6:48 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: Alexey Kardashevskiy, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <CAOSf1CFwj93TYGppJVU5djEe4TN6ezo36G=DxWbFU4buaCWM4g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]
On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:
> On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > On 19/06/2019 14:27, Sam Bobroff wrote:
> > > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> > >>
> > >> On 07/05/2019 14:30, Sam Bobroff wrote:
> > >>> Also remove useless comment.
> > >>>
> > >>> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > >>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >>> ---
> > *snip*
> > >
> > > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > > (using the PDN information to form the PCI address) is quite common
> > > across the EEH code, so I think rather than changing a couple of
> > > specific cases, I should do a separate cleanup patch and introduce
> > > something like pdn_debug(pdn, "...."). What do you think?
> >
> > I'd switch them all to already existing dev_dbg/pci_debug rather than
> > adding pdn_debug as imho it should not have been used in the first place
> > really...
> >
> > > (I don't know exactly when edev->pdev can be NULL.)
> >
> > ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> > know if it can or cannot be NULL :)
>
> As far as I can tell edev->pdev is NULL in two cases:
>
> 1. Before eeh_device_add_late() has been called on the pdev. The late
> part of the add maps the pdev to an edev and sets the pdev's edev
> pointer and vis a vis.
> 2. While recoverying EEH unaware devices. Unaware devices are
> destroyed and rescanned and the edev->pdev pointer is cleared by
> pcibios_device_release()
>
> In most of these cases it should be safe to use the pci_*() functions
> rather than making a new one up for printing pdns. In the cases where
> we might not have a PCI dev i'd make a new set of prints that take an
> EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
> rather than later.
>
> Oliver
I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and
eeh_add_device_late() to use dev_dbg() and post a new version.
For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no
pci_dev available yet and while it would be nice to use the eeh_dev
rather than the pdn, it doesn't seem to have the bus/device/fn
information we need. Am I missing something there? (The code in the
probe functions seems to get it from the pci_dn.)
If there isn't an easy way around this, would it therefore be reasonable
to just leave them open-coded as they are?
Cheers,
Sam.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [EXTERNAL] Re: [PATCH v2 3/6] powerpc/eeh: Improve debug messages around device addition
From: Oliver O'Halloran @ 2019-07-16 7:00 UTC (permalink / raw)
To: Sam Bobroff; +Cc: Alexey Kardashevskiy, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <20190716064827.GF3645@tungsten.ozlabs.ibm.com>
On Tue, 2019-07-16 at 16:48 +1000, Sam Bobroff wrote:
> On Thu, Jun 20, 2019 at 01:45:24PM +1000, Oliver O'Halloran wrote:
> > On Thu, Jun 20, 2019 at 12:40 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > On 19/06/2019 14:27, Sam Bobroff wrote:
> > > > On Tue, Jun 11, 2019 at 03:47:58PM +1000, Alexey Kardashevskiy wrote:
> > > > > On 07/05/2019 14:30, Sam Bobroff wrote:
> > > > > > Also remove useless comment.
> > > > > >
> > > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > > > > > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > > > ---
> > > *snip*
> > > > I can see that edev will be non-NULL here, but that pr_debug() pattern
> > > > (using the PDN information to form the PCI address) is quite common
> > > > across the EEH code, so I think rather than changing a couple of
> > > > specific cases, I should do a separate cleanup patch and introduce
> > > > something like pdn_debug(pdn, "...."). What do you think?
> > >
> > > I'd switch them all to already existing dev_dbg/pci_debug rather than
> > > adding pdn_debug as imho it should not have been used in the first place
> > > really...
> > >
> > > > (I don't know exactly when edev->pdev can be NULL.)
> > >
> > > ... and if you switch to dev_dbg/pci_debug, I think quite soon you'll
> > > know if it can or cannot be NULL :)
> >
> > As far as I can tell edev->pdev is NULL in two cases:
> >
> > 1. Before eeh_device_add_late() has been called on the pdev. The late
> > part of the add maps the pdev to an edev and sets the pdev's edev
> > pointer and vis a vis.
> > 2. While recoverying EEH unaware devices. Unaware devices are
> > destroyed and rescanned and the edev->pdev pointer is cleared by
> > pcibios_device_release()
> >
> > In most of these cases it should be safe to use the pci_*() functions
> > rather than making a new one up for printing pdns. In the cases where
> > we might not have a PCI dev i'd make a new set of prints that take an
> > EEH dev rather than a pci_dn since i'd like pci_dn to die sooner
> > rather than later.
> >
> > Oliver
>
> I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and
> eeh_add_device_late() to use dev_dbg() and post a new version.
>
> For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no
> pci_dev available yet and while it would be nice to use the eeh_dev
> rather than the pdn, it doesn't seem to have the bus/device/fn
> information we need. Am I missing something there? (The code in the
> probe functions seems to get it from the pci_dn.)
We do have a pci_dev in the powernv case since pnv_eeh_probe() isn't
called until the late probe happens (which is after the pci_dev has
been created). I've got some patches to rework the probe path to make
this a bit clearer, but they need a bit more work.
>
> If there isn't an easy way around this, would it therefore be reasonable
> to just leave them open-coded as they are?
I've had this patch floating around a while that should do the trick.
The PCI_BUSNO macro is probably unnecessary since I'm sure there is
something that does it in generic code, but I couldn't find it.
From 61ff8c23c4d13ff640fb2d069dcedacdf2ee22dd Mon Sep 17 00:00:00 2001
From: Oliver O'Halloran <oohall@gmail.com>
Date: Thu, 18 Apr 2019 18:25:13 +1000
Subject: [PATCH] powerpc/eeh: Add bdfn field to eeh_dev
Preperation for removing pci_dn from the powernv EEH code. The only thing we
really use pci_dn for is to get the bdfn of the device for config space
accesses, so adding that information to eeh_dev reduces the need to carry
around the pci_dn.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/include/asm/eeh.h | 2 ++
arch/powerpc/include/asm/ppc-pci.h | 2 ++
arch/powerpc/kernel/eeh_dev.c | 2 ++
3 files changed, 6 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 7fd476d..a208e02 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -131,6 +131,8 @@ static inline bool eeh_pe_passed(struct eeh_pe *pe)
struct eeh_dev {
int mode; /* EEH mode */
int class_code; /* Class code of the device */
+ int bdfn; /* bdfn of device (for cfg ops) */
+ struct pci_controller *controller;
int pe_config_addr; /* PE config address */
u32 config_space[16]; /* Saved PCI config space */
int pcix_cap; /* Saved PCIx capability */
diff --git a/arch/powerpc/include/asm/ppc-pci.h b/arch/powerpc/include/asm/ppc-pci.h
index cec2d64..72860de 100644
--- a/arch/powerpc/include/asm/ppc-pci.h
+++ b/arch/powerpc/include/asm/ppc-pci.h
@@ -74,6 +74,8 @@ static inline const char *eeh_driver_name(struct pci_dev *pdev)
#endif /* CONFIG_EEH */
+#define PCI_BUSNO(bdfn) ((bdfn >> 8) & 0xff)
+
#else /* CONFIG_PCI */
static inline void init_pci_config_tokens(void) { }
#endif /* !CONFIG_PCI */
diff --git a/arch/powerpc/kernel/eeh_dev.c b/arch/powerpc/kernel/eeh_dev.c
index c4317c4..7370185 100644
--- a/arch/powerpc/kernel/eeh_dev.c
+++ b/arch/powerpc/kernel/eeh_dev.c
@@ -47,6 +47,8 @@ struct eeh_dev *eeh_dev_init(struct pci_dn *pdn)
/* Associate EEH device with OF node */
pdn->edev = edev;
edev->pdn = pdn;
+ edev->bdfn = (pdn->busno << 8) | pdn->devfn;
+ edev->controller = pdn->phb;
return edev;
}
--
2.9.5
^ permalink raw reply related
* Re: [PATCH] powerpc: remove meaningless KBUILD_ARFLAGS addition
From: Masahiro Yamada @ 2019-07-16 7:14 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Stephen Rothwell, Linux Kernel Mailing List, Nicholas Piggin,
Paul Mackerras, linuxppc-dev
In-Reply-To: <20190715181618.GG20882@gate.crashing.org>
On Tue, Jul 16, 2019 at 3:16 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Mon, Jul 15, 2019 at 09:03:46PM +0900, Masahiro Yamada wrote:
> > On Mon, Jul 15, 2019 at 4:30 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 05:05:34PM +1000, Michael Ellerman wrote:
> > > > Segher Boessenkool <segher@kernel.crashing.org> writes:
> > > > > 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.
> >
> > I use binutils 2.30
> > It does not understand --target option.
> >
> > $ powerpc-linux-ar --version
> > GNU ar (GNU Binutils) 2.30
> > Copyright (C) 2018 Free Software Foundation, Inc.
> > This program is free software; you may redistribute it under the terms of
> > the GNU General Public License version 3 or (at your option) any later version.
> > This program has absolutely no warranty.
> >
> > If I give --target=elf$(BITS)-$(GNUTARGET) option, I see this:
> > powerpc-linux-ar: -t: No such file or directory
>
> You need to provide a valid command line, like
>
> $ powerpc-linux-ar tv smth.a --target=elf32-powerpc
>
> ar is a bit weird.
Ah, I see!
I had missed the space being required.
Since I cannot test old binutils,
I will leave this to ppc people.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v9 05/10] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-07-16 8:03 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Tetsuo Handa, Alexei Starovoitov,
linux-kernel, David Howells, linux-kselftest, sparclinux,
Shuah Khan, raven, linux-arch, linux-s390, Tycho Andersen, paul,
Aleksa Sarai, linux-arm-kernel, linux-mips, linux-xtensa,
Kees Cook, Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, rgb, linux-api, Chanho Min,
Jeff Layton, Oleg Nesterov, Eric Biederman, linux-alpha,
linux-fsdevel, Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190714035826.GQ17978@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]
On 2019-07-14, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 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?
In addition, LOOKUP_NO_EVAL == LOOKUP_OPEN (0x100). Is that meant to be
the case? Also I just saw you have a patch in work.namei that fixes this
up -- do you want me to rebase on top of that?
--
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 v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
From: wen.yang99 @ 2019-07-16 8:26 UTC (permalink / raw)
To: viresh.kumar
Cc: wang.yi59, linux-pm, rjw, linux-kernel, xue.zhihong,
cheng.shengyu, linuxppc-dev
In-Reply-To: <20190712034409.zyl6sskrr6ra5nd3@vireshk-i7>
[-- Attachment #1.1: Type: text/plain, Size: 3497 bytes --]
> > The cpu variable is still being used in the of_get_property() call
> > after the of_node_put() call, which may result in use-after-free.
> >
> > Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux-pm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined but not used.
> > v5: put together the code to get, use, and release cpu device_node.
> > v4: restore the blank line.
> > v3: fix a leaked reference.
> > v2: clean up the code according to the advice of viresh.
> >
> > drivers/cpufreq/pasemi-cpufreq.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
> > index 6b1e4ab..7d557f9 100644
> > --- a/drivers/cpufreq/pasemi-cpufreq.c
> > +++ b/drivers/cpufreq/pasemi-cpufreq.c
> > @@ -131,10 +131,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > int err = -ENODEV;
> >
> > cpu = of_get_cpu_node(policy->cpu, NULL);
> > + if (!cpu)
> > + goto out;
> >
> > + max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > of_node_put(cpu);
> > - if (!cpu)
> > + if (!max_freqp) {
> > + err = -EINVAL;
> > goto out;
> > + }
> > +
> > + /* we need the freq in kHz */
> > + max_freq = *max_freqp / 1000;
> >
> > dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
> > if (!dn)
> > @@ -171,16 +179,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > }
> >
> > pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> > -
> > - max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> > - if (!max_freqp) {
> > - err = -EINVAL;
> > - goto out_unmap_sdcpwr;
> > - }
> > -
> > - /* we need the freq in kHz */
> > - max_freq = *max_freqp / 1000;
> > -
> > pr_debug("max clock-frequency is at %u kHz\n", max_freq);
> > pr_debug("initializing frequency table\n");
> >
> > @@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > policy->cur = pas_freqs[cur_astate].frequency;
> > ppc_proc_freq = policy->cur * 1000ul;
> >
> > - return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
> > + err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>
> So you are trying to fix an earlier issue here with this. Should have
> been a separate patch. Over that I have just sent a patch now to make
> this routine return void.
>
> https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org/
>
> So all you need to do is to remove the label out_unmap_sdcpwr instead.
Okay thank you.
Now this patch
(https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org/)
seems to have not been merged into the linux-next.
In order to avoid code conflicts, we will wait until this patch is merged in and then send v7.
--
Thanks and regards,
Wen
> > + if (err)
> > + goto out_unmap_sdcpwr;
> > +
> > + return 0;
> >
> > out_unmap_sdcpwr:
> > iounmap(sdcpwr_mapbase);
> > --
> > 2.9.5
^ permalink raw reply
* Re: [PATCH v3 10/11] mm/memory_hotplug: Make unregister_memory_block_under_nodes() never fail
From: Oscar Salvador @ 2019-07-16 8:46 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-s390, linux-ia64, linux-sh, Greg Kroah-Hartman, Mark Brown,
Rafael J. Wysocki, linux-kernel, Chris Wilson, Michal Hocko,
linux-mm, David S. Miller, Jonathan Cameron, Alex Deucher,
Igor Mammedov, akpm, Wei Yang, linuxppc-dev, Dan Williams,
linux-arm-kernel
In-Reply-To: <d450488d-7a82-f7a9-c8d3-b69a0bca48c6@redhat.com>
On Mon, Jul 15, 2019 at 01:10:33PM +0200, David Hildenbrand wrote:
> On 01.07.19 12:27, Michal Hocko wrote:
> > On Mon 01-07-19 11:36:44, Oscar Salvador wrote:
> >> On Mon, Jul 01, 2019 at 10:51:44AM +0200, Michal Hocko wrote:
> >>> Yeah, we do not allow to offline multi zone (node) ranges so the current
> >>> code seems to be over engineered.
> >>>
> >>> Anyway, I am wondering why do we have to strictly check for already
> >>> removed nodes links. Is the sysfs code going to complain we we try to
> >>> remove again?
> >>
> >> No, sysfs will silently "fail" if the symlink has already been removed.
> >> At least that is what I saw last time I played with it.
> >>
> >> I guess the question is what if sysfs handling changes in the future
> >> and starts dropping warnings when trying to remove a symlink is not there.
> >> Maybe that is unlikely to happen?
> >
> > And maybe we handle it then rather than have a static allocation that
> > everybody with hotremove configured has to pay for.
> >
>
> So what's the suggestion? Dropping the nodemask_t completely and calling
> sysfs_remove_link() on already potentially removed links?
>
> Of course, we can also just use mem_blk->nid and rest assured that it
> will never be called for memory blocks belonging to multiple nodes.
Hi David,
While it is easy to construct a scenario where a memblock belongs to multiple
nodes, I have to confess that I yet have not seen that in a real-world scenario.
Given said that, I think that the less risky way is to just drop the nodemask_t
and do not care about calling sysfs_remove_link() for already removed links.
As I said, sysfs_remove_link() will silently fail when it fails to find the
symlink, so I do not think it is a big deal.
--
Oscar Salvador
SUSE L3
^ permalink raw reply
* [Bug 203647] Locking API testsuite fails "mixed read-lock/lock-write ABBA" rlock on kernels >=4.14.x
From: bugzilla-daemon @ 2019-07-16 8:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-203647-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=203647
Anatoly Pugachev (matorola@gmail.com) changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |matorola@gmail.com
--- Comment #7 from Anatoly Pugachev (matorola@gmail.com) ---
it's the same for sparc64:
`
[ 0.000020] PROMLIB: Sun IEEE Boot Prom 'OBP 4.38.12 2018/03/28 14:54'
[ 0.000032] PROMLIB: Root node compatible: sun4v
[ 0.000080] Linux version 5.2.0-10808-g9637d517347e (mator@ttip) (gcc
version 8.3.0 (Debian 8.3.0-7)) #1080 SMP Tue Jul 16 10:46:19 MSK 2019
[ 0.000386] printk: bootconsole [earlyprom0] enabled
[ 0.000441] ARCH: SUN4V
...
[ 0.451068] memory used by lock dependency info: 3855 kB
[ 0.451104] per task-struct memory footprint: 1920 bytes
[ 0.451140] ------------------------
[ 0.451167] | Locking API testsuite:
[ 0.451194]
----------------------------------------------------------------------------
[ 0.451244] | spin |wlock |rlock |mutex |
wsem | rsem |
[ 0.451294]
--------------------------------------------------------------------------
[ 0.451350] A-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.454281] A-B-B-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.457443] A-B-B-C-C-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.460744] A-B-C-A-B-C deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.464032] A-B-B-C-C-D-D-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.467545] A-B-C-D-B-D-D-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.471009] A-B-C-D-B-C-D-A deadlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.474475] double unlock: ok | ok | ok | ok |
ok | ok | ok |
[ 0.477425] initialize held: ok | ok | ok | ok |
ok | ok | ok |
[ 0.480301]
--------------------------------------------------------------------------
[ 0.480352] recursive read-lock: | ok |
| ok |
[ 0.481247] recursive read-lock #2: | ok |
| ok |
[ 0.482120] mixed read-write-lock: | ok |
| ok |
[ 0.482998] mixed write-read-lock: | ok |
| ok |
[ 0.483878] mixed read-lock/lock-write ABBA: |FAILED|
| ok |
[ 0.484755] mixed read-lock/lock-read ABBA: | ok |
| ok |
[ 0.485676] mixed write-lock/lock-write ABBA: | ok |
| ok |
[ 0.486597]
--------------------------------------------------------------------------
`
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
From: Viresh Kumar @ 2019-07-16 9:00 UTC (permalink / raw)
To: wen.yang99
Cc: wang.yi59, linux-pm, rjw, linux-kernel, xue.zhihong,
cheng.shengyu, linuxppc-dev
In-Reply-To: <201907161626465333445@zte.com.cn>
On 16-07-19, 16:26, wen.yang99@zte.com.cn wrote:
> Okay thank you.
> Now this patch
> (https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org/)
> seems to have not been merged into the linux-next.
>
> In order to avoid code conflicts, we will wait until this patch is merged in and then send v7.
Please rebase on PM tree's linux-next branch instead and resend your
patch.
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
--
viresh
^ permalink raw reply
* [PATCH v2] powerpc/nvdimm: Pick the nearby online node if the device node is not online
From: Aneesh Kumar K.V @ 2019-07-16 9:05 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: Aneesh Kumar K.V, linuxppc-dev
This is similar to what ACPI does. Nvdimm layer doesn't bring the SCM device
numa node online. Hence we need to make sure we always use an online node
as ndr_desc.numa_node. Otherwise this result in kernel crashes. The target
node is used by dax/kmem and that will bring up the numa node online correctly.
Without this patch, we do hit kernel crash as below because we try to access
uninitialized NODE_DATA in different code paths.
cpu 0x0: Vector: 300 (Data Access) at [c0000000fac53170]
pc: c0000000004bbc50: ___slab_alloc+0x120/0xca0
lr: c0000000004bc834: __slab_alloc+0x64/0xc0
sp: c0000000fac53400
msr: 8000000002009033
dar: 73e8
dsisr: 80000
current = 0xc0000000fabb6d80
paca = 0xc000000003870000 irqmask: 0x03 irq_happened: 0x01
pid = 7, comm = kworker/u16:0
Linux version 5.2.0-06234-g76bd729b2644 (kvaneesh@ltc-boston123) (gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)) #135 SMP Thu Jul 11 05:36:30 CDT 2019
enter ? for help
[link register ] c0000000004bc834 __slab_alloc+0x64/0xc0
[c0000000fac53400] c0000000fac53480 (unreliable)
[c0000000fac53500] c0000000004bc818 __slab_alloc+0x48/0xc0
[c0000000fac53560] c0000000004c30a0 __kmalloc_node_track_caller+0x3c0/0x6b0
[c0000000fac535d0] c000000000cfafe4 devm_kmalloc+0x74/0xc0
[c0000000fac53600] c000000000d69434 nd_region_activate+0x144/0x560
[c0000000fac536d0] c000000000d6b19c nd_region_probe+0x17c/0x370
[c0000000fac537b0] c000000000d6349c nvdimm_bus_probe+0x10c/0x230
[c0000000fac53840] c000000000cf3cc4 really_probe+0x254/0x4e0
[c0000000fac538d0] c000000000cf429c driver_probe_device+0x16c/0x1e0
[c0000000fac53950] c000000000cf0b44 bus_for_each_drv+0x94/0x130
[c0000000fac539b0] c000000000cf392c __device_attach+0xdc/0x200
[c0000000fac53a50] c000000000cf231c bus_probe_device+0x4c/0xf0
[c0000000fac53a90] c000000000ced268 device_add+0x528/0x810
[c0000000fac53b60] c000000000d62a58 nd_async_device_register+0x28/0xa0
[c0000000fac53bd0] c0000000001ccb8c async_run_entry_fn+0xcc/0x1f0
[c0000000fac53c50] c0000000001bcd9c process_one_work+0x46c/0x860
[c0000000fac53d20] c0000000001bd4f4 worker_thread+0x364/0x5f0
[c0000000fac53db0] c0000000001c7260 kthread+0x1b0/0x1c0
[c0000000fac53e20] c00000000000b954 ret_from_kernel_thread+0x5c/0x68
With the patch we get
# numactl -H
available: 2 nodes (0-1)
node 0 cpus:
node 0 size: 0 MB
node 0 free: 0 MB
node 1 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
node 1 size: 130865 MB
node 1 free: 129130 MB
node distances:
node 0 1
0: 10 20
1: 20 10
# cat /sys/bus/nd/devices/region0/numa_node
0
# dmesg | grep papr_scm
[ 91.332305] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Region registered with target node 2 and online node 0
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
changes from V1:
* handle NUMA_NO_NODE
arch/powerpc/platforms/pseries/papr_scm.c | 30 +++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index c8ec670ee924..b813bc92f35f 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -255,12 +255,32 @@ static const struct attribute_group *papr_scm_dimm_groups[] = {
NULL,
};
+static inline int papr_scm_node(int node)
+{
+ int min_dist = INT_MAX, dist;
+ int nid, min_node;
+
+ if ((node == NUMA_NO_NODE) || node_online(node))
+ return node;
+
+ min_node = first_online_node;
+ for_each_online_node(nid) {
+ dist = node_distance(node, nid);
+ if (dist < min_dist) {
+ min_dist = dist;
+ min_node = nid;
+ }
+ }
+ return min_node;
+}
+
static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
{
struct device *dev = &p->pdev->dev;
struct nd_mapping_desc mapping;
struct nd_region_desc ndr_desc;
unsigned long dimm_flags;
+ int target_nid, online_nid;
p->bus_desc.ndctl = papr_scm_ndctl;
p->bus_desc.module = THIS_MODULE;
@@ -299,8 +319,11 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
memset(&ndr_desc, 0, sizeof(ndr_desc));
ndr_desc.attr_groups = region_attr_groups;
- ndr_desc.numa_node = dev_to_node(&p->pdev->dev);
- ndr_desc.target_node = ndr_desc.numa_node;
+ target_nid = dev_to_node(&p->pdev->dev);
+ online_nid = papr_scm_node(target_nid);
+ set_dev_node(&p->pdev->dev, online_nid);
+ ndr_desc.numa_node = online_nid;
+ ndr_desc.target_node = target_nid;
ndr_desc.res = &p->res;
ndr_desc.of_node = p->dn;
ndr_desc.provider_data = p;
@@ -318,6 +341,9 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
ndr_desc.res, p->dn);
goto err;
}
+ if (target_nid != online_nid)
+ dev_info(dev, "Region registered with target node %d and online node %d",
+ target_nid, online_nid);
return 0;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH kernel v2] powerpc/xive: Drop deregistered irqs
From: Alexey Kardashevskiy @ 2019-07-16 9:10 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev
Cc: Alistair Popple, Daniel Henrique Barboza, Greg Kurz,
Nicholas Piggin, Paul Mackerras, David Gibson
In-Reply-To: <303bfc1d-230b-76ed-8059-48951ee16426@kaod.org>
On 16/07/2019 18:59, Cédric Le Goater wrote:
> On 15/07/2019 09:11, Alexey Kardashevskiy wrote:
>> There is a race between releasing an irq on one cpu and fetching it
>> from XIVE on another cpu as there does not seem to be any locking between
>> these, probably because xive_irq_chip::irq_shutdown() is supposed to
>> remove the irq from all queues in the system which it does not do.
>>
>> As a result, when such released irq appears in a queue, we take it
>> from the queue but we do not change the current priority on that cpu and
>> since there is no handler for the irq, EOI is never called and the cpu
>> current priority remains elevated (7 vs. 0xff==unmasked). If another irq
>> is assigned to the same cpu, then that device stops working until irq
>> is moved to another cpu or the device is reset.
>>
>> This adds a new ppc_md.orphan_irq callback which is called if no irq
>> descriptor is found. The XIVE implementation drops the current priority
>> to 0xff which effectively unmasks interrupts in a current CPU.
>
>
> The test on generic_handle_irq() catches interrupt events that
> were served on a target CPU while the source interrupt was being
> shutdown on another CPU.
>
> The orphan_irq() handler restores the CPPR in such cases.
>
> This looks OK to me. I would have added some more comments in the
> code.
Which and where? Thanks,
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
>
> And adding to the list of future cleanups : a 'set_cppr' helper.
>
> Thanks,
>
> C.
>
>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * added ppc_md.orphan_irq
>>
>> ---
>>
>> Found it on P9 system with:
>> - a host with 8 cpus online
>> - a boot disk on ahci with its msix on cpu#0
>> - a guest with 2xGPUs + 6xNVLink + 4 cpus
>> - GPU#0 from the guest is bound to the same cpu#0.
>>
>> Killing a guest killed ahci and therefore the host because of the race.
>> Note that VFIO masks interrupts first and only then resets the device.
>> ---
>> arch/powerpc/include/asm/machdep.h | 3 +++
>> arch/powerpc/kernel/irq.c | 9 ++++++---
>> arch/powerpc/sysdev/xive/common.c | 10 ++++++++++
>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
>> index c43d6eca9edd..6cc14e28e89a 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -59,6 +59,9 @@ struct machdep_calls {
>> /* Return an irq, or 0 to indicate there are none pending. */
>> unsigned int (*get_irq)(void);
>>
>> + /* Drops irq if it does not have a valid descriptor */
>> + void (*orphan_irq)(unsigned int irq);
>> +
>> /* PCI stuff */
>> /* Called after allocating resources */
>> void (*pcibios_fixup)(void);
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index bc68c53af67c..b4e06d05bdba 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -632,10 +632,13 @@ void __do_irq(struct pt_regs *regs)
>> may_hard_irq_enable();
>>
>> /* And finally process it */
>> - if (unlikely(!irq))
>> + if (unlikely(!irq)) {
>> __this_cpu_inc(irq_stat.spurious_irqs);
>> - else
>> - generic_handle_irq(irq);
>> + } else if (generic_handle_irq(irq)) {
>> + if (ppc_md.orphan_irq)
>> + ppc_md.orphan_irq(irq);
>> + __this_cpu_inc(irq_stat.spurious_irqs);
>> + }
>>
>> trace_irq_exit(regs);
>>
>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>> index 082c7e1c20f0..b4054091999a 100644
>> --- a/arch/powerpc/sysdev/xive/common.c
>> +++ b/arch/powerpc/sysdev/xive/common.c
>> @@ -283,6 +283,15 @@ static unsigned int xive_get_irq(void)
>> return irq;
>> }
>>
>> +static void xive_orphan_irq(unsigned int irq)
>> +{
>> + struct xive_cpu *xc = __this_cpu_read(xive_cpu);
>> +
>> + xc->cppr = 0xff;
>> + out_8(xive_tima + xive_tima_offset + TM_CPPR, 0xff);
>> + DBG_VERBOSE("orphan_irq: irq %d, adjusting CPPR to 0xff\n", irq);
>> +}
>> +
>> /*
>> * After EOI'ing an interrupt, we need to re-check the queue
>> * to see if another interrupt is pending since multiple
>> @@ -1419,6 +1428,7 @@ bool __init xive_core_init(const struct xive_ops *ops, void __iomem *area, u32 o
>> xive_irq_priority = max_prio;
>>
>> ppc_md.get_irq = xive_get_irq;
>> + ppc_md.orphan_irq = xive_orphan_irq;
>> __xive_enabled = true;
>>
>> pr_devel("Initializing host..\n");
>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel v2] powerpc/xive: Drop deregistered irqs
From: Cédric Le Goater @ 2019-07-16 9:14 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Alistair Popple, Daniel Henrique Barboza, Greg Kurz,
Nicholas Piggin, Paul Mackerras, David Gibson
In-Reply-To: <b8722c86-317d-ba60-1b16-d848c86d0e71@ozlabs.ru>
On 16/07/2019 11:10, Alexey Kardashevskiy wrote:
>
>
> On 16/07/2019 18:59, Cédric Le Goater wrote:
>> On 15/07/2019 09:11, Alexey Kardashevskiy wrote:
>>> There is a race between releasing an irq on one cpu and fetching it
>>> from XIVE on another cpu as there does not seem to be any locking between
>>> these, probably because xive_irq_chip::irq_shutdown() is supposed to
>>> remove the irq from all queues in the system which it does not do.
>>>
>>> As a result, when such released irq appears in a queue, we take it
>>> from the queue but we do not change the current priority on that cpu and
>>> since there is no handler for the irq, EOI is never called and the cpu
>>> current priority remains elevated (7 vs. 0xff==unmasked). If another irq
>>> is assigned to the same cpu, then that device stops working until irq
>>> is moved to another cpu or the device is reset.
>>>
>>> This adds a new ppc_md.orphan_irq callback which is called if no irq
>>> descriptor is found. The XIVE implementation drops the current priority
>>> to 0xff which effectively unmasks interrupts in a current CPU.
>>
>>
>> The test on generic_handle_irq() catches interrupt events that
>> were served on a target CPU while the source interrupt was being
>> shutdown on another CPU.
>>
>> The orphan_irq() handler restores the CPPR in such cases.
>>
>> This looks OK to me. I would have added some more comments in the
>> code.
>
>
> Which and where? Thanks,
Above xive_orphan_irq() explaining the complete problem that we are
addressing. XIVE is not super obvious when looking at the code ...
C.
>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>>
>> And adding to the list of future cleanups : a 'set_cppr' helper.
>>
>> Thanks,
>>
>> C.
>>
>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * added ppc_md.orphan_irq
>>>
>>> ---
>>>
>>> Found it on P9 system with:
>>> - a host with 8 cpus online
>>> - a boot disk on ahci with its msix on cpu#0
>>> - a guest with 2xGPUs + 6xNVLink + 4 cpus
>>> - GPU#0 from the guest is bound to the same cpu#0.
>>>
>>> Killing a guest killed ahci and therefore the host because of the race.
>>> Note that VFIO masks interrupts first and only then resets the device.
>>> ---
>>> arch/powerpc/include/asm/machdep.h | 3 +++
>>> arch/powerpc/kernel/irq.c | 9 ++++++---
>>> arch/powerpc/sysdev/xive/common.c | 10 ++++++++++
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
>>> index c43d6eca9edd..6cc14e28e89a 100644
>>> --- a/arch/powerpc/include/asm/machdep.h
>>> +++ b/arch/powerpc/include/asm/machdep.h
>>> @@ -59,6 +59,9 @@ struct machdep_calls {
>>> /* Return an irq, or 0 to indicate there are none pending. */
>>> unsigned int (*get_irq)(void);
>>> + /* Drops irq if it does not have a valid descriptor */
>>> + void (*orphan_irq)(unsigned int irq);
>>> +
>>> /* PCI stuff */
>>> /* Called after allocating resources */
>>> void (*pcibios_fixup)(void);
>>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>>> index bc68c53af67c..b4e06d05bdba 100644
>>> --- a/arch/powerpc/kernel/irq.c
>>> +++ b/arch/powerpc/kernel/irq.c
>>> @@ -632,10 +632,13 @@ void __do_irq(struct pt_regs *regs)
>>> may_hard_irq_enable();
>>> /* And finally process it */
>>> - if (unlikely(!irq))
>>> + if (unlikely(!irq)) {
>>> __this_cpu_inc(irq_stat.spurious_irqs);
>>> - else
>>> - generic_handle_irq(irq);
>>> + } else if (generic_handle_irq(irq)) {
>>> + if (ppc_md.orphan_irq)
>>> + ppc_md.orphan_irq(irq);
>>> + __this_cpu_inc(irq_stat.spurious_irqs);
>>> + }
>>> trace_irq_exit(regs);
>>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>>> index 082c7e1c20f0..b4054091999a 100644
>>> --- a/arch/powerpc/sysdev/xive/common.c
>>> +++ b/arch/powerpc/sysdev/xive/common.c
>>> @@ -283,6 +283,15 @@ static unsigned int xive_get_irq(void)
>>> return irq;
>>> }
>>> +static void xive_orphan_irq(unsigned int irq)
>>> +{
>>> + struct xive_cpu *xc = __this_cpu_read(xive_cpu);
>>> +
>>> + xc->cppr = 0xff;
>>> + out_8(xive_tima + xive_tima_offset + TM_CPPR, 0xff);
>>> + DBG_VERBOSE("orphan_irq: irq %d, adjusting CPPR to 0xff\n", irq);
>>> +}
>>> +
>>> /*
>>> * After EOI'ing an interrupt, we need to re-check the queue
>>> * to see if another interrupt is pending since multiple
>>> @@ -1419,6 +1428,7 @@ bool __init xive_core_init(const struct xive_ops *ops, void __iomem *area, u32 o
>>> xive_irq_priority = max_prio;
>>> ppc_md.get_irq = xive_get_irq;
>>> + ppc_md.orphan_irq = xive_orphan_irq;
>>> __xive_enabled = true;
>>> pr_devel("Initializing host..\n");
>>>
>>
>
^ permalink raw reply
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