* [PATCH v2 5/6] KVM: PPC: Book3S HV: Use threads_per_subcore in KVM
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Michael Neuling, kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <1400832930-17050-1-git-send-email-mikey@neuling.org>
From: Michael Ellerman <mpe@ellerman.id.au>
To support split core on POWER8 we need to modify various parts of the
KVM code to use threads_per_subcore instead of threads_per_core. On
systems that do not support split core threads_per_subcore ==
threads_per_core and these changes are a nop.
We use threads_per_subcore as the value reported by KVM_CAP_PPC_SMT.
This communicates to userspace that guests can only be created with
a value of threads_per_core that is less than or equal to the current
threads_per_subcore. This ensures that guests can only be created with a
thread configuration that we are able to run given the current split
core mode.
Although threads_per_subcore can change during the life of the system,
the commit that enables that will ensure that threads_per_subcore does
not change during the life of a KVM VM.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Acked-by: Alexander Graf <agraf@suse.de>
Acked-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv.c | 26 ++++++++++++++++----------
arch/powerpc/kvm/powerpc.c | 2 +-
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index d7b74f8..5e86f28 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1266,7 +1266,7 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
int core;
struct kvmppc_vcore *vcore;
- core = id / threads_per_core;
+ core = id / threads_per_subcore;
if (core >= KVM_MAX_VCORES)
goto out;
@@ -1305,7 +1305,7 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
init_waitqueue_head(&vcore->wq);
vcore->preempt_tb = TB_NIL;
vcore->lpcr = kvm->arch.lpcr;
- vcore->first_vcpuid = core * threads_per_core;
+ vcore->first_vcpuid = core * threads_per_subcore;
vcore->kvm = kvm;
}
kvm->arch.vcores[core] = vcore;
@@ -1495,16 +1495,19 @@ static void kvmppc_wait_for_nap(struct kvmppc_vcore *vc)
static int on_primary_thread(void)
{
int cpu = smp_processor_id();
- int thr = cpu_thread_in_core(cpu);
+ int thr;
- if (thr)
+ /* Are we on a primary subcore? */
+ if (cpu_thread_in_subcore(cpu))
return 0;
- while (++thr < threads_per_core)
+
+ thr = 0;
+ while (++thr < threads_per_subcore)
if (cpu_online(cpu + thr))
return 0;
/* Grab all hw threads so they can't go into the kernel */
- for (thr = 1; thr < threads_per_core; ++thr) {
+ for (thr = 1; thr < threads_per_subcore; ++thr) {
if (kvmppc_grab_hwthread(cpu + thr)) {
/* Couldn't grab one; let the others go */
do {
@@ -1563,15 +1566,18 @@ static void kvmppc_run_core(struct kvmppc_vcore *vc)
}
/*
- * Make sure we are running on thread 0, and that
- * secondary threads are offline.
+ * Make sure we are running on primary threads, and that secondary
+ * threads are offline. Also check if the number of threads in this
+ * guest are greater than the current system threads per guest.
*/
- if (threads_per_core > 1 && !on_primary_thread()) {
+ if ((threads_per_core > 1) &&
+ ((vc->num_threads > threads_per_subcore) || !on_primary_thread())) {
list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list)
vcpu->arch.ret = -EBUSY;
goto out;
}
+
vc->pcpu = smp_processor_id();
list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
kvmppc_start_thread(vcpu);
@@ -1599,7 +1605,7 @@ static void kvmppc_run_core(struct kvmppc_vcore *vc)
/* wait for secondary threads to finish writing their state to memory */
if (vc->nap_count < vc->n_woken)
kvmppc_wait_for_nap(vc);
- for (i = 0; i < threads_per_core; ++i)
+ for (i = 0; i < threads_per_subcore; ++i)
kvmppc_release_hwthread(vc->pcpu + i);
/* prevent other vcpu threads from doing kvmppc_start_thread() now */
vc->vcore_state = VCORE_EXITING;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 3cf541a..27919a8 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -384,7 +384,7 @@ int kvm_dev_ioctl_check_extension(long ext)
#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
case KVM_CAP_PPC_SMT:
if (hv_enabled)
- r = threads_per_core;
+ r = threads_per_subcore;
else
r = 0;
break;
--
1.9.1
^ permalink raw reply related
* [PATCH v2 4/6] powerpc: Check cpu_thread_in_subcore() in __cpu_up()
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Michael Neuling, kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <1400832930-17050-1-git-send-email-mikey@neuling.org>
From: Michael Ellerman <mpe@ellerman.id.au>
To support split core we need to change the check in __cpu_up() that
determines if a cpu is allowed to come online.
Currently we refuse to online cpus which are not the primary thread
within their core.
On POWER8 with split core support this check needs to instead refuse to
online cpus which are not the primary thread within their *sub* core.
On POWER7 and other systems that do not support split core,
threads_per_subcore == threads_per_core and so the check is equivalent.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/smp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 6edae3d..b5222c4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -489,7 +489,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
* Don't allow secondary threads to come online if inhibited
*/
if (threads_per_core > 1 && secondaries_inhibited() &&
- cpu % threads_per_core != 0)
+ cpu_thread_in_subcore(cpu))
return -EBUSY;
if (smp_ops == NULL ||
--
1.9.1
^ permalink raw reply related
* [PATCH v2 3/6] powerpc: Add threads_per_subcore
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Michael Neuling, kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <1400832930-17050-1-git-send-email-mikey@neuling.org>
From: Michael Ellerman <mpe@ellerman.id.au>
On POWER8 we have a new concept of a subcore. This is what happens when
you take a regular core and split it. A subcore is a grouping of two or
four SMT threads, as well as a handfull of SPRs which allows the subcore
to appear as if it were a core from the point of view of a guest.
Unlike threads_per_core which is fixed at boot, threads_per_subcore can
change while the system is running. Most code will not want to use
threads_per_subcore.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/cputhreads.h | 7 +++++++
arch/powerpc/kernel/setup-common.c | 4 +++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/cputhreads.h b/arch/powerpc/include/asm/cputhreads.h
index ac3eedb..2bf8e93 100644
--- a/arch/powerpc/include/asm/cputhreads.h
+++ b/arch/powerpc/include/asm/cputhreads.h
@@ -18,10 +18,12 @@
#ifdef CONFIG_SMP
extern int threads_per_core;
+extern int threads_per_subcore;
extern int threads_shift;
extern cpumask_t threads_core_mask;
#else
#define threads_per_core 1
+#define threads_per_subcore 1
#define threads_shift 0
#define threads_core_mask (CPU_MASK_CPU0)
#endif
@@ -74,6 +76,11 @@ static inline int cpu_thread_in_core(int cpu)
return cpu & (threads_per_core - 1);
}
+static inline int cpu_thread_in_subcore(int cpu)
+{
+ return cpu & (threads_per_subcore - 1);
+}
+
static inline int cpu_first_thread_sibling(int cpu)
{
return cpu & ~(threads_per_core - 1);
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 3cf25c8..aa0f5ed 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -390,9 +390,10 @@ void __init check_for_initrd(void)
#ifdef CONFIG_SMP
-int threads_per_core, threads_shift;
+int threads_per_core, threads_per_subcore, threads_shift;
cpumask_t threads_core_mask;
EXPORT_SYMBOL_GPL(threads_per_core);
+EXPORT_SYMBOL_GPL(threads_per_subcore);
EXPORT_SYMBOL_GPL(threads_shift);
EXPORT_SYMBOL_GPL(threads_core_mask);
@@ -401,6 +402,7 @@ static void __init cpu_init_thread_core_maps(int tpc)
int i;
threads_per_core = tpc;
+ threads_per_subcore = tpc;
cpumask_clear(&threads_core_mask);
/* This implementation only supports power of 2 number of threads
--
1.9.1
^ permalink raw reply related
* [PATCH v2 2/6] powerpc/powernv: Make it possible to skip the IRQHAPPENED check in power7_nap()
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Michael Neuling, kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <1400832930-17050-1-git-send-email-mikey@neuling.org>
From: Michael Ellerman <mpe@ellerman.id.au>
To support split core we need to be able to force all secondaries into
nap, so the core can detect they are idle and do an unsplit.
Currently power7_nap() will return without napping if there is an irq
pending. We want to ignore the pending irq and nap anyway, we will deal
with the interrupt later.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/kernel/idle_power7.S | 9 +++++++++
arch/powerpc/platforms/powernv/smp.c | 2 +-
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index d660dc3..6d59072 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -449,7 +449,7 @@ extern unsigned long cpuidle_disable;
enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
extern int powersave_nap; /* set if nap mode can be used in idle loop */
-extern void power7_nap(void);
+extern void power7_nap(int check_irq);
extern void power7_sleep(void);
extern void flush_instruction_cache(void);
extern void hard_reset_now(void);
diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
index dca6e16..2480256 100644
--- a/arch/powerpc/kernel/idle_power7.S
+++ b/arch/powerpc/kernel/idle_power7.S
@@ -39,6 +39,10 @@
* Pass requested state in r3:
* 0 - nap
* 1 - sleep
+ *
+ * To check IRQ_HAPPENED in r4
+ * 0 - don't check
+ * 1 - check
*/
_GLOBAL(power7_powersave_common)
/* Use r3 to pass state nap/sleep/winkle */
@@ -71,6 +75,8 @@ _GLOBAL(power7_powersave_common)
lbz r0,PACAIRQHAPPENED(r13)
cmpwi cr0,r0,0
beq 1f
+ cmpwi cr0,r4,0
+ beq 1f
addi r1,r1,INT_FRAME_SIZE
ld r0,16(r1)
mtlr r0
@@ -114,15 +120,18 @@ _GLOBAL(power7_idle)
lwz r4,ADDROFF(powersave_nap)(r3)
cmpwi 0,r4,0
beqlr
+ li r3, 1
/* fall through */
_GLOBAL(power7_nap)
+ mr r4,r3
li r3,0
b power7_powersave_common
/* No return */
_GLOBAL(power7_sleep)
li r3,1
+ li r4,0
b power7_powersave_common
/* No return */
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 1601a1e..65faf99 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -159,7 +159,7 @@ static void pnv_smp_cpu_kill_self(void)
mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
while (!generic_check_cpu_restart(cpu)) {
ppc64_runlatch_off();
- power7_nap();
+ power7_nap(1);
ppc64_runlatch_on();
if (!generic_check_cpu_restart(cpu)) {
DBG("CPU%d Unexpected exit while offline !\n", cpu);
--
1.9.1
^ permalink raw reply related
* [PATCH v2 1/6] KVM: PPC: Book3S HV: Rework the secondary inhibit code
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: Michael Neuling, kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <1400832930-17050-1-git-send-email-mikey@neuling.org>
From: Michael Ellerman <mpe@ellerman.id.au>
As part of the support for split core on POWER8, we want to be able to
block splitting of the core while KVM VMs are active.
The logic to do that would be exactly the same as the code we currently
have for inhibiting onlining of secondaries.
Instead of adding an identical mechanism to block split core, rework the
secondary inhibit code to be a "HV KVM is active" check. We can then use
that in both the cpu hotplug code and the upcoming split core code.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Acked-by: Alexander Graf <agraf@suse.de>
Acked-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/include/asm/kvm_ppc.h | 7 +++++++
arch/powerpc/include/asm/smp.h | 8 --------
arch/powerpc/kernel/smp.c | 34 +++-------------------------------
arch/powerpc/kvm/book3s_hv.c | 8 ++++----
arch/powerpc/kvm/book3s_hv_builtin.c | 31 +++++++++++++++++++++++++++++++
5 files changed, 45 insertions(+), 43 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 4096f16..2c8e399 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -337,6 +337,10 @@ static inline void kvmppc_fast_vcpu_kick(struct kvm_vcpu *vcpu)
vcpu->kvm->arch.kvm_ops->fast_vcpu_kick(vcpu);
}
+extern void kvm_hv_vm_activated(void);
+extern void kvm_hv_vm_deactivated(void);
+extern bool kvm_hv_mode_active(void);
+
#else
static inline void __init kvm_cma_reserve(void)
{}
@@ -356,6 +360,9 @@ static inline void kvmppc_fast_vcpu_kick(struct kvm_vcpu *vcpu)
{
kvm_vcpu_kick(vcpu);
}
+
+static inline bool kvm_hv_mode_active(void) { return false; }
+
#endif
#ifdef CONFIG_KVM_XICS
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index ff51046..5a6614a 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -68,14 +68,6 @@ void generic_mach_cpu_die(void);
void generic_set_cpu_dead(unsigned int cpu);
void generic_set_cpu_up(unsigned int cpu);
int generic_check_cpu_restart(unsigned int cpu);
-
-extern void inhibit_secondary_onlining(void);
-extern void uninhibit_secondary_onlining(void);
-
-#else /* HOTPLUG_CPU */
-static inline void inhibit_secondary_onlining(void) {}
-static inline void uninhibit_secondary_onlining(void) {}
-
#endif
#ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e2a4232..6edae3d 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -36,6 +36,7 @@
#include <linux/atomic.h>
#include <asm/irq.h>
#include <asm/hw_irq.h>
+#include <asm/kvm_ppc.h>
#include <asm/page.h>
#include <asm/pgtable.h>
#include <asm/prom.h>
@@ -457,38 +458,9 @@ int generic_check_cpu_restart(unsigned int cpu)
return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
}
-static atomic_t secondary_inhibit_count;
-
-/*
- * Don't allow secondary CPU threads to come online
- */
-void inhibit_secondary_onlining(void)
-{
- /*
- * This makes secondary_inhibit_count stable during cpu
- * online/offline operations.
- */
- get_online_cpus();
-
- atomic_inc(&secondary_inhibit_count);
- put_online_cpus();
-}
-EXPORT_SYMBOL_GPL(inhibit_secondary_onlining);
-
-/*
- * Allow secondary CPU threads to come online again
- */
-void uninhibit_secondary_onlining(void)
-{
- get_online_cpus();
- atomic_dec(&secondary_inhibit_count);
- put_online_cpus();
-}
-EXPORT_SYMBOL_GPL(uninhibit_secondary_onlining);
-
-static int secondaries_inhibited(void)
+static bool secondaries_inhibited(void)
{
- return atomic_read(&secondary_inhibit_count);
+ return kvm_hv_mode_active();
}
#else /* HOTPLUG_CPU */
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 8227dba..d7b74f8 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -2317,10 +2317,10 @@ static int kvmppc_core_init_vm_hv(struct kvm *kvm)
spin_lock_init(&kvm->arch.slot_phys_lock);
/*
- * Don't allow secondary CPU threads to come online
- * while any KVM VMs exist.
+ * Track that we now have a HV mode VM active. This blocks secondary
+ * CPU threads from coming online.
*/
- inhibit_secondary_onlining();
+ kvm_hv_vm_activated();
return 0;
}
@@ -2336,7 +2336,7 @@ static void kvmppc_free_vcores(struct kvm *kvm)
static void kvmppc_core_destroy_vm_hv(struct kvm *kvm)
{
- uninhibit_secondary_onlining();
+ kvm_hv_vm_deactivated();
kvmppc_free_vcores(kvm);
if (kvm->arch.rma) {
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
index 8cd0dae..7cde8a6 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -6,6 +6,7 @@
* published by the Free Software Foundation.
*/
+#include <linux/cpu.h>
#include <linux/kvm_host.h>
#include <linux/preempt.h>
#include <linux/export.h>
@@ -181,3 +182,33 @@ void __init kvm_cma_reserve(void)
kvm_cma_declare_contiguous(selected_size, align_size);
}
}
+
+/*
+ * When running HV mode KVM we need to block certain operations while KVM VMs
+ * exist in the system. We use a counter of VMs to track this.
+ *
+ * One of the operations we need to block is onlining of secondaries, so we
+ * protect hv_vm_count with get/put_online_cpus().
+ */
+static atomic_t hv_vm_count;
+
+void kvm_hv_vm_activated(void)
+{
+ get_online_cpus();
+ atomic_inc(&hv_vm_count);
+ put_online_cpus();
+}
+EXPORT_SYMBOL_GPL(kvm_hv_vm_activated);
+
+void kvm_hv_vm_deactivated(void)
+{
+ get_online_cpus();
+ atomic_dec(&hv_vm_count);
+ put_online_cpus();
+}
+EXPORT_SYMBOL_GPL(kvm_hv_vm_deactivated);
+
+bool kvm_hv_mode_active(void)
+{
+ return atomic_read(&hv_vm_count) != 0;
+}
--
1.9.1
^ permalink raw reply related
* [PATCH v2 0/6] Implement split core for POWER8
From: Michael Neuling @ 2014-05-23 8:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Alexander Graf
Cc: linuxppc-dev, Paul Mackerras, kvm-ppc, kvm
This patch series implements split core mode on POWER8. This enables up to 4
subcores per core which can each independently run guests (per guest SPRs like
SDR1, LPIDR etc are replicated per subcore). Lots more documentation on this
feature in the code and commit messages.
Most of this code is in the powernv platform but there's a couple of KVM
specific patches too.
Patch series authored by mpe and me with a few bug fixes from others.
v2:
There are some minor updates based on comments and I've added the Acks by
Paulus and Alex for the KVM code.
^ permalink raw reply
* RE: [2/2] powerpc/corenet64_smp_defconfig: enable RTC support
From: Shengzhou.Liu @ 2014-05-23 8:03 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20140522225144.GA1670@home.buserror.net>
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, May 23, 2014 6:52 AM
> To: Liu Shengzhou-B36685
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: Re: [2/2] powerpc/corenet64_smp_defconfig: enable RTC support
>=20
> > +++ b/arch/powerpc/configs/corenet64_smp_defconfig
> > @@ -125,6 +125,11 @@ CONFIG_USB_EHCI_FSL=3Dy CONFIG_USB_STORAGE=3Dy
> > CONFIG_MMC=3Dy CONFIG_MMC_SDHCI=3Dy
> > +CONFIG_RTC_CLASS=3Dy
> > +CONFIG_RTC_DRV_CMOS=3Dy
> > +CONFIG_RTC_DRV_DS1307=3Dy
> > +CONFIG_RTC_DRV_DS1374=3Dy
> > +CONFIG_RTC_DRV_DS3232=3Dy
> > CONFIG_EDAC=3Dy
> > CONFIG_EDAC_MM_EDAC=3Dy
> > CONFIG_DMADEVICES=3Dy
>=20
> Why only corenet64 and not corenet32?
>=20
> -Scott
[Shengzhou] There is already RTC support in corenet32, only missing in core=
net64.
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-23 7:37 UTC (permalink / raw)
To: Alexander Graf
Cc: aik@ozlabs.ru, Gavin Shan, kvm-ppc@vger.kernel.org,
Alex Williamson, qiudayu@linux.vnet.ibm.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <76F750C7-EAA1-455F-A64F-BAB826F66281@suse.de>
On Fri, May 23, 2014 at 08:55:15AM +0200, Alexander Graf wrote:
>> Am 23.05.2014 um 06:37 schrieb Gavin Shan <gwshan@linux.vnet.ibm.com>:
>>> On Thu, May 22, 2014 at 09:10:53PM -0600, Alex Williamson wrote:
>>>> On Thu, 2014-05-22 at 18:23 +1000, Gavin Shan wrote:
>>>> The patch adds new IOCTL commands for VFIO PCI device to support
>>>> EEH functionality for PCI devices, which have been passed through
>>>> from host to somebody else via VFIO.
.../...
>>>> +
>>>> +/*
>>>> + * Reset is the major step to recover problematic PE. The following
>>>> + * command helps on that.
>>>> + */
>>>> +struct vfio_eeh_pe_reset {
>>>> + __u32 argsz;
>>>> + __u32 option;
>>>> +};
>>>> +
>>>> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
>>>> +
>>>> +/*
>>>> + * One of the steps for recovery after PE reset is to configure the
>>>> + * PCI bridges affected by the PE reset.
>>>> + */
>>>> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
>>>
>>> What can the user do differently by making these separate ioctls?
>>
>> hrm, I didn't understood as well. Alex.G could have the explaination.
>
>Alex raised the same concern as me: why separate reset and configure? When we want to recover a device, we need a reset call anyway, right?
>
Ok. With current ioctl commands, "reset+configure" is required to do
error recovery. Before the recovery, we also need call "configure"
in order to retrieve error log correctly.
Also, they corresponds to 2 separate RTAS services: "ibm,set-slot-reset"
and "ibm,configure-pe".
Thanks,
Gavin
^ permalink raw reply
* Re: powerpc: remove checks for CONFIG_BOOK3E_MMU_TLB_STATS
From: Paul Bolle @ 2014-05-23 7:33 UTC (permalink / raw)
To: Scott Wood; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20140522223758.GA20343@home.buserror.net>
Scott,
On Thu, 2014-05-22 at 17:37 -0500, Scott Wood wrote:
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S: Assembler messages:
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:89: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:238: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:269: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:281: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:441: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:510: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:881: Error: unrecognized opcode: `tlb_miss_prolog_stats'
> /home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:918: Error: unrecognized opcode: `tlb_miss_prolog_stats'
Thanks for testing!
That's a bit surprising. The patch is intended to be a non event. Ie, it
only removes what the preprocessor would have removed anyway. Unless I
botched it, of course.
What exactly did you test there?
Paul Bolle
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-23 6:55 UTC (permalink / raw)
To: Gavin Shan
Cc: aik@ozlabs.ru, Gavin Shan, kvm-ppc@vger.kernel.org,
Alex Williamson, qiudayu@linux.vnet.ibm.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20140523043722.GA11572@shangw>
> Am 23.05.2014 um 06:37 schrieb Gavin Shan <gwshan@linux.vnet.ibm.com>:
>=20
>> On Thu, May 22, 2014 at 09:10:53PM -0600, Alex Williamson wrote:
>>> On Thu, 2014-05-22 at 18:23 +1000, Gavin Shan wrote:
>>> The patch adds new IOCTL commands for VFIO PCI device to support
>>> EEH functionality for PCI devices, which have been passed through
>>> from host to somebody else via VFIO.
>>>=20
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>> Documentation/vfio.txt | 88 ++++++++++-
>>> arch/powerpc/include/asm/eeh.h | 17 +++
>>> arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++=
++++++
>>> drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
>>> include/uapi/linux/vfio.h | 53 +++++++
>>> 5 files changed, 603 insertions(+), 7 deletions(-)
>>=20
>> Maybe a chicken and egg problem, but it seems like we could split the
>> platform code and vfio code into separate patches.
>=20
> Ok. I'll keep egg/chicken separated in next revision.
>=20
>>>=20
>>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>>> index b9ca023..dd13db6 100644
>>> --- a/Documentation/vfio.txt
>>> +++ b/Documentation/vfio.txt
>>> @@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented=
in real mode which provides
>>> an excellent performance which has limitations such as inability to do
>>> locked pages accounting in real time.
>>>=20
>>> -So 3 additional ioctls have been added:
>>> +4) PPC64 guests detect PCI errors and recover from them via EEH RTAS se=
rvices,
>>> +which works on the basis of additional ioctl commands.
>>> +
>>> +So 8 additional ioctls have been added:
>>>=20
>>> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
>>> of the DMA window on the PCI bus.
>>> @@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
>>>=20
>>> VFIO_IOMMU_DISABLE - disables the container.
>>>=20
>>> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on th=
e
>>> + specified device. Also, it can be used to remove IO or DMA
>>> + stopped state on the frozen PE.
>>> +
>>> + VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified=
>>> + PE or query PE sharing mode.
>>> +
>>> + VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state=
.
>>> +
>>> + VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps fo=
r
>>> + error recovering.
>>> +
>>> + VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. I=
t's
>>> + one of the major steps for error recoverying.
>>>=20
>>> The code flow from the example above should be slightly changed:
>>>=20
>>> @@ -346,6 +363,75 @@ The code flow from the example above should be slig=
htly changed:
>>> ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
>>> .....
>>>=20
>>> +Based on the initial example we have, the following piece of code could=
be
>>> +reference for EEH setup and error handling:
>>> +
>>> + struct vfio_eeh_pe_set_option option =3D { .argsz =3D sizeof(option=
) };
>>> + struct vfio_eeh_pe_get_addr addr =3D { .argsz =3D sizeof(addr) };
>>> + struct vfio_eeh_pe_get_state state =3D { .argsz =3D sizeof(state) }=
;
>>> + struct vfio_eeh_pe_reset reset =3D { .argsz =3D sizeof(reset) };
>>> + struct vfio_eeh_pe_configure config =3D { .argsz =3D sizeof(config)=
};
>>> +
>>> + ....
>>> +
>>> + /* Get a file descriptor for the device */
>>> + device =3D ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
>>> +
>>> + /* Enable the EEH functionality on the device */
>>> + option.option =3D EEH_OPT_ENABLE;
>>> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>>> +
>>> + /* Retrieve PE address and create and maintain PE by yourself */
>>> + addr.option =3D EEH_OPT_GET_PE_ADDR;
>>> + ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
>>> +
>>> + /* Assure EEH is supported on the PE and make PE functional */
>>> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>>> +
>>> + /* Save device's state. pci_save_state() would be good enough
>>> + * as an example.
>>> + */
>>> +
>>> + /* Test and setup the device */
>>> + ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
>>> +
>>> + ....
>>> +
>>> + /* When 0xFF's returned from reading PCI config space or IO BARs
>>> + * of the PCI device. Check the PE state to see if that has been
>>> + * frozen.
>>> + */
>>> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>>=20
>> There's no notification, the user needs to observe the return value an
>> poll? Should we be enabling an eventfd to notify the user of the state
>> change?
>=20
> Yes. The user needs to monitor the return value. we should have one notifi=
cation,
> but it's for later as we discussed :-)
>=20
>>> +
>>> + /* Waiting for pending PCI transactions to be completed and don't
>>> + * produce any more PCI traffic from/to the affected PE until
>>> + * recovery is finished.
>>> + */
>>> +
>>> + /* Enable IO for the affected PE and collect logs. Usually, the
>>> + * standard part of PCI config space, AER registers are dumped
>>> + * as logs for further analysis.
>>> + */
>>> + option.option =3D EEH_OPT_THAW_MMIO;
>>> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>>=20
>> How does the guest learn about the error? Does it need to?
>=20
> When guest detects 0xFF's from reading PCI config space or IO, it's going
> check the device (PE) state. If the device (PE) has been put into frozen
> state, the recovery will be started.
>=20
>>> +
>>> + /* Issue PE reset */
>>> + reset.option =3D EEH_RESET_HOT;
>>> + ioctl(device, VFIO_EEH_PE_RESET, &reset);
>>> +
>>> + /* Configure the PCI bridges for the affected PE */
>>> + ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
>>> +
>>=20
>> I'm not sure I see why we've split these into separate ioctls. FWIW,
>> the one ioctl we currently have for reset that takes no options is
>> probably going to be the first to get deprecated because of it.
>>=20
>>> + /* Restored state we saved at initialization time. pci_restore_stat=
e()
>>> + * is good enough as an example.
>>> + */
>>> +
>>> + /* Hopefully, error is recovered successfully. Now, you can resume t=
o
>>> + * start PCI traffic to/from the affected PE.
>>> + */
>>> +
>>> + ....
>>> +
>>> ------------------------------------------------------------------------=
-------
>>>=20
>>> [1] VFIO was originally an acronym for "Virtual Function I/O" in its
>>> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/e=
eh.h
>>> index 34a2d83..dd5f1cf 100644
>>> --- a/arch/powerpc/include/asm/eeh.h
>>> +++ b/arch/powerpc/include/asm/eeh.h
>>> @@ -191,6 +191,11 @@ enum {
>>> #define EEH_OPT_ENABLE 1 /* EEH enable */
>>> #define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
>>> #define EEH_OPT_THAW_DMA 3 /* DMA enable */
>>> +#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
>>> +#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
>>> +#define EEH_PE_MODE_NONE 0 /* Invalid mode */
>>> +#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
>>> +#define EEH_PE_MODE_SHARED 2 /* Shared mode */
>>> #define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
>>> #define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
>>> #define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
>>> @@ -198,6 +203,11 @@ enum {
>>> #define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
>>> #define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
>>> #define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
>>> +#define EEH_PE_STATE_NORMAL 0 /* Normal state */
>>> +#define EEH_PE_STATE_RESET 1 /* PE reset */
>>> +#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
>>> +#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
>>> +#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
>>> #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
>>> #define EEH_RESET_HOT 1 /* Hot reset */
>>> #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
>>> @@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
>>> void eeh_add_device_tree_late(struct pci_bus *);
>>> void eeh_add_sysfs_files(struct pci_bus *);
>>> void eeh_remove_device(struct pci_dev *);
>>> +int eeh_dev_open(struct pci_dev *pdev);
>>> +void eeh_dev_release(struct pci_dev *pdev);
>>> +int eeh_pe_set_option(struct pci_dev *pdev, int option);
>>> +int eeh_pe_get_addr(struct pci_dev *pdev, int option);
>>> +int eeh_pe_get_state(struct pci_dev *pdev);
>>> +int eeh_pe_reset(struct pci_dev *pdev, int option);
>>> +int eeh_pe_configure(struct pci_dev *pdev);
>>>=20
>>> /**
>>> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>> index 9c6b899..b90a474 100644
>>> --- a/arch/powerpc/kernel/eeh.c
>>> +++ b/arch/powerpc/kernel/eeh.c
>>> @@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops =3D NULL;
>>> /* Lock to avoid races due to multiple reports of an error */
>>> DEFINE_RAW_SPINLOCK(confirm_error_lock);
>>>=20
>>> +/* Lock to protect passed flags */
>>> +static DEFINE_MUTEX(eeh_dev_mutex);
>>> +
>>> /* Buffer for reporting pci register dumps. Its here in BSS, and
>>> * not dynamically alloced, so that it ends up in RMO where RTAS
>>> * can access it.
>>> @@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
>>> edev->mode &=3D ~EEH_DEV_SYSFS;
>>> }
>>>=20
>>> +/**
>>> + * eeh_dev_open - Mark EEH device and PE as passed through
>>> + * @pdev: PCI device
>>> + *
>>> + * Mark the indicated EEH device and PE as passed through.
>>> + * In the result, the EEH errors detected on the PE won't be
>>> + * reported. The owner of the device will be responsible for
>>> + * detection and recovery.
>>> + */
>>> +int eeh_dev_open(struct pci_dev *pdev)
>>> +{
>>> + struct eeh_dev *edev;
>>> +
>>> + mutex_lock(&eeh_dev_mutex);
>>> +
>>> + /* No PCI device ? */
>>> + if (!pdev) {
>>> + mutex_unlock(&eeh_dev_mutex);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + /* No EEH device ? */
>>> + edev =3D pci_dev_to_eeh_dev(pdev);
>>> + if (!edev || !edev->pe) {
>>> + mutex_unlock(&eeh_dev_mutex);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + eeh_dev_set_passed(edev, true);
>>> + eeh_pe_set_passed(edev->pe, true);
>>> + mutex_unlock(&eeh_dev_mutex);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_dev_open);
>>> +
>>> +/**
>>> + * eeh_dev_release - Reclaim the ownership of EEH device
>>> + * @pdev: PCI device
>>> + *
>>> + * Reclaim ownership of EEH device, potentially the corresponding
>>> + * PE. In the result, the EEH errors detected on the PE will be
>>> + * reported and handled as usual.
>>> + */
>>> +void eeh_dev_release(struct pci_dev *pdev)
>>> +{
>>> + bool release_pe =3D true;
>>> + struct eeh_pe *pe =3D NULL;
>>> + struct eeh_dev *tmp, *edev;
>>> +
>>> + mutex_lock(&eeh_dev_mutex);
>>> +
>>> + /* No PCI device ? */
>>> + if (!pdev) {
>>> + mutex_unlock(&eeh_dev_mutex);
>>> + return;
>>> + }
>>> +
>>> + /* No EEH device ? */
>>> + edev =3D pci_dev_to_eeh_dev(pdev);
>>> + if (!edev || !eeh_dev_passed(edev) ||
>>> + !edev->pe || !eeh_pe_passed(pe)) {
>>> + mutex_unlock(&eeh_dev_mutex);
>>> + return;
>>> + }
>>> +
>>> + /* Release device */
>>> + pe =3D edev->pe;
>>> + eeh_dev_set_passed(edev, false);
>>> +
>>> + /* Release PE */
>>> + eeh_pe_for_each_dev(pe, edev, tmp) {
>>> + if (eeh_dev_passed(edev)) {
>>> + release_pe =3D false;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (release_pe)
>>> + eeh_pe_set_passed(pe, false);
>>> +
>>> + mutex_unlock(&eeh_dev_mutex);
>>> +}
>>> +EXPORT_SYMBOL(eeh_dev_release);
>>> +
>>> +static int eeh_dev_check(struct pci_dev *pdev,
>>> + struct eeh_dev **pedev,
>>> + struct eeh_pe **ppe)
>>> +{
>>> + struct eeh_dev *edev;
>>> +
>>> + /* No device ? */
>>> + if (!pdev)
>>> + return -ENODEV;
>>> +
>>> + edev =3D pci_dev_to_eeh_dev(pdev);
>>> + if (!edev || !eeh_dev_passed(edev) ||
>>> + !edev->pe || !eeh_pe_passed(edev->pe))
>>> + return -ENODEV;
>>> +
>>> + if (pedev)
>>> + *pedev =3D edev;
>>> + if (ppe)
>>> + *ppe =3D edev->pe;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * eeh_pe_set_option - Set options for the indicated PE
>>> + * @pdev: PCI device
>>> + * @option: requested option
>>> + *
>>> + * The routine is called to enable or disable EEH functionality
>>> + * on the indicated PE, to enable IO or DMA for the frozen PE.
>>> + */
>>> +int eeh_pe_set_option(struct pci_dev *pdev, int option)
>>> +{
>>> + struct eeh_dev *edev;
>>> + struct eeh_pe *pe;
>>> + int ret =3D 0;
>>> +
>>> + /* Device existing ? */
>>> + ret =3D eeh_dev_check(pdev, &edev, &pe);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (option) {
>>> + case EEH_OPT_DISABLE:
>>> + case EEH_OPT_ENABLE:
>>> + break;
>>> + case EEH_OPT_THAW_MMIO:
>>> + case EEH_OPT_THAW_DMA:
>>> + if (!eeh_ops || !eeh_ops->set_option) {
>>> + ret =3D -ENOENT;
>>> + break;
>>> + }
>>> +
>>> + ret =3D eeh_ops->set_option(pe, option);
>>> + break;
>>> + default:
>>> + pr_debug("%s: Option %d out of range (%d, %d)\n",
>>> + __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>>> + ret =3D -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_pe_set_option);
>>> +
>>> +/**
>>> + * eeh_pe_get_addr - Retrieve the PE address or sharing mode
>>> + * @pdev: PCI device
>>> + * @option: option
>>> + *
>>> + * Retrieve the PE address or sharing mode.
>>> + */
>>> +int eeh_pe_get_addr(struct pci_dev *pdev, int option)
>>> +{
>>> + struct pci_bus *bus;
>>> + struct eeh_dev *edev;
>>> + struct eeh_pe *pe;
>>> + int ret =3D 0;
>>> +
>>> + /* Device existing ? */
>>> + ret =3D eeh_dev_check(pdev, &edev, &pe);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (option) {
>>> + case EEH_OPT_GET_PE_ADDR:
>>> + bus =3D eeh_pe_bus_get(pe);
>>> + if (!bus) {
>>> + ret =3D -ENODEV;
>>> + break;
>>> + }
>>> +
>>> + /* PE address has format "00BBSS00" */
>>> + ret =3D bus->number << 16;
>>> + break;
>>> + case EEH_OPT_GET_PE_MODE:
>>> + /* Wa always have shared PE */
>>> + ret =3D EEH_PE_MODE_SHARED;
>>> + break;
>>> + default:
>>> + pr_debug("%s: option %d out of range (0, 1)\n",
>>> + __func__, option);
>>> + ret =3D -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
>>> +
>>> +/**
>>> + * eeh_pe_get_state - Retrieve PE's state
>>> + * @pdev: PCI device
>>> + *
>>> + * Retrieve the PE's state, which includes 3 aspects: enabled
>>> + * DMA, enabled IO and asserted reset.
>>> + */
>>> +int eeh_pe_get_state(struct pci_dev *pdev)
>>> +{
>>> + struct eeh_dev *edev;
>>> + struct eeh_pe *pe;
>>> + int result, ret =3D 0;
>>> +
>>> + /* Device existing ? */
>>> + ret =3D eeh_dev_check(pdev, &edev, &pe);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (!eeh_ops || !eeh_ops->get_state)
>>> + return -ENOENT;
>>> +
>>> + result =3D eeh_ops->get_state(pe, NULL);
>>> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>> + (result & EEH_STATE_DMA_ENABLED) &&
>>> + (result & EEH_STATE_MMIO_ENABLED))
>>> + ret =3D EEH_PE_STATE_NORMAL;
>>> + else if (result & EEH_STATE_RESET_ACTIVE)
>>> + ret =3D EEH_PE_STATE_RESET;
>>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>> + !(result & EEH_STATE_DMA_ENABLED) &&
>>> + !(result & EEH_STATE_MMIO_ENABLED))
>>> + ret =3D EEH_PE_STATE_STOPPED_IO_DMA;
>>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>> + (result & EEH_STATE_DMA_ENABLED) &&
>>> + !(result & EEH_STATE_MMIO_ENABLED))
>>> + ret =3D EEH_PE_STATE_STOPPED_DMA;
>>> + else
>>> + ret =3D EEH_PE_STATE_UNAVAIL;
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_pe_get_state);
>>> +
>>> +/**
>>> + * eeh_pe_reset - Issue PE reset according to specified type
>>> + * @pdev: PCI device
>>> + * @option: reset type
>>> + *
>>> + * The routine is called to reset the specified PE with the
>>> + * indicated type, either fundamental reset or hot reset.
>>> + * PE reset is the most important part for error recovery.
>>> + */
>>> +int eeh_pe_reset(struct pci_dev *pdev, int option)
>>> +{
>>> + struct eeh_dev *edev;
>>> + struct eeh_pe *pe;
>>> + int ret =3D 0;
>>> +
>>> + /* Device existing ? */
>>> + ret =3D eeh_dev_check(pdev, &edev, &pe);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
>>> + return -ENOENT;
>>> +
>>> + switch (option) {
>>> + case EEH_RESET_DEACTIVATE:
>>> + ret =3D eeh_ops->reset(pe, option);
>>> + if (ret)
>>> + break;
>>> +
>>> + /*
>>> + * The PE is still in frozen state and we need clear
>>> + * that. It's good to clear frozen state after deassert
>>> + * to avoid messy IO access during reset, which might
>>> + * cause recursive frozen PE.
>>> + */
>>> + ret =3D eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
>>> + if (!ret)
>>> + ret =3D eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
>>> + if (!ret)
>>> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>>> + break;
>>> + case EEH_RESET_HOT:
>>> + case EEH_RESET_FUNDAMENTAL:
>>> + ret =3D eeh_ops->reset(pe, option);
>>> + break;
>>> + default:
>>> + pr_debug("%s: Unsupported option %d\n",
>>> + __func__, option);
>>> + ret =3D -EINVAL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_pe_reset);
>>> +
>>> +/**
>>> + * eeh_pe_configure - Configure PCI bridges after PE reset
>>> + * @pdev: PCI device
>>> + *
>>> + * The routine is called to restore the PCI config space for
>>> + * those PCI devices, especially PCI bridges affected by PE
>>> + * reset issued previously.
>>> + */
>>> +int eeh_pe_configure(struct pci_dev *pdev)
>>> +{
>>> + struct eeh_dev *edev;
>>> + struct eeh_pe *pe;
>>> + int ret =3D 0;
>>> +
>>> + /* Device existing ? */
>>> + ret =3D eeh_dev_check(pdev, &edev, &pe);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Restore config space for the affected devices */
>>> + eeh_pe_restore_bars(pe);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(eeh_pe_configure);
>>> +
>>> static int proc_eeh_show(struct seq_file *m, void *v)
>>> {
>>> if (!eeh_enabled()) {
>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>> index 7ba0424..301ac18 100644
>>> --- a/drivers/vfio/pci/vfio_pci.c
>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>> @@ -25,6 +25,9 @@
>>> #include <linux/types.h>
>>> #include <linux/uaccess.h>
>>> #include <linux/vfio.h>
>>> +#ifdef CONFIG_EEH
>>> +#include <asm/eeh.h>
>>> +#endif
>>=20
>> Can we make a vfio_pci_eeh file that properly stubs out anything called
>> from common code? I don't want to see all these inline ifdefs.
>=20
> well. do you want see drivers/vfio/vfio-pci/vfio_pci_eeh.c and export
> following functins for vfio_pci.c to use?
>=20
> vfio_pci_eeh_open()
> vfio_pci_eeh_release()
> vfio_pci_eeh_ioctl()
>=20
>=20
>>>=20
>>> #include "vfio_pci_private.h"
>>>=20
>>> @@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_devic=
e *vdev)
>>> pci_restore_state(pdev);
>>> }
>>>=20
>>> +static void vfio_eeh_pci_release(struct pci_dev *pdev)
>>> +{
>>> +#ifdef CONFIG_EEH
>>> + eeh_dev_release(pdev);
>>> +#endif
>>> +}
>>> +
>>> static void vfio_pci_release(void *device_data)
>>> {
>>> struct vfio_pci_device *vdev =3D device_data;
>>>=20
>>> - if (atomic_dec_and_test(&vdev->refcnt))
>>> + if (atomic_dec_and_test(&vdev->refcnt)) {
>>> + vfio_eeh_pci_release(vdev->pdev);
>>> vfio_pci_disable(vdev);
>>> + }
>>>=20
>>> module_put(THIS_MODULE);
>>> }
>>>=20
>>> +static int vfio_eeh_pci_open(struct pci_dev *pdev)
>>> +{
>>> + int ret =3D 0;
>>> +
>>> +#ifdef CONFIG_EEH
>>> + ret =3D eeh_dev_open(pdev);
>>> +#endif
>>> + return ret;
>>> +}
>>> +
>>> static int vfio_pci_open(void *device_data)
>>> {
>>> struct vfio_pci_device *vdev =3D device_data;
>>> + int ret;
>>>=20
>>> if (!try_module_get(THIS_MODULE))
>>> return -ENODEV;
>>>=20
>>> if (atomic_inc_return(&vdev->refcnt) =3D=3D 1) {
>>> - int ret =3D vfio_pci_enable(vdev);
>>> - if (ret) {
>>> - module_put(THIS_MODULE);
>>> - return ret;
>>> - }
>>> + ret =3D vfio_pci_enable(vdev);
>>> + if (ret)
>>> + goto error;
>>> +
>>> + ret =3D vfio_eeh_pci_open(vdev->pdev);
>>> + if (ret)
>>> + goto error;
>>> }
>>>=20
>>> return 0;
>>> +error:
>>> + module_put(THIS_MODULE);
>>> + return ret;
>>> }
>>>=20
>>> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_=
type)
>>> @@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci=
_dev *pdev,
>>> return walk.ret;
>>> }
>>>=20
>>> +static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
>>> + unsigned int cmd,
>>> + unsigned long arg)
>>> +{
>>> + unsigned long minsz;
>>> + int ret =3D 0;
>>> +
>>> +#ifdef CONFIG_EEH
>>> + switch (cmd) {
>>> + case VFIO_EEH_PE_SET_OPTION: {
>>> + struct vfio_eeh_pe_set_option option;
>>> +
>>> + minsz =3D offsetofend(struct vfio_eeh_pe_set_option, option);
>>> + if (copy_from_user(&option, (void __user *)arg, minsz))
>>> + return -EFAULT;
>>> + if (option.argsz < minsz)
>>> + return -EINVAL;
>>> +
>>> + ret =3D eeh_pe_set_option(pdev, option.option);
>>> + break;
>>> + }
>>> + case VFIO_EEH_PE_GET_ADDR: {
>>> + struct vfio_eeh_pe_get_addr addr;
>>> +
>>> + minsz =3D offsetofend(struct vfio_eeh_pe_get_addr, info);
>>> + if (copy_from_user(&addr, (void __user *)arg, minsz))
>>> + return -EFAULT;
>>> + if (addr.argsz < minsz)
>>> + return -EINVAL;
>>> +
>>> + ret =3D eeh_pe_get_addr(pdev, addr.option);
>>> + if (ret >=3D 0) {
>>> + addr.info =3D ret;
>>> + if (copy_to_user((void __user *)arg, &addr, minsz))
>>> + return -EFAULT;
>>> + ret =3D 0;
>>> + }
>>> +
>>> + break;
>>> + }
>>> + case VFIO_EEH_PE_GET_STATE: {
>>> + struct vfio_eeh_pe_get_state state;
>>> +
>>> + minsz =3D offsetofend(struct vfio_eeh_pe_get_state, state);
>>> + if (copy_from_user(&state, (void __user *)arg, minsz))
>>> + return -EFAULT;
>>> + if (state.argsz < minsz)
>>> + return -EINVAL;
>>> +
>>> + ret =3D eeh_pe_get_state(pdev);
>>> + if (ret >=3D 0) {
>>> + state.state =3D ret;
>>> + if (copy_to_user((void __user *)arg, &state, minsz))
>>> + return -EFAULT;
>>> + ret =3D 0;
>>> + }
>>> + break;
>>> + }
>>> + case VFIO_EEH_PE_RESET: {
>>> + struct vfio_eeh_pe_reset reset;
>>> +
>>> + minsz =3D offsetofend(struct vfio_eeh_pe_reset, option);
>>> + if (copy_from_user(&reset, (void __user *)arg, minsz))
>>> + return -EFAULT;
>>> + if (reset.argsz < minsz)
>>> + return -EINVAL;
>>> +
>>> + ret =3D eeh_pe_reset(pdev, reset.option);
>>> + break;
>>> + }
>>> + case VFIO_EEH_PE_CONFIGURE:
>>> + ret =3D eeh_pe_configure(pdev);
>>> + break;
>>> + default:
>>> + ret =3D -EINVAL;
>>> + pr_debug("%s: Cannot handle command %d\n",
>>> + __func__, cmd);
>>> + }
>>> +#else
>>> + ret =3D -ENOENT;
>>> +#endif
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static long vfio_pci_ioctl(void *device_data,
>>> unsigned int cmd, unsigned long arg)
>>> {
>>> @@ -682,6 +795,12 @@ hot_reset_release:
>>>=20
>>> kfree(groups);
>>> return ret;
>>> + } else if (cmd =3D=3D VFIO_EEH_PE_SET_OPTION ||
>>> + cmd =3D=3D VFIO_EEH_PE_GET_ADDR ||
>>> + cmd =3D=3D VFIO_EEH_PE_GET_STATE ||
>>> + cmd =3D=3D VFIO_EEH_PE_RESET ||
>>> + cmd =3D=3D VFIO_EEH_PE_CONFIGURE) {
>>> + return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
>>> }
>>>=20
>>> return -ENOTTY;
>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>> index cb9023d..ef55682 100644
>>> --- a/include/uapi/linux/vfio.h
>>> +++ b/include/uapi/linux/vfio.h
>>> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>>>=20
>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>>=20
>>> +/*
>>> + * EEH functionality can be enabled or disabled on one specific device.=
>>> + * Also, the DMA or IO frozen state can be removed from the frozen PE
>>> + * if required.
>>> + */
>>> +struct vfio_eeh_pe_set_option {
>>> + __u32 argsz;
>>> + __u32 option;
>>> +};
>>> +
>>> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>>=20
>> What proposed ioctls are making you jump to 21?
>>=20
>> argsz is probably not as useful without a flags field. Otherwise the
>> caller can't indicate what the extra space is.
>=20
> The QEMU patches are based on Alexey's additional feature ("ddw"), which
> consumed several ioctl commands.
>=20
> ok. So you also prefer to remove "argsz"?
>=20
>>> +
>>> +/*
>>> + * Each EEH PE should have unique address to be identified. The command=
>>> + * helps to retrieve the address and the sharing mode of the PE.
>>> + */
>>> +struct vfio_eeh_pe_get_addr {
>>> + __u32 argsz;
>>> + __u32 option;
>>> + __u32 info;
>>> +};
>>> +
>>> +#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
>>> +
>>> +/*
>>> + * EEH PE might have been frozen because of PCI errors. Also, it might
>>> + * be experiencing reset for error revoery. The following command helps=
>>> + * to get the state.
>>> + */
>>> +struct vfio_eeh_pe_get_state {
>>> + __u32 argsz;
>>> + __u32 state;
>>> +};
>>> +
>>> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
>>> +
>>> +/*
>>> + * Reset is the major step to recover problematic PE. The following
>>> + * command helps on that.
>>> + */
>>> +struct vfio_eeh_pe_reset {
>>> + __u32 argsz;
>>> + __u32 option;
>>> +};
>>> +
>>> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
>>> +
>>> +/*
>>> + * One of the steps for recovery after PE reset is to configure the
>>> + * PCI bridges affected by the PE reset.
>>> + */
>>> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
>>=20
>> What can the user do differently by making these separate ioctls?
>=20
> hrm, I didn't understood as well. Alex.G could have the explaination.
Alex raised the same concern as me: why separate reset and configure? When w=
e want to recover a device, we need a reset call anyway, right?
Alex
>=20
>>> +
>>> /* ***************************************************************** */
>>>=20
>>> #endif /* _UAPIVFIO_H */
>=20
> Thanks,
> Gavin
>=20
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-23 6:52 UTC (permalink / raw)
To: Alex Williamson
Cc: aik@ozlabs.ru, Gavin Shan, kvm-ppc@vger.kernel.org,
qiudayu@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1400815400.3289.436.camel@ul30vt.home>
> Am 23.05.2014 um 05:23 schrieb Alex Williamson <alex.williamson@redhat.com=
>:
>=20
>> On Fri, 2014-05-23 at 10:37 +1000, Gavin Shan wrote:
>>> On Fri, May 23, 2014 at 10:17:30AM +1000, Gavin Shan wrote:
>>>> On Thu, May 22, 2014 at 11:55:29AM +0200, Alexander Graf wrote:
>>>> On 22.05.14 10:23, Gavin Shan wrote:
>>=20
>> .../...
>>=20
>>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>> index cb9023d..ef55682 100644
>>>>> --- a/include/uapi/linux/vfio.h
>>>>> +++ b/include/uapi/linux/vfio.h
>>>>> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>>>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12=
)
>>>>> +/*
>>>>> + * EEH functionality can be enabled or disabled on one specific devic=
e.
>>>>> + * Also, the DMA or IO frozen state can be removed from the frozen PE=
>>>>> + * if required.
>>>>> + */
>>>>> +struct vfio_eeh_pe_set_option {
>>>>> + __u32 argsz;
>>>>=20
>>>> What is this argsz thing? Is this your way of maintaining backwards
>>>> compatibility when we introduce new fields? A new field will change
>>>> the ioctl number, so I don't think that makes a lot of sense :).
>>>>=20
>>>> Just make the ioctl have a u32 as incoming argument. No fancy
>>>> structs, no complicated code.
>>>>=20
>>>> The same applies for a number of structs below.
>>>=20
>>> ok. Will do in next revision.
>>=20
>> Rechecked include/uapi/linux/vfio.h, the data struct for each ioctl comma=
nd
>> always has "argsz". I guess it was used as checker by Alex.W. Do you real=
ly
>> want remove "argsz" ?
>=20
>=20
> IIRC, this was actually a suggestion incorporated from David Gibson, but
> using _IO with an argsz and flags field we can maintain compatibility
> without bumping the ioctl number. It really only makes sense if we have
> a flags field so we can identify what additional information is being
> provided. Flags can be used as a bitmap of trailing structures or as
> revision if we want a set of trailing structures that may change over
> time. Unless you can come up with a good argument against it that would
> prevent us inventing a new ioctl as soon as we need a minor tweak, I'd
> prefer to keep it. As I noted in a previous comment, the one ioctl we
> have for reset that doesn't take any options is likely going to be the
> first ioctl that we need to entirely replace. If we don't keep argsz,
> it seems like we probably need a flags field and reserved structures.
>=20
>>>>> + __u32 option;
>>>>> +};
>>>>> +
>>>>> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>>>>> +
>>>>> +/*
>>>>> + * Each EEH PE should have unique address to be identified. The comma=
nd
>>>>> + * helps to retrieve the address and the sharing mode of the PE.
>>>>> + */
>>>>> +struct vfio_eeh_pe_get_addr {
>>>>> + __u32 argsz;
>>>>> + __u32 option;
>>>>> + __u32 info;
>>>>=20
>>>> Any particular reason you need the info field? Can't the return value
>>>> of the ioctl hold this? Then you only have a single u32 argument left
>>>> to the ioctl again.
>>>=20
>>> ok. Will do in next revision.
>>=20
>> If we eventually remove "argsz" and let ioctl() return value to hold
>> information (or negative number for errors), we don't need any data
>> struct because the 3rd parameter of ioctl() would be used as input
>> and I only need one input parameter. Do you want see this ?
>>=20
>> Hopefully, Alex.W saw this and hasn't objections :)
>=20
> I'm not sure why we're pushing for the minimal data set to pass to an
> ioctl. Seems like a recipe for dead, useless ioctls. Thanks,
>=20
The ioctl number includes sizeof(payload). So if a new parameter gets added,=
that would be a different ioctl number.
If you want to maintain backwards compatibility ioctl number wise in the ker=
nel, you'll have to have a "flags" field to indicate whether new data is ava=
ilable and a "pad" field, prefarably in a union, that ensures the size of th=
e struct doesn't change.
I'm not sure it's really necessary here to have identical ioctl numbers if w=
e add parameters, since we can always just define a new ioctl with a bigger p=
ayload that can then become the default handler and a shim backwards compati=
ble handler with the old number.
But if you think it is important, let's do it for real, not just halfway.
Alex
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Benjamin Herrenschmidt @ 2014-05-23 5:00 UTC (permalink / raw)
To: Gavin Shan, Alex Williamson; +Cc: aik, qiudayu, linuxppc-dev, agraf, kvm-ppc
In-Reply-To: <20140523043722.GA11572@shangw>
On Fri, 2014-05-23 at 14:37 +1000, Gavin Shan wrote:
> >There's no notification, the user needs to observe the return value an
> >poll? Should we be enabling an eventfd to notify the user of the state
> >change?
> >
>
> Yes. The user needs to monitor the return value. we should have one notification,
> but it's for later as we discussed :-)
../..
> >How does the guest learn about the error? Does it need to?
>
> When guest detects 0xFF's from reading PCI config space or IO, it's going
> check the device (PE) state. If the device (PE) has been put into frozen
> state, the recovery will be started.
Quick recap for Alex W (we discussed that with Alex G).
While a notification looks like a worthwhile addition in the long run, it
is not sufficient and not used today and I prefer that we keep that as something
to add later for those two main reasons:
- First, the kernel itself isn't always notified. For example, if we implement
on top of an RTAS backend (PR KVM under pHyp) or if we are on top of PowerNV but
the error is a PHB "fence" (the entire PCI Host bridge gets fenced out in hardware
due to an internal error), then we get no notification. Only polling of the
hardware or firmware will tell us. Since we don't want to have a polling timer
in the kernel, that means that the userspace client of VFIO (or alternatively
the KVM guest) is the one that polls.
- Second, this is how our primary user expects it: The primary (and only initial)
user of this will be qemu/KVM for PAPR guests and they don't have a notification
mechanism. Instead they query the EEH state after detecting an all 1's return from
MMIO or config space. This is how PAPR specifies it so we are just implementing the
spec here :-)
Because of these, I think we shouldn't worry too much about notification at
this stage.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-23 4:37 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1400814653.3289.428.camel@ul30vt.home>
On Thu, May 22, 2014 at 09:10:53PM -0600, Alex Williamson wrote:
>On Thu, 2014-05-22 at 18:23 +1000, Gavin Shan wrote:
>> The patch adds new IOCTL commands for VFIO PCI device to support
>> EEH functionality for PCI devices, which have been passed through
>> from host to somebody else via VFIO.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> Documentation/vfio.txt | 88 ++++++++++-
>> arch/powerpc/include/asm/eeh.h | 17 +++
>> arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
>> include/uapi/linux/vfio.h | 53 +++++++
>> 5 files changed, 603 insertions(+), 7 deletions(-)
>
>Maybe a chicken and egg problem, but it seems like we could split the
>platform code and vfio code into separate patches.
>
Ok. I'll keep egg/chicken separated in next revision.
>>
>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> index b9ca023..dd13db6 100644
>> --- a/Documentation/vfio.txt
>> +++ b/Documentation/vfio.txt
>> @@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
>> an excellent performance which has limitations such as inability to do
>> locked pages accounting in real time.
>>
>> -So 3 additional ioctls have been added:
>> +4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
>> +which works on the basis of additional ioctl commands.
>> +
>> +So 8 additional ioctls have been added:
>>
>> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
>> of the DMA window on the PCI bus.
>> @@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
>>
>> VFIO_IOMMU_DISABLE - disables the container.
>>
>> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on the
>> + specified device. Also, it can be used to remove IO or DMA
>> + stopped state on the frozen PE.
>> +
>> + VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified
>> + PE or query PE sharing mode.
>> +
>> + VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state.
>> +
>> + VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps for
>> + error recovering.
>> +
>> + VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. It's
>> + one of the major steps for error recoverying.
>>
>> The code flow from the example above should be slightly changed:
>>
>> @@ -346,6 +363,75 @@ The code flow from the example above should be slightly changed:
>> ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
>> .....
>>
>> +Based on the initial example we have, the following piece of code could be
>> +reference for EEH setup and error handling:
>> +
>> + struct vfio_eeh_pe_set_option option = { .argsz = sizeof(option) };
>> + struct vfio_eeh_pe_get_addr addr = { .argsz = sizeof(addr) };
>> + struct vfio_eeh_pe_get_state state = { .argsz = sizeof(state) };
>> + struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
>> + struct vfio_eeh_pe_configure config = { .argsz = sizeof(config) };
>> +
>> + ....
>> +
>> + /* Get a file descriptor for the device */
>> + device = ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
>> +
>> + /* Enable the EEH functionality on the device */
>> + option.option = EEH_OPT_ENABLE;
>> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>> +
>> + /* Retrieve PE address and create and maintain PE by yourself */
>> + addr.option = EEH_OPT_GET_PE_ADDR;
>> + ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
>> +
>> + /* Assure EEH is supported on the PE and make PE functional */
>> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>> +
>> + /* Save device's state. pci_save_state() would be good enough
>> + * as an example.
>> + */
>> +
>> + /* Test and setup the device */
>> + ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
>> +
>> + ....
>> +
>> + /* When 0xFF's returned from reading PCI config space or IO BARs
>> + * of the PCI device. Check the PE state to see if that has been
>> + * frozen.
>> + */
>> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>
>There's no notification, the user needs to observe the return value an
>poll? Should we be enabling an eventfd to notify the user of the state
>change?
>
Yes. The user needs to monitor the return value. we should have one notification,
but it's for later as we discussed :-)
>> +
>> + /* Waiting for pending PCI transactions to be completed and don't
>> + * produce any more PCI traffic from/to the affected PE until
>> + * recovery is finished.
>> + */
>> +
>> + /* Enable IO for the affected PE and collect logs. Usually, the
>> + * standard part of PCI config space, AER registers are dumped
>> + * as logs for further analysis.
>> + */
>> + option.option = EEH_OPT_THAW_MMIO;
>> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>
>How does the guest learn about the error? Does it need to?
When guest detects 0xFF's from reading PCI config space or IO, it's going
check the device (PE) state. If the device (PE) has been put into frozen
state, the recovery will be started.
>> +
>> + /* Issue PE reset */
>> + reset.option = EEH_RESET_HOT;
>> + ioctl(device, VFIO_EEH_PE_RESET, &reset);
>> +
>> + /* Configure the PCI bridges for the affected PE */
>> + ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
>> +
>
>I'm not sure I see why we've split these into separate ioctls. FWIW,
>the one ioctl we currently have for reset that takes no options is
>probably going to be the first to get deprecated because of it.
>
>> + /* Restored state we saved at initialization time. pci_restore_state()
>> + * is good enough as an example.
>> + */
>> +
>> + /* Hopefully, error is recovered successfully. Now, you can resume to
>> + * start PCI traffic to/from the affected PE.
>> + */
>> +
>> + ....
>> +
>> -------------------------------------------------------------------------------
>>
>> [1] VFIO was originally an acronym for "Virtual Function I/O" in its
>> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>> index 34a2d83..dd5f1cf 100644
>> --- a/arch/powerpc/include/asm/eeh.h
>> +++ b/arch/powerpc/include/asm/eeh.h
>> @@ -191,6 +191,11 @@ enum {
>> #define EEH_OPT_ENABLE 1 /* EEH enable */
>> #define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
>> #define EEH_OPT_THAW_DMA 3 /* DMA enable */
>> +#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
>> +#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
>> +#define EEH_PE_MODE_NONE 0 /* Invalid mode */
>> +#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
>> +#define EEH_PE_MODE_SHARED 2 /* Shared mode */
>> #define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
>> #define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
>> #define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
>> @@ -198,6 +203,11 @@ enum {
>> #define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
>> #define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
>> #define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
>> +#define EEH_PE_STATE_NORMAL 0 /* Normal state */
>> +#define EEH_PE_STATE_RESET 1 /* PE reset */
>> +#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
>> +#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
>> +#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
>> #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
>> #define EEH_RESET_HOT 1 /* Hot reset */
>> #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
>> @@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
>> void eeh_add_device_tree_late(struct pci_bus *);
>> void eeh_add_sysfs_files(struct pci_bus *);
>> void eeh_remove_device(struct pci_dev *);
>> +int eeh_dev_open(struct pci_dev *pdev);
>> +void eeh_dev_release(struct pci_dev *pdev);
>> +int eeh_pe_set_option(struct pci_dev *pdev, int option);
>> +int eeh_pe_get_addr(struct pci_dev *pdev, int option);
>> +int eeh_pe_get_state(struct pci_dev *pdev);
>> +int eeh_pe_reset(struct pci_dev *pdev, int option);
>> +int eeh_pe_configure(struct pci_dev *pdev);
>>
>> /**
>> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>> index 9c6b899..b90a474 100644
>> --- a/arch/powerpc/kernel/eeh.c
>> +++ b/arch/powerpc/kernel/eeh.c
>> @@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops = NULL;
>> /* Lock to avoid races due to multiple reports of an error */
>> DEFINE_RAW_SPINLOCK(confirm_error_lock);
>>
>> +/* Lock to protect passed flags */
>> +static DEFINE_MUTEX(eeh_dev_mutex);
>> +
>> /* Buffer for reporting pci register dumps. Its here in BSS, and
>> * not dynamically alloced, so that it ends up in RMO where RTAS
>> * can access it.
>> @@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
>> edev->mode &= ~EEH_DEV_SYSFS;
>> }
>>
>> +/**
>> + * eeh_dev_open - Mark EEH device and PE as passed through
>> + * @pdev: PCI device
>> + *
>> + * Mark the indicated EEH device and PE as passed through.
>> + * In the result, the EEH errors detected on the PE won't be
>> + * reported. The owner of the device will be responsible for
>> + * detection and recovery.
>> + */
>> +int eeh_dev_open(struct pci_dev *pdev)
>> +{
>> + struct eeh_dev *edev;
>> +
>> + mutex_lock(&eeh_dev_mutex);
>> +
>> + /* No PCI device ? */
>> + if (!pdev) {
>> + mutex_unlock(&eeh_dev_mutex);
>> + return -ENODEV;
>> + }
>> +
>> + /* No EEH device ? */
>> + edev = pci_dev_to_eeh_dev(pdev);
>> + if (!edev || !edev->pe) {
>> + mutex_unlock(&eeh_dev_mutex);
>> + return -ENODEV;
>> + }
>> +
>> + eeh_dev_set_passed(edev, true);
>> + eeh_pe_set_passed(edev->pe, true);
>> + mutex_unlock(&eeh_dev_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_dev_open);
>> +
>> +/**
>> + * eeh_dev_release - Reclaim the ownership of EEH device
>> + * @pdev: PCI device
>> + *
>> + * Reclaim ownership of EEH device, potentially the corresponding
>> + * PE. In the result, the EEH errors detected on the PE will be
>> + * reported and handled as usual.
>> + */
>> +void eeh_dev_release(struct pci_dev *pdev)
>> +{
>> + bool release_pe = true;
>> + struct eeh_pe *pe = NULL;
>> + struct eeh_dev *tmp, *edev;
>> +
>> + mutex_lock(&eeh_dev_mutex);
>> +
>> + /* No PCI device ? */
>> + if (!pdev) {
>> + mutex_unlock(&eeh_dev_mutex);
>> + return;
>> + }
>> +
>> + /* No EEH device ? */
>> + edev = pci_dev_to_eeh_dev(pdev);
>> + if (!edev || !eeh_dev_passed(edev) ||
>> + !edev->pe || !eeh_pe_passed(pe)) {
>> + mutex_unlock(&eeh_dev_mutex);
>> + return;
>> + }
>> +
>> + /* Release device */
>> + pe = edev->pe;
>> + eeh_dev_set_passed(edev, false);
>> +
>> + /* Release PE */
>> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> + if (eeh_dev_passed(edev)) {
>> + release_pe = false;
>> + break;
>> + }
>> + }
>> +
>> + if (release_pe)
>> + eeh_pe_set_passed(pe, false);
>> +
>> + mutex_unlock(&eeh_dev_mutex);
>> +}
>> +EXPORT_SYMBOL(eeh_dev_release);
>> +
>> +static int eeh_dev_check(struct pci_dev *pdev,
>> + struct eeh_dev **pedev,
>> + struct eeh_pe **ppe)
>> +{
>> + struct eeh_dev *edev;
>> +
>> + /* No device ? */
>> + if (!pdev)
>> + return -ENODEV;
>> +
>> + edev = pci_dev_to_eeh_dev(pdev);
>> + if (!edev || !eeh_dev_passed(edev) ||
>> + !edev->pe || !eeh_pe_passed(edev->pe))
>> + return -ENODEV;
>> +
>> + if (pedev)
>> + *pedev = edev;
>> + if (ppe)
>> + *ppe = edev->pe;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * eeh_pe_set_option - Set options for the indicated PE
>> + * @pdev: PCI device
>> + * @option: requested option
>> + *
>> + * The routine is called to enable or disable EEH functionality
>> + * on the indicated PE, to enable IO or DMA for the frozen PE.
>> + */
>> +int eeh_pe_set_option(struct pci_dev *pdev, int option)
>> +{
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int ret = 0;
>> +
>> + /* Device existing ? */
>> + ret = eeh_dev_check(pdev, &edev, &pe);
>> + if (ret)
>> + return ret;
>> +
>> + switch (option) {
>> + case EEH_OPT_DISABLE:
>> + case EEH_OPT_ENABLE:
>> + break;
>> + case EEH_OPT_THAW_MMIO:
>> + case EEH_OPT_THAW_DMA:
>> + if (!eeh_ops || !eeh_ops->set_option) {
>> + ret = -ENOENT;
>> + break;
>> + }
>> +
>> + ret = eeh_ops->set_option(pe, option);
>> + break;
>> + default:
>> + pr_debug("%s: Option %d out of range (%d, %d)\n",
>> + __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>> + ret = -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_pe_set_option);
>> +
>> +/**
>> + * eeh_pe_get_addr - Retrieve the PE address or sharing mode
>> + * @pdev: PCI device
>> + * @option: option
>> + *
>> + * Retrieve the PE address or sharing mode.
>> + */
>> +int eeh_pe_get_addr(struct pci_dev *pdev, int option)
>> +{
>> + struct pci_bus *bus;
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int ret = 0;
>> +
>> + /* Device existing ? */
>> + ret = eeh_dev_check(pdev, &edev, &pe);
>> + if (ret)
>> + return ret;
>> +
>> + switch (option) {
>> + case EEH_OPT_GET_PE_ADDR:
>> + bus = eeh_pe_bus_get(pe);
>> + if (!bus) {
>> + ret = -ENODEV;
>> + break;
>> + }
>> +
>> + /* PE address has format "00BBSS00" */
>> + ret = bus->number << 16;
>> + break;
>> + case EEH_OPT_GET_PE_MODE:
>> + /* Wa always have shared PE */
>> + ret = EEH_PE_MODE_SHARED;
>> + break;
>> + default:
>> + pr_debug("%s: option %d out of range (0, 1)\n",
>> + __func__, option);
>> + ret = -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
>> +
>> +/**
>> + * eeh_pe_get_state - Retrieve PE's state
>> + * @pdev: PCI device
>> + *
>> + * Retrieve the PE's state, which includes 3 aspects: enabled
>> + * DMA, enabled IO and asserted reset.
>> + */
>> +int eeh_pe_get_state(struct pci_dev *pdev)
>> +{
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int result, ret = 0;
>> +
>> + /* Device existing ? */
>> + ret = eeh_dev_check(pdev, &edev, &pe);
>> + if (ret)
>> + return ret;
>> +
>> + if (!eeh_ops || !eeh_ops->get_state)
>> + return -ENOENT;
>> +
>> + result = eeh_ops->get_state(pe, NULL);
>> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + (result & EEH_STATE_DMA_ENABLED) &&
>> + (result & EEH_STATE_MMIO_ENABLED))
>> + ret = EEH_PE_STATE_NORMAL;
>> + else if (result & EEH_STATE_RESET_ACTIVE)
>> + ret = EEH_PE_STATE_RESET;
>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + !(result & EEH_STATE_DMA_ENABLED) &&
>> + !(result & EEH_STATE_MMIO_ENABLED))
>> + ret = EEH_PE_STATE_STOPPED_IO_DMA;
>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + (result & EEH_STATE_DMA_ENABLED) &&
>> + !(result & EEH_STATE_MMIO_ENABLED))
>> + ret = EEH_PE_STATE_STOPPED_DMA;
>> + else
>> + ret = EEH_PE_STATE_UNAVAIL;
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_pe_get_state);
>> +
>> +/**
>> + * eeh_pe_reset - Issue PE reset according to specified type
>> + * @pdev: PCI device
>> + * @option: reset type
>> + *
>> + * The routine is called to reset the specified PE with the
>> + * indicated type, either fundamental reset or hot reset.
>> + * PE reset is the most important part for error recovery.
>> + */
>> +int eeh_pe_reset(struct pci_dev *pdev, int option)
>> +{
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int ret = 0;
>> +
>> + /* Device existing ? */
>> + ret = eeh_dev_check(pdev, &edev, &pe);
>> + if (ret)
>> + return ret;
>> +
>> + if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
>> + return -ENOENT;
>> +
>> + switch (option) {
>> + case EEH_RESET_DEACTIVATE:
>> + ret = eeh_ops->reset(pe, option);
>> + if (ret)
>> + break;
>> +
>> + /*
>> + * The PE is still in frozen state and we need clear
>> + * that. It's good to clear frozen state after deassert
>> + * to avoid messy IO access during reset, which might
>> + * cause recursive frozen PE.
>> + */
>> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
>> + if (!ret)
>> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
>> + if (!ret)
>> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>> + break;
>> + case EEH_RESET_HOT:
>> + case EEH_RESET_FUNDAMENTAL:
>> + ret = eeh_ops->reset(pe, option);
>> + break;
>> + default:
>> + pr_debug("%s: Unsupported option %d\n",
>> + __func__, option);
>> + ret = -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_pe_reset);
>> +
>> +/**
>> + * eeh_pe_configure - Configure PCI bridges after PE reset
>> + * @pdev: PCI device
>> + *
>> + * The routine is called to restore the PCI config space for
>> + * those PCI devices, especially PCI bridges affected by PE
>> + * reset issued previously.
>> + */
>> +int eeh_pe_configure(struct pci_dev *pdev)
>> +{
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int ret = 0;
>> +
>> + /* Device existing ? */
>> + ret = eeh_dev_check(pdev, &edev, &pe);
>> + if (ret)
>> + return ret;
>> +
>> + /* Restore config space for the affected devices */
>> + eeh_pe_restore_bars(pe);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_pe_configure);
>> +
>> static int proc_eeh_show(struct seq_file *m, void *v)
>> {
>> if (!eeh_enabled()) {
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index 7ba0424..301ac18 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -25,6 +25,9 @@
>> #include <linux/types.h>
>> #include <linux/uaccess.h>
>> #include <linux/vfio.h>
>> +#ifdef CONFIG_EEH
>> +#include <asm/eeh.h>
>> +#endif
>
>Can we make a vfio_pci_eeh file that properly stubs out anything called
>from common code? I don't want to see all these inline ifdefs.
>
well. do you want see drivers/vfio/vfio-pci/vfio_pci_eeh.c and export
following functins for vfio_pci.c to use?
vfio_pci_eeh_open()
vfio_pci_eeh_release()
vfio_pci_eeh_ioctl()
>>
>> #include "vfio_pci_private.h"
>>
>> @@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>> pci_restore_state(pdev);
>> }
>>
>> +static void vfio_eeh_pci_release(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_EEH
>> + eeh_dev_release(pdev);
>> +#endif
>> +}
>> +
>> static void vfio_pci_release(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>>
>> - if (atomic_dec_and_test(&vdev->refcnt))
>> + if (atomic_dec_and_test(&vdev->refcnt)) {
>> + vfio_eeh_pci_release(vdev->pdev);
>> vfio_pci_disable(vdev);
>> + }
>>
>> module_put(THIS_MODULE);
>> }
>>
>> +static int vfio_eeh_pci_open(struct pci_dev *pdev)
>> +{
>> + int ret = 0;
>> +
>> +#ifdef CONFIG_EEH
>> + ret = eeh_dev_open(pdev);
>> +#endif
>> + return ret;
>> +}
>> +
>> static int vfio_pci_open(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>> + int ret;
>>
>> if (!try_module_get(THIS_MODULE))
>> return -ENODEV;
>>
>> if (atomic_inc_return(&vdev->refcnt) == 1) {
>> - int ret = vfio_pci_enable(vdev);
>> - if (ret) {
>> - module_put(THIS_MODULE);
>> - return ret;
>> - }
>> + ret = vfio_pci_enable(vdev);
>> + if (ret)
>> + goto error;
>> +
>> + ret = vfio_eeh_pci_open(vdev->pdev);
>> + if (ret)
>> + goto error;
>> }
>>
>> return 0;
>> +error:
>> + module_put(THIS_MODULE);
>> + return ret;
>> }
>>
>> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
>> @@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
>> return walk.ret;
>> }
>>
>> +static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
>> + unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + unsigned long minsz;
>> + int ret = 0;
>> +
>> +#ifdef CONFIG_EEH
>> + switch (cmd) {
>> + case VFIO_EEH_PE_SET_OPTION: {
>> + struct vfio_eeh_pe_set_option option;
>> +
>> + minsz = offsetofend(struct vfio_eeh_pe_set_option, option);
>> + if (copy_from_user(&option, (void __user *)arg, minsz))
>> + return -EFAULT;
>> + if (option.argsz < minsz)
>> + return -EINVAL;
>> +
>> + ret = eeh_pe_set_option(pdev, option.option);
>> + break;
>> + }
>> + case VFIO_EEH_PE_GET_ADDR: {
>> + struct vfio_eeh_pe_get_addr addr;
>> +
>> + minsz = offsetofend(struct vfio_eeh_pe_get_addr, info);
>> + if (copy_from_user(&addr, (void __user *)arg, minsz))
>> + return -EFAULT;
>> + if (addr.argsz < minsz)
>> + return -EINVAL;
>> +
>> + ret = eeh_pe_get_addr(pdev, addr.option);
>> + if (ret >= 0) {
>> + addr.info = ret;
>> + if (copy_to_user((void __user *)arg, &addr, minsz))
>> + return -EFAULT;
>> + ret = 0;
>> + }
>> +
>> + break;
>> + }
>> + case VFIO_EEH_PE_GET_STATE: {
>> + struct vfio_eeh_pe_get_state state;
>> +
>> + minsz = offsetofend(struct vfio_eeh_pe_get_state, state);
>> + if (copy_from_user(&state, (void __user *)arg, minsz))
>> + return -EFAULT;
>> + if (state.argsz < minsz)
>> + return -EINVAL;
>> +
>> + ret = eeh_pe_get_state(pdev);
>> + if (ret >= 0) {
>> + state.state = ret;
>> + if (copy_to_user((void __user *)arg, &state, minsz))
>> + return -EFAULT;
>> + ret = 0;
>> + }
>> + break;
>> + }
>> + case VFIO_EEH_PE_RESET: {
>> + struct vfio_eeh_pe_reset reset;
>> +
>> + minsz = offsetofend(struct vfio_eeh_pe_reset, option);
>> + if (copy_from_user(&reset, (void __user *)arg, minsz))
>> + return -EFAULT;
>> + if (reset.argsz < minsz)
>> + return -EINVAL;
>> +
>> + ret = eeh_pe_reset(pdev, reset.option);
>> + break;
>> + }
>> + case VFIO_EEH_PE_CONFIGURE:
>> + ret = eeh_pe_configure(pdev);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + pr_debug("%s: Cannot handle command %d\n",
>> + __func__, cmd);
>> + }
>> +#else
>> + ret = -ENOENT;
>> +#endif
>> +
>> + return ret;
>> +}
>> +
>> static long vfio_pci_ioctl(void *device_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -682,6 +795,12 @@ hot_reset_release:
>>
>> kfree(groups);
>> return ret;
>> + } else if (cmd == VFIO_EEH_PE_SET_OPTION ||
>> + cmd == VFIO_EEH_PE_GET_ADDR ||
>> + cmd == VFIO_EEH_PE_GET_STATE ||
>> + cmd == VFIO_EEH_PE_RESET ||
>> + cmd == VFIO_EEH_PE_CONFIGURE) {
>> + return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
>> }
>>
>> return -ENOTTY;
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index cb9023d..ef55682 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>>
>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>
>> +/*
>> + * EEH functionality can be enabled or disabled on one specific device.
>> + * Also, the DMA or IO frozen state can be removed from the frozen PE
>> + * if required.
>> + */
>> +struct vfio_eeh_pe_set_option {
>> + __u32 argsz;
>> + __u32 option;
>> +};
>> +
>> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>
>What proposed ioctls are making you jump to 21?
>
>argsz is probably not as useful without a flags field. Otherwise the
>caller can't indicate what the extra space is.
>
The QEMU patches are based on Alexey's additional feature ("ddw"), which
consumed several ioctl commands.
ok. So you also prefer to remove "argsz"?
>> +
>> +/*
>> + * Each EEH PE should have unique address to be identified. The command
>> + * helps to retrieve the address and the sharing mode of the PE.
>> + */
>> +struct vfio_eeh_pe_get_addr {
>> + __u32 argsz;
>> + __u32 option;
>> + __u32 info;
>> +};
>> +
>> +#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
>> +
>> +/*
>> + * EEH PE might have been frozen because of PCI errors. Also, it might
>> + * be experiencing reset for error revoery. The following command helps
>> + * to get the state.
>> + */
>> +struct vfio_eeh_pe_get_state {
>> + __u32 argsz;
>> + __u32 state;
>> +};
>> +
>> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
>> +
>> +/*
>> + * Reset is the major step to recover problematic PE. The following
>> + * command helps on that.
>> + */
>> +struct vfio_eeh_pe_reset {
>> + __u32 argsz;
>> + __u32 option;
>> +};
>> +
>> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
>> +
>> +/*
>> + * One of the steps for recovery after PE reset is to configure the
>> + * PCI bridges affected by the PE reset.
>> + */
>> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
>
>What can the user do differently by making these separate ioctls?
>
hrm, I didn't understood as well. Alex.G could have the explaination.
>> +
>> /* ***************************************************************** */
>>
>> #endif /* _UAPIVFIO_H */
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Alex Williamson @ 2014-05-23 3:23 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, Alexander Graf, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <20140523003737.GA15136@shangw>
On Fri, 2014-05-23 at 10:37 +1000, Gavin Shan wrote:
> On Fri, May 23, 2014 at 10:17:30AM +1000, Gavin Shan wrote:
> >On Thu, May 22, 2014 at 11:55:29AM +0200, Alexander Graf wrote:
> >>On 22.05.14 10:23, Gavin Shan wrote:
>
> .../...
>
> >>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >>>index cb9023d..ef55682 100644
> >>>--- a/include/uapi/linux/vfio.h
> >>>+++ b/include/uapi/linux/vfio.h
> >>>@@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
> >>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> >>>+/*
> >>>+ * EEH functionality can be enabled or disabled on one specific device.
> >>>+ * Also, the DMA or IO frozen state can be removed from the frozen PE
> >>>+ * if required.
> >>>+ */
> >>>+struct vfio_eeh_pe_set_option {
> >>>+ __u32 argsz;
> >>
> >>What is this argsz thing? Is this your way of maintaining backwards
> >>compatibility when we introduce new fields? A new field will change
> >>the ioctl number, so I don't think that makes a lot of sense :).
> >>
> >>Just make the ioctl have a u32 as incoming argument. No fancy
> >>structs, no complicated code.
> >>
> >>The same applies for a number of structs below.
> >>
> >
> >ok. Will do in next revision.
> >
>
> Rechecked include/uapi/linux/vfio.h, the data struct for each ioctl command
> always has "argsz". I guess it was used as checker by Alex.W. Do you really
> want remove "argsz" ?
IIRC, this was actually a suggestion incorporated from David Gibson, but
using _IO with an argsz and flags field we can maintain compatibility
without bumping the ioctl number. It really only makes sense if we have
a flags field so we can identify what additional information is being
provided. Flags can be used as a bitmap of trailing structures or as
revision if we want a set of trailing structures that may change over
time. Unless you can come up with a good argument against it that would
prevent us inventing a new ioctl as soon as we need a minor tweak, I'd
prefer to keep it. As I noted in a previous comment, the one ioctl we
have for reset that doesn't take any options is likely going to be the
first ioctl that we need to entirely replace. If we don't keep argsz,
it seems like we probably need a flags field and reserved structures.
> >>>+ __u32 option;
> >>>+};
> >>>+
> >>>+#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
> >>>+
> >>>+/*
> >>>+ * Each EEH PE should have unique address to be identified. The command
> >>>+ * helps to retrieve the address and the sharing mode of the PE.
> >>>+ */
> >>>+struct vfio_eeh_pe_get_addr {
> >>>+ __u32 argsz;
> >>>+ __u32 option;
> >>>+ __u32 info;
> >>
> >>Any particular reason you need the info field? Can't the return value
> >>of the ioctl hold this? Then you only have a single u32 argument left
> >>to the ioctl again.
> >>
> >
> >ok. Will do in next revision.
> >
>
> If we eventually remove "argsz" and let ioctl() return value to hold
> information (or negative number for errors), we don't need any data
> struct because the 3rd parameter of ioctl() would be used as input
> and I only need one input parameter. Do you want see this ?
>
> Hopefully, Alex.W saw this and hasn't objections :)
I'm not sure why we're pushing for the minimal data set to pass to an
ioctl. Seems like a recipe for dead, useless ioctls. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Alex Williamson @ 2014-05-23 3:10 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, agraf, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-3-git-send-email-gwshan@linux.vnet.ibm.com>
On Thu, 2014-05-22 at 18:23 +1000, Gavin Shan wrote:
> The patch adds new IOCTL commands for VFIO PCI device to support
> EEH functionality for PCI devices, which have been passed through
> from host to somebody else via VFIO.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> Documentation/vfio.txt | 88 ++++++++++-
> arch/powerpc/include/asm/eeh.h | 17 +++
> arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++++++++
> drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
> include/uapi/linux/vfio.h | 53 +++++++
> 5 files changed, 603 insertions(+), 7 deletions(-)
Maybe a chicken and egg problem, but it seems like we could split the
platform code and vfio code into separate patches.
>
> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> index b9ca023..dd13db6 100644
> --- a/Documentation/vfio.txt
> +++ b/Documentation/vfio.txt
> @@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
> an excellent performance which has limitations such as inability to do
> locked pages accounting in real time.
>
> -So 3 additional ioctls have been added:
> +4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
> +which works on the basis of additional ioctl commands.
> +
> +So 8 additional ioctls have been added:
>
> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
> of the DMA window on the PCI bus.
> @@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
>
> VFIO_IOMMU_DISABLE - disables the container.
>
> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on the
> + specified device. Also, it can be used to remove IO or DMA
> + stopped state on the frozen PE.
> +
> + VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified
> + PE or query PE sharing mode.
> +
> + VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state.
> +
> + VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps for
> + error recovering.
> +
> + VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. It's
> + one of the major steps for error recoverying.
>
> The code flow from the example above should be slightly changed:
>
> @@ -346,6 +363,75 @@ The code flow from the example above should be slightly changed:
> ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
> .....
>
> +Based on the initial example we have, the following piece of code could be
> +reference for EEH setup and error handling:
> +
> + struct vfio_eeh_pe_set_option option = { .argsz = sizeof(option) };
> + struct vfio_eeh_pe_get_addr addr = { .argsz = sizeof(addr) };
> + struct vfio_eeh_pe_get_state state = { .argsz = sizeof(state) };
> + struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
> + struct vfio_eeh_pe_configure config = { .argsz = sizeof(config) };
> +
> + ....
> +
> + /* Get a file descriptor for the device */
> + device = ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
> +
> + /* Enable the EEH functionality on the device */
> + option.option = EEH_OPT_ENABLE;
> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
> +
> + /* Retrieve PE address and create and maintain PE by yourself */
> + addr.option = EEH_OPT_GET_PE_ADDR;
> + ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
> +
> + /* Assure EEH is supported on the PE and make PE functional */
> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
> +
> + /* Save device's state. pci_save_state() would be good enough
> + * as an example.
> + */
> +
> + /* Test and setup the device */
> + ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
> +
> + ....
> +
> + /* When 0xFF's returned from reading PCI config space or IO BARs
> + * of the PCI device. Check the PE state to see if that has been
> + * frozen.
> + */
> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
There's no notification, the user needs to observe the return value an
poll? Should we be enabling an eventfd to notify the user of the state
change?
> +
> + /* Waiting for pending PCI transactions to be completed and don't
> + * produce any more PCI traffic from/to the affected PE until
> + * recovery is finished.
> + */
> +
> + /* Enable IO for the affected PE and collect logs. Usually, the
> + * standard part of PCI config space, AER registers are dumped
> + * as logs for further analysis.
> + */
> + option.option = EEH_OPT_THAW_MMIO;
> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
How does the guest learn about the error? Does it need to?
> +
> + /* Issue PE reset */
> + reset.option = EEH_RESET_HOT;
> + ioctl(device, VFIO_EEH_PE_RESET, &reset);
> +
> + /* Configure the PCI bridges for the affected PE */
> + ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
> +
I'm not sure I see why we've split these into separate ioctls. FWIW,
the one ioctl we currently have for reset that takes no options is
probably going to be the first to get deprecated because of it.
> + /* Restored state we saved at initialization time. pci_restore_state()
> + * is good enough as an example.
> + */
> +
> + /* Hopefully, error is recovered successfully. Now, you can resume to
> + * start PCI traffic to/from the affected PE.
> + */
> +
> + ....
> +
> -------------------------------------------------------------------------------
>
> [1] VFIO was originally an acronym for "Virtual Function I/O" in its
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 34a2d83..dd5f1cf 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -191,6 +191,11 @@ enum {
> #define EEH_OPT_ENABLE 1 /* EEH enable */
> #define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
> #define EEH_OPT_THAW_DMA 3 /* DMA enable */
> +#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
> +#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
> +#define EEH_PE_MODE_NONE 0 /* Invalid mode */
> +#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
> +#define EEH_PE_MODE_SHARED 2 /* Shared mode */
> #define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
> #define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
> #define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
> @@ -198,6 +203,11 @@ enum {
> #define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
> #define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
> #define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
> +#define EEH_PE_STATE_NORMAL 0 /* Normal state */
> +#define EEH_PE_STATE_RESET 1 /* PE reset */
> +#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
> +#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
> +#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
> #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
> #define EEH_RESET_HOT 1 /* Hot reset */
> #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
> @@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
> void eeh_add_device_tree_late(struct pci_bus *);
> void eeh_add_sysfs_files(struct pci_bus *);
> void eeh_remove_device(struct pci_dev *);
> +int eeh_dev_open(struct pci_dev *pdev);
> +void eeh_dev_release(struct pci_dev *pdev);
> +int eeh_pe_set_option(struct pci_dev *pdev, int option);
> +int eeh_pe_get_addr(struct pci_dev *pdev, int option);
> +int eeh_pe_get_state(struct pci_dev *pdev);
> +int eeh_pe_reset(struct pci_dev *pdev, int option);
> +int eeh_pe_configure(struct pci_dev *pdev);
>
> /**
> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 9c6b899..b90a474 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops = NULL;
> /* Lock to avoid races due to multiple reports of an error */
> DEFINE_RAW_SPINLOCK(confirm_error_lock);
>
> +/* Lock to protect passed flags */
> +static DEFINE_MUTEX(eeh_dev_mutex);
> +
> /* Buffer for reporting pci register dumps. Its here in BSS, and
> * not dynamically alloced, so that it ends up in RMO where RTAS
> * can access it.
> @@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
> edev->mode &= ~EEH_DEV_SYSFS;
> }
>
> +/**
> + * eeh_dev_open - Mark EEH device and PE as passed through
> + * @pdev: PCI device
> + *
> + * Mark the indicated EEH device and PE as passed through.
> + * In the result, the EEH errors detected on the PE won't be
> + * reported. The owner of the device will be responsible for
> + * detection and recovery.
> + */
> +int eeh_dev_open(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> +
> + mutex_lock(&eeh_dev_mutex);
> +
> + /* No PCI device ? */
> + if (!pdev) {
> + mutex_unlock(&eeh_dev_mutex);
> + return -ENODEV;
> + }
> +
> + /* No EEH device ? */
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !edev->pe) {
> + mutex_unlock(&eeh_dev_mutex);
> + return -ENODEV;
> + }
> +
> + eeh_dev_set_passed(edev, true);
> + eeh_pe_set_passed(edev->pe, true);
> + mutex_unlock(&eeh_dev_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(eeh_dev_open);
> +
> +/**
> + * eeh_dev_release - Reclaim the ownership of EEH device
> + * @pdev: PCI device
> + *
> + * Reclaim ownership of EEH device, potentially the corresponding
> + * PE. In the result, the EEH errors detected on the PE will be
> + * reported and handled as usual.
> + */
> +void eeh_dev_release(struct pci_dev *pdev)
> +{
> + bool release_pe = true;
> + struct eeh_pe *pe = NULL;
> + struct eeh_dev *tmp, *edev;
> +
> + mutex_lock(&eeh_dev_mutex);
> +
> + /* No PCI device ? */
> + if (!pdev) {
> + mutex_unlock(&eeh_dev_mutex);
> + return;
> + }
> +
> + /* No EEH device ? */
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !eeh_dev_passed(edev) ||
> + !edev->pe || !eeh_pe_passed(pe)) {
> + mutex_unlock(&eeh_dev_mutex);
> + return;
> + }
> +
> + /* Release device */
> + pe = edev->pe;
> + eeh_dev_set_passed(edev, false);
> +
> + /* Release PE */
> + eeh_pe_for_each_dev(pe, edev, tmp) {
> + if (eeh_dev_passed(edev)) {
> + release_pe = false;
> + break;
> + }
> + }
> +
> + if (release_pe)
> + eeh_pe_set_passed(pe, false);
> +
> + mutex_unlock(&eeh_dev_mutex);
> +}
> +EXPORT_SYMBOL(eeh_dev_release);
> +
> +static int eeh_dev_check(struct pci_dev *pdev,
> + struct eeh_dev **pedev,
> + struct eeh_pe **ppe)
> +{
> + struct eeh_dev *edev;
> +
> + /* No device ? */
> + if (!pdev)
> + return -ENODEV;
> +
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !eeh_dev_passed(edev) ||
> + !edev->pe || !eeh_pe_passed(edev->pe))
> + return -ENODEV;
> +
> + if (pedev)
> + *pedev = edev;
> + if (ppe)
> + *ppe = edev->pe;
> +
> + return 0;
> +}
> +
> +/**
> + * eeh_pe_set_option - Set options for the indicated PE
> + * @pdev: PCI device
> + * @option: requested option
> + *
> + * The routine is called to enable or disable EEH functionality
> + * on the indicated PE, to enable IO or DMA for the frozen PE.
> + */
> +int eeh_pe_set_option(struct pci_dev *pdev, int option)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + switch (option) {
> + case EEH_OPT_DISABLE:
> + case EEH_OPT_ENABLE:
> + break;
> + case EEH_OPT_THAW_MMIO:
> + case EEH_OPT_THAW_DMA:
> + if (!eeh_ops || !eeh_ops->set_option) {
> + ret = -ENOENT;
> + break;
> + }
> +
> + ret = eeh_ops->set_option(pe, option);
> + break;
> + default:
> + pr_debug("%s: Option %d out of range (%d, %d)\n",
> + __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_set_option);
> +
> +/**
> + * eeh_pe_get_addr - Retrieve the PE address or sharing mode
> + * @pdev: PCI device
> + * @option: option
> + *
> + * Retrieve the PE address or sharing mode.
> + */
> +int eeh_pe_get_addr(struct pci_dev *pdev, int option)
> +{
> + struct pci_bus *bus;
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + switch (option) {
> + case EEH_OPT_GET_PE_ADDR:
> + bus = eeh_pe_bus_get(pe);
> + if (!bus) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + /* PE address has format "00BBSS00" */
> + ret = bus->number << 16;
> + break;
> + case EEH_OPT_GET_PE_MODE:
> + /* Wa always have shared PE */
> + ret = EEH_PE_MODE_SHARED;
> + break;
> + default:
> + pr_debug("%s: option %d out of range (0, 1)\n",
> + __func__, option);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
> +
> +/**
> + * eeh_pe_get_state - Retrieve PE's state
> + * @pdev: PCI device
> + *
> + * Retrieve the PE's state, which includes 3 aspects: enabled
> + * DMA, enabled IO and asserted reset.
> + */
> +int eeh_pe_get_state(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int result, ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + if (!eeh_ops || !eeh_ops->get_state)
> + return -ENOENT;
> +
> + result = eeh_ops->get_state(pe, NULL);
> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + (result & EEH_STATE_DMA_ENABLED) &&
> + (result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_NORMAL;
> + else if (result & EEH_STATE_RESET_ACTIVE)
> + ret = EEH_PE_STATE_RESET;
> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + !(result & EEH_STATE_DMA_ENABLED) &&
> + !(result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_STOPPED_IO_DMA;
> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + (result & EEH_STATE_DMA_ENABLED) &&
> + !(result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_STOPPED_DMA;
> + else
> + ret = EEH_PE_STATE_UNAVAIL;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_get_state);
> +
> +/**
> + * eeh_pe_reset - Issue PE reset according to specified type
> + * @pdev: PCI device
> + * @option: reset type
> + *
> + * The routine is called to reset the specified PE with the
> + * indicated type, either fundamental reset or hot reset.
> + * PE reset is the most important part for error recovery.
> + */
> +int eeh_pe_reset(struct pci_dev *pdev, int option)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
> + return -ENOENT;
> +
> + switch (option) {
> + case EEH_RESET_DEACTIVATE:
> + ret = eeh_ops->reset(pe, option);
> + if (ret)
> + break;
> +
> + /*
> + * The PE is still in frozen state and we need clear
> + * that. It's good to clear frozen state after deassert
> + * to avoid messy IO access during reset, which might
> + * cause recursive frozen PE.
> + */
> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
> + if (!ret)
> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
> + if (!ret)
> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
> + break;
> + case EEH_RESET_HOT:
> + case EEH_RESET_FUNDAMENTAL:
> + ret = eeh_ops->reset(pe, option);
> + break;
> + default:
> + pr_debug("%s: Unsupported option %d\n",
> + __func__, option);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_reset);
> +
> +/**
> + * eeh_pe_configure - Configure PCI bridges after PE reset
> + * @pdev: PCI device
> + *
> + * The routine is called to restore the PCI config space for
> + * those PCI devices, especially PCI bridges affected by PE
> + * reset issued previously.
> + */
> +int eeh_pe_configure(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + /* Restore config space for the affected devices */
> + eeh_pe_restore_bars(pe);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_configure);
> +
> static int proc_eeh_show(struct seq_file *m, void *v)
> {
> if (!eeh_enabled()) {
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 7ba0424..301ac18 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -25,6 +25,9 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> +#ifdef CONFIG_EEH
> +#include <asm/eeh.h>
> +#endif
Can we make a vfio_pci_eeh file that properly stubs out anything called
from common code? I don't want to see all these inline ifdefs.
>
> #include "vfio_pci_private.h"
>
> @@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> pci_restore_state(pdev);
> }
>
> +static void vfio_eeh_pci_release(struct pci_dev *pdev)
> +{
> +#ifdef CONFIG_EEH
> + eeh_dev_release(pdev);
> +#endif
> +}
> +
> static void vfio_pci_release(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
>
> - if (atomic_dec_and_test(&vdev->refcnt))
> + if (atomic_dec_and_test(&vdev->refcnt)) {
> + vfio_eeh_pci_release(vdev->pdev);
> vfio_pci_disable(vdev);
> + }
>
> module_put(THIS_MODULE);
> }
>
> +static int vfio_eeh_pci_open(struct pci_dev *pdev)
> +{
> + int ret = 0;
> +
> +#ifdef CONFIG_EEH
> + ret = eeh_dev_open(pdev);
> +#endif
> + return ret;
> +}
> +
> static int vfio_pci_open(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
> + int ret;
>
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
>
> if (atomic_inc_return(&vdev->refcnt) == 1) {
> - int ret = vfio_pci_enable(vdev);
> - if (ret) {
> - module_put(THIS_MODULE);
> - return ret;
> - }
> + ret = vfio_pci_enable(vdev);
> + if (ret)
> + goto error;
> +
> + ret = vfio_eeh_pci_open(vdev->pdev);
> + if (ret)
> + goto error;
> }
>
> return 0;
> +error:
> + module_put(THIS_MODULE);
> + return ret;
> }
>
> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
> @@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
> return walk.ret;
> }
>
> +static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + unsigned long minsz;
> + int ret = 0;
> +
> +#ifdef CONFIG_EEH
> + switch (cmd) {
> + case VFIO_EEH_PE_SET_OPTION: {
> + struct vfio_eeh_pe_set_option option;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_set_option, option);
> + if (copy_from_user(&option, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (option.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_set_option(pdev, option.option);
> + break;
> + }
> + case VFIO_EEH_PE_GET_ADDR: {
> + struct vfio_eeh_pe_get_addr addr;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_get_addr, info);
> + if (copy_from_user(&addr, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (addr.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_get_addr(pdev, addr.option);
> + if (ret >= 0) {
> + addr.info = ret;
> + if (copy_to_user((void __user *)arg, &addr, minsz))
> + return -EFAULT;
> + ret = 0;
> + }
> +
> + break;
> + }
> + case VFIO_EEH_PE_GET_STATE: {
> + struct vfio_eeh_pe_get_state state;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_get_state, state);
> + if (copy_from_user(&state, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (state.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_get_state(pdev);
> + if (ret >= 0) {
> + state.state = ret;
> + if (copy_to_user((void __user *)arg, &state, minsz))
> + return -EFAULT;
> + ret = 0;
> + }
> + break;
> + }
> + case VFIO_EEH_PE_RESET: {
> + struct vfio_eeh_pe_reset reset;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_reset, option);
> + if (copy_from_user(&reset, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (reset.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_reset(pdev, reset.option);
> + break;
> + }
> + case VFIO_EEH_PE_CONFIGURE:
> + ret = eeh_pe_configure(pdev);
> + break;
> + default:
> + ret = -EINVAL;
> + pr_debug("%s: Cannot handle command %d\n",
> + __func__, cmd);
> + }
> +#else
> + ret = -ENOENT;
> +#endif
> +
> + return ret;
> +}
> +
> static long vfio_pci_ioctl(void *device_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -682,6 +795,12 @@ hot_reset_release:
>
> kfree(groups);
> return ret;
> + } else if (cmd == VFIO_EEH_PE_SET_OPTION ||
> + cmd == VFIO_EEH_PE_GET_ADDR ||
> + cmd == VFIO_EEH_PE_GET_STATE ||
> + cmd == VFIO_EEH_PE_RESET ||
> + cmd == VFIO_EEH_PE_CONFIGURE) {
> + return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
> }
>
> return -ENOTTY;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index cb9023d..ef55682 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>
> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>
> +/*
> + * EEH functionality can be enabled or disabled on one specific device.
> + * Also, the DMA or IO frozen state can be removed from the frozen PE
> + * if required.
> + */
> +struct vfio_eeh_pe_set_option {
> + __u32 argsz;
> + __u32 option;
> +};
> +
> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
What proposed ioctls are making you jump to 21?
argsz is probably not as useful without a flags field. Otherwise the
caller can't indicate what the extra space is.
> +
> +/*
> + * Each EEH PE should have unique address to be identified. The command
> + * helps to retrieve the address and the sharing mode of the PE.
> + */
> +struct vfio_eeh_pe_get_addr {
> + __u32 argsz;
> + __u32 option;
> + __u32 info;
> +};
> +
> +#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
> +
> +/*
> + * EEH PE might have been frozen because of PCI errors. Also, it might
> + * be experiencing reset for error revoery. The following command helps
> + * to get the state.
> + */
> +struct vfio_eeh_pe_get_state {
> + __u32 argsz;
> + __u32 state;
> +};
> +
> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
> +
> +/*
> + * Reset is the major step to recover problematic PE. The following
> + * command helps on that.
> + */
> +struct vfio_eeh_pe_reset {
> + __u32 argsz;
> + __u32 option;
> +};
> +
> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
> +
> +/*
> + * One of the steps for recovery after PE reset is to configure the
> + * PCI bridges affected by the PE reset.
> + */
> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
What can the user do differently by making these separate ioctls?
> +
> /* ***************************************************************** */
>
> #endif /* _UAPIVFIO_H */
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-23 0:37 UTC (permalink / raw)
To: Gavin Shan
Cc: aik, Alexander Graf, kvm-ppc, alex.williamson, qiudayu,
linuxppc-dev
In-Reply-To: <20140523001730.GA12584@shangw>
On Fri, May 23, 2014 at 10:17:30AM +1000, Gavin Shan wrote:
>On Thu, May 22, 2014 at 11:55:29AM +0200, Alexander Graf wrote:
>>On 22.05.14 10:23, Gavin Shan wrote:
.../...
>>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>index cb9023d..ef55682 100644
>>>--- a/include/uapi/linux/vfio.h
>>>+++ b/include/uapi/linux/vfio.h
>>>@@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>>+/*
>>>+ * EEH functionality can be enabled or disabled on one specific device.
>>>+ * Also, the DMA or IO frozen state can be removed from the frozen PE
>>>+ * if required.
>>>+ */
>>>+struct vfio_eeh_pe_set_option {
>>>+ __u32 argsz;
>>
>>What is this argsz thing? Is this your way of maintaining backwards
>>compatibility when we introduce new fields? A new field will change
>>the ioctl number, so I don't think that makes a lot of sense :).
>>
>>Just make the ioctl have a u32 as incoming argument. No fancy
>>structs, no complicated code.
>>
>>The same applies for a number of structs below.
>>
>
>ok. Will do in next revision.
>
Rechecked include/uapi/linux/vfio.h, the data struct for each ioctl command
always has "argsz". I guess it was used as checker by Alex.W. Do you really
want remove "argsz" ?
>>>+ __u32 option;
>>>+};
>>>+
>>>+#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>>>+
>>>+/*
>>>+ * Each EEH PE should have unique address to be identified. The command
>>>+ * helps to retrieve the address and the sharing mode of the PE.
>>>+ */
>>>+struct vfio_eeh_pe_get_addr {
>>>+ __u32 argsz;
>>>+ __u32 option;
>>>+ __u32 info;
>>
>>Any particular reason you need the info field? Can't the return value
>>of the ioctl hold this? Then you only have a single u32 argument left
>>to the ioctl again.
>>
>
>ok. Will do in next revision.
>
If we eventually remove "argsz" and let ioctl() return value to hold
information (or negative number for errors), we don't need any data
struct because the 3rd parameter of ioctl() would be used as input
and I only need one input parameter. Do you want see this ?
Hopefully, Alex.W saw this and hasn't objections :)
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v2] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Stephen Rothwell @ 2014-05-23 0:20 UTC (permalink / raw)
To: Cody P Schafer
Cc: peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <537E8E1F.9020303@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 393 bytes --]
Hi Cody,
On Thu, 22 May 2014 16:54:07 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
> I've got a slight preference toward keeping it as is, which lets all of
> the non-error path code stay outside of if/else blocks (and the error
> handling is kept ever so slightly more consistent).
No problem.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-23 0:17 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537DC991.9010401@suse.de>
On Thu, May 22, 2014 at 11:55:29AM +0200, Alexander Graf wrote:
>
>On 22.05.14 10:23, Gavin Shan wrote:
>>The patch adds new IOCTL commands for VFIO PCI device to support
>>EEH functionality for PCI devices, which have been passed through
>>from host to somebody else via VFIO.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>This already looks a *lot* more sane than the previous versions.
>We're slowly getting there I think ;).
>
>Ben, could you please check through the exported EEH interface itself
>and verify whether it does all the lockings correctly, uses the API
>properly and doesn't allow for a user space program to blow up the
>system?
>
>
>>---
>> Documentation/vfio.txt | 88 ++++++++++-
>> arch/powerpc/include/asm/eeh.h | 17 +++
>> arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
>> include/uapi/linux/vfio.h | 53 +++++++
>> 5 files changed, 603 insertions(+), 7 deletions(-)
>>
>>diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>>index b9ca023..dd13db6 100644
>>--- a/Documentation/vfio.txt
>>+++ b/Documentation/vfio.txt
>>@@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
>> an excellent performance which has limitations such as inability to do
>> locked pages accounting in real time.
>>-So 3 additional ioctls have been added:
>>+4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
>>+which works on the basis of additional ioctl commands.
>>+
>>+So 8 additional ioctls have been added:
>> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
>> of the DMA window on the PCI bus.
>>@@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
>> VFIO_IOMMU_DISABLE - disables the container.
>>+ VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on the
>
>functionality
>
Typo. Will fix in next revision.
>>+ specified device. Also, it can be used to remove IO or DMA
>>+ stopped state on the frozen PE.
>>+
>>+ VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified
>>+ PE or query PE sharing mode.
>
>What is PE?
>
will add description about that to Documentation/vfio.txt.
>>+
>>+ VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state.
>>+
>>+ VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps for
>>+ error recovering.
>>+
>>+ VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. It's
>>+ one of the major steps for error recoverying.
>> The code flow from the example above should be slightly changed:
>>@@ -346,6 +363,75 @@ The code flow from the example above should be slightly changed:
>> ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
>> .....
>>+Based on the initial example we have, the following piece of code could be
>>+reference for EEH setup and error handling:
>>+
>>+ struct vfio_eeh_pe_set_option option = { .argsz = sizeof(option) };
>>+ struct vfio_eeh_pe_get_addr addr = { .argsz = sizeof(addr) };
>>+ struct vfio_eeh_pe_get_state state = { .argsz = sizeof(state) };
>>+ struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
>>+ struct vfio_eeh_pe_configure config = { .argsz = sizeof(config) };
>>+
>>+ ....
>>+
>>+ /* Get a file descriptor for the device */
>>+ device = ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
>>+
>>+ /* Enable the EEH functionality on the device */
>>+ option.option = EEH_OPT_ENABLE;
>>+ ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>>+
>>+ /* Retrieve PE address and create and maintain PE by yourself */
>>+ addr.option = EEH_OPT_GET_PE_ADDR;
>>+ ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
>>+
>>+ /* Assure EEH is supported on the PE and make PE functional */
>>+ ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>>+
>>+ /* Save device's state. pci_save_state() would be good enough
>>+ * as an example.
>>+ */
>>+
>>+ /* Test and setup the device */
>>+ ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
>>+
>>+ ....
>>+
>>+ /* When 0xFF's returned from reading PCI config space or IO BARs
>>+ * of the PCI device. Check the PE state to see if that has been
>>+ * frozen.
>>+ */
>>+ ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
>>+
>>+ /* Waiting for pending PCI transactions to be completed and don't
>>+ * produce any more PCI traffic from/to the affected PE until
>>+ * recovery is finished.
>>+ */
>>+
>>+ /* Enable IO for the affected PE and collect logs. Usually, the
>>+ * standard part of PCI config space, AER registers are dumped
>>+ * as logs for further analysis.
>>+ */
>>+ option.option = EEH_OPT_THAW_MMIO;
>>+ ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
>>+
>>+ /* Issue PE reset */
>>+ reset.option = EEH_RESET_HOT;
>>+ ioctl(device, VFIO_EEH_PE_RESET, &reset);
>>+
>>+ /* Configure the PCI bridges for the affected PE */
>>+ ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
>>+
>>+ /* Restored state we saved at initialization time. pci_restore_state()
>>+ * is good enough as an example.
>>+ */
>>+
>>+ /* Hopefully, error is recovered successfully. Now, you can resume to
>>+ * start PCI traffic to/from the affected PE.
>>+ */
>>+
>>+ ....
>>+
>> -------------------------------------------------------------------------------
>> [1] VFIO was originally an acronym for "Virtual Function I/O" in its
>>diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>>index 34a2d83..dd5f1cf 100644
>>--- a/arch/powerpc/include/asm/eeh.h
>>+++ b/arch/powerpc/include/asm/eeh.h
>>@@ -191,6 +191,11 @@ enum {
>> #define EEH_OPT_ENABLE 1 /* EEH enable */
>> #define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
>> #define EEH_OPT_THAW_DMA 3 /* DMA enable */
>>+#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
>>+#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
>>+#define EEH_PE_MODE_NONE 0 /* Invalid mode */
>>+#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
>>+#define EEH_PE_MODE_SHARED 2 /* Shared mode */
>> #define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
>> #define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
>> #define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
>>@@ -198,6 +203,11 @@ enum {
>> #define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
>> #define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
>> #define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
>>+#define EEH_PE_STATE_NORMAL 0 /* Normal state */
>>+#define EEH_PE_STATE_RESET 1 /* PE reset */
>>+#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
>>+#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
>>+#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
>> #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
>> #define EEH_RESET_HOT 1 /* Hot reset */
>> #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
>>@@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
>> void eeh_add_device_tree_late(struct pci_bus *);
>> void eeh_add_sysfs_files(struct pci_bus *);
>> void eeh_remove_device(struct pci_dev *);
>>+int eeh_dev_open(struct pci_dev *pdev);
>>+void eeh_dev_release(struct pci_dev *pdev);
>>+int eeh_pe_set_option(struct pci_dev *pdev, int option);
>>+int eeh_pe_get_addr(struct pci_dev *pdev, int option);
>>+int eeh_pe_get_state(struct pci_dev *pdev);
>>+int eeh_pe_reset(struct pci_dev *pdev, int option);
>>+int eeh_pe_configure(struct pci_dev *pdev);
>> /**
>> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
>>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>index 9c6b899..b90a474 100644
>>--- a/arch/powerpc/kernel/eeh.c
>>+++ b/arch/powerpc/kernel/eeh.c
>>@@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops = NULL;
>> /* Lock to avoid races due to multiple reports of an error */
>> DEFINE_RAW_SPINLOCK(confirm_error_lock);
>>+/* Lock to protect passed flags */
>>+static DEFINE_MUTEX(eeh_dev_mutex);
>>+
>> /* Buffer for reporting pci register dumps. Its here in BSS, and
>> * not dynamically alloced, so that it ends up in RMO where RTAS
>> * can access it.
>>@@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
>> edev->mode &= ~EEH_DEV_SYSFS;
>> }
>>+/**
>>+ * eeh_dev_open - Mark EEH device and PE as passed through
>>+ * @pdev: PCI device
>>+ *
>>+ * Mark the indicated EEH device and PE as passed through.
>>+ * In the result, the EEH errors detected on the PE won't be
>>+ * reported. The owner of the device will be responsible for
>>+ * detection and recovery.
>>+ */
>>+int eeh_dev_open(struct pci_dev *pdev)
>>+{
>>+ struct eeh_dev *edev;
>>+
>>+ mutex_lock(&eeh_dev_mutex);
>>+
>>+ /* No PCI device ? */
>>+ if (!pdev) {
>>+ mutex_unlock(&eeh_dev_mutex);
>>+ return -ENODEV;
>>+ }
>>+
>>+ /* No EEH device ? */
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !edev->pe) {
>>+ mutex_unlock(&eeh_dev_mutex);
>>+ return -ENODEV;
>>+ }
>>+
>>+ eeh_dev_set_passed(edev, true);
>>+ eeh_pe_set_passed(edev->pe, true);
>>+ mutex_unlock(&eeh_dev_mutex);
>>+
>>+ return 0;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_dev_open);
>>+
>>+/**
>>+ * eeh_dev_release - Reclaim the ownership of EEH device
>>+ * @pdev: PCI device
>>+ *
>>+ * Reclaim ownership of EEH device, potentially the corresponding
>>+ * PE. In the result, the EEH errors detected on the PE will be
>>+ * reported and handled as usual.
>>+ */
>>+void eeh_dev_release(struct pci_dev *pdev)
>>+{
>>+ bool release_pe = true;
>>+ struct eeh_pe *pe = NULL;
>>+ struct eeh_dev *tmp, *edev;
>>+
>>+ mutex_lock(&eeh_dev_mutex);
>>+
>>+ /* No PCI device ? */
>>+ if (!pdev) {
>>+ mutex_unlock(&eeh_dev_mutex);
>>+ return;
>>+ }
>>+
>>+ /* No EEH device ? */
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !eeh_dev_passed(edev) ||
>>+ !edev->pe || !eeh_pe_passed(pe)) {
>>+ mutex_unlock(&eeh_dev_mutex);
>>+ return;
>>+ }
>>+
>>+ /* Release device */
>>+ pe = edev->pe;
>>+ eeh_dev_set_passed(edev, false);
>>+
>>+ /* Release PE */
>>+ eeh_pe_for_each_dev(pe, edev, tmp) {
>>+ if (eeh_dev_passed(edev)) {
>>+ release_pe = false;
>>+ break;
>>+ }
>>+ }
>>+
>>+ if (release_pe)
>>+ eeh_pe_set_passed(pe, false);
>>+
>>+ mutex_unlock(&eeh_dev_mutex);
>>+}
>>+EXPORT_SYMBOL(eeh_dev_release);
>>+
>>+static int eeh_dev_check(struct pci_dev *pdev,
>>+ struct eeh_dev **pedev,
>>+ struct eeh_pe **ppe)
>>+{
>>+ struct eeh_dev *edev;
>>+
>>+ /* No device ? */
>>+ if (!pdev)
>>+ return -ENODEV;
>>+
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !eeh_dev_passed(edev) ||
>>+ !edev->pe || !eeh_pe_passed(edev->pe))
>>+ return -ENODEV;
>>+
>>+ if (pedev)
>>+ *pedev = edev;
>>+ if (ppe)
>>+ *ppe = edev->pe;
>>+
>>+ return 0;
>>+}
>>+
>>+/**
>>+ * eeh_pe_set_option - Set options for the indicated PE
>>+ * @pdev: PCI device
>>+ * @option: requested option
>>+ *
>>+ * The routine is called to enable or disable EEH functionality
>>+ * on the indicated PE, to enable IO or DMA for the frozen PE.
>>+ */
>>+int eeh_pe_set_option(struct pci_dev *pdev, int option)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_dev_check(pdev, &edev, &pe);
>>+ if (ret)
>>+ return ret;
>>+
>>+ switch (option) {
>>+ case EEH_OPT_DISABLE:
>>+ case EEH_OPT_ENABLE:
>
>This deserves a comment
>
Yep. will add it, thanks.
>>+ break;
>>+ case EEH_OPT_THAW_MMIO:
>>+ case EEH_OPT_THAW_DMA:
>>+ if (!eeh_ops || !eeh_ops->set_option) {
>>+ ret = -ENOENT;
>>+ break;
>>+ }
>>+
>>+ ret = eeh_ops->set_option(pe, option);
>>+ break;
>>+ default:
>>+ pr_debug("%s: Option %d out of range (%d, %d)\n",
>>+ __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>>+ ret = -EINVAL;
>>+ }
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_pe_set_option);
>>+
>>+/**
>>+ * eeh_pe_get_addr - Retrieve the PE address or sharing mode
>>+ * @pdev: PCI device
>>+ * @option: option
>>+ *
>>+ * Retrieve the PE address or sharing mode.
>>+ */
>>+int eeh_pe_get_addr(struct pci_dev *pdev, int option)
>>+{
>>+ struct pci_bus *bus;
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_dev_check(pdev, &edev, &pe);
>>+ if (ret)
>
>Probably safer to write this as ret < 0. Positive return values are
>success now.
>
Ok. Will fix it.
>>+ return ret;
>>+
>>+ switch (option) {
>>+ case EEH_OPT_GET_PE_ADDR:
>>+ bus = eeh_pe_bus_get(pe);
>>+ if (!bus) {
>>+ ret = -ENODEV;
>>+ break;
>>+ }
>>+
>>+ /* PE address has format "00BBSS00" */
>>+ ret = bus->number << 16;
>>+ break;
>>+ case EEH_OPT_GET_PE_MODE:
>>+ /* Wa always have shared PE */
>
>Wa?
>
Basically, PE could have one single PCI device (function), or a PCI bus
(including subordinate PCI devices). we always have the later case currently.
And it's called PE in "shared mode" and I called it as "shared PE" :-)
>>+ ret = EEH_PE_MODE_SHARED;
>>+ break;
>>+ default:
>>+ pr_debug("%s: option %d out of range (0, 1)\n",
>>+ __func__, option);
>>+ ret = -EINVAL;
>>+ }
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
>>+
>>+/**
>>+ * eeh_pe_get_state - Retrieve PE's state
>>+ * @pdev: PCI device
>>+ *
>>+ * Retrieve the PE's state, which includes 3 aspects: enabled
>>+ * DMA, enabled IO and asserted reset.
>>+ */
>>+int eeh_pe_get_state(struct pci_dev *pdev)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int result, ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_dev_check(pdev, &edev, &pe);
>>+ if (ret)
>>+ return ret;
>>+
>>+ if (!eeh_ops || !eeh_ops->get_state)
>>+ return -ENOENT;
>>+
>>+ result = eeh_ops->get_state(pe, NULL);
>>+ if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ (result & EEH_STATE_DMA_ENABLED) &&
>>+ (result & EEH_STATE_MMIO_ENABLED))
>>+ ret = EEH_PE_STATE_NORMAL;
>>+ else if (result & EEH_STATE_RESET_ACTIVE)
>>+ ret = EEH_PE_STATE_RESET;
>>+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ !(result & EEH_STATE_DMA_ENABLED) &&
>>+ !(result & EEH_STATE_MMIO_ENABLED))
>>+ ret = EEH_PE_STATE_STOPPED_IO_DMA;
>>+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ (result & EEH_STATE_DMA_ENABLED) &&
>>+ !(result & EEH_STATE_MMIO_ENABLED))
>>+ ret = EEH_PE_STATE_STOPPED_DMA;
>>+ else
>>+ ret = EEH_PE_STATE_UNAVAIL;
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_pe_get_state);
>>+
>>+/**
>>+ * eeh_pe_reset - Issue PE reset according to specified type
>>+ * @pdev: PCI device
>>+ * @option: reset type
>>+ *
>>+ * The routine is called to reset the specified PE with the
>>+ * indicated type, either fundamental reset or hot reset.
>>+ * PE reset is the most important part for error recovery.
>>+ */
>>+int eeh_pe_reset(struct pci_dev *pdev, int option)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_dev_check(pdev, &edev, &pe);
>>+ if (ret)
>>+ return ret;
>>+
>>+ if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
>>+ return -ENOENT;
>>+
>>+ switch (option) {
>>+ case EEH_RESET_DEACTIVATE:
>>+ ret = eeh_ops->reset(pe, option);
>>+ if (ret)
>>+ break;
>>+
>>+ /*
>>+ * The PE is still in frozen state and we need clear
>
>to
>
Will fix, thanks.
>>+ * that. It's good to clear frozen state after deassert
>>+ * to avoid messy IO access during reset, which might
>>+ * cause recursive frozen PE.
>>+ */
>>+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
>>+ if (!ret)
>>+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
>>+ if (!ret)
>>+ eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>>+ break;
>>+ case EEH_RESET_HOT:
>>+ case EEH_RESET_FUNDAMENTAL:
>>+ ret = eeh_ops->reset(pe, option);
>>+ break;
>>+ default:
>>+ pr_debug("%s: Unsupported option %d\n",
>>+ __func__, option);
>>+ ret = -EINVAL;
>>+ }
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_pe_reset);
>>+
>>+/**
>>+ * eeh_pe_configure - Configure PCI bridges after PE reset
>>+ * @pdev: PCI device
>>+ *
>>+ * The routine is called to restore the PCI config space for
>>+ * those PCI devices, especially PCI bridges affected by PE
>>+ * reset issued previously.
>
>So would it make sense to combine this with the reset call?
>
I hope to keep it as it's one of the major steps to do error
recovery. Lets keep it.
>>+ */
>>+int eeh_pe_configure(struct pci_dev *pdev)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_dev_check(pdev, &edev, &pe);
>>+ if (ret)
>>+ return ret;
>>+
>>+ /* Restore config space for the affected devices */
>>+ eeh_pe_restore_bars(pe);
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_pe_configure);
>>+
>> static int proc_eeh_show(struct seq_file *m, void *v)
>> {
>> if (!eeh_enabled()) {
>>diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>index 7ba0424..301ac18 100644
>>--- a/drivers/vfio/pci/vfio_pci.c
>>+++ b/drivers/vfio/pci/vfio_pci.c
>>@@ -25,6 +25,9 @@
>> #include <linux/types.h>
>> #include <linux/uaccess.h>
>> #include <linux/vfio.h>
>>+#ifdef CONFIG_EEH
>>+#include <asm/eeh.h>
>>+#endif
>> #include "vfio_pci_private.h"
>>@@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>> pci_restore_state(pdev);
>> }
>>+static void vfio_eeh_pci_release(struct pci_dev *pdev)
>>+{
>>+#ifdef CONFIG_EEH
>>+ eeh_dev_release(pdev);
>>+#endif
>>+}
>>+
>> static void vfio_pci_release(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>>- if (atomic_dec_and_test(&vdev->refcnt))
>>+ if (atomic_dec_and_test(&vdev->refcnt)) {
>>+ vfio_eeh_pci_release(vdev->pdev);
>> vfio_pci_disable(vdev);
>>+ }
>> module_put(THIS_MODULE);
>> }
>>+static int vfio_eeh_pci_open(struct pci_dev *pdev)
>>+{
>>+ int ret = 0;
>>+
>>+#ifdef CONFIG_EEH
>>+ ret = eeh_dev_open(pdev);
>>+#endif
>>+ return ret;
>>+}
>>+
>> static int vfio_pci_open(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>>+ int ret;
>> if (!try_module_get(THIS_MODULE))
>> return -ENODEV;
>> if (atomic_inc_return(&vdev->refcnt) == 1) {
>>- int ret = vfio_pci_enable(vdev);
>>- if (ret) {
>>- module_put(THIS_MODULE);
>>- return ret;
>>- }
>>+ ret = vfio_pci_enable(vdev);
>>+ if (ret)
>>+ goto error;
>>+
>>+ ret = vfio_eeh_pci_open(vdev->pdev);
>>+ if (ret)
>>+ goto error;
>> }
>> return 0;
>>+error:
>>+ module_put(THIS_MODULE);
>>+ return ret;
>> }
>> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
>>@@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
>> return walk.ret;
>> }
>>+static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
>>+ unsigned int cmd,
>>+ unsigned long arg)
>>+{
>>+ unsigned long minsz;
>>+ int ret = 0;
>>+
>>+#ifdef CONFIG_EEH
>>+ switch (cmd) {
>>+ case VFIO_EEH_PE_SET_OPTION: {
>>+ struct vfio_eeh_pe_set_option option;
>>+
>>+ minsz = offsetofend(struct vfio_eeh_pe_set_option, option);
>>+ if (copy_from_user(&option, (void __user *)arg, minsz))
>>+ return -EFAULT;
>>+ if (option.argsz < minsz)
>>+ return -EINVAL;
>>+
>>+ ret = eeh_pe_set_option(pdev, option.option);
>>+ break;
>>+ }
>>+ case VFIO_EEH_PE_GET_ADDR: {
>>+ struct vfio_eeh_pe_get_addr addr;
>>+
>>+ minsz = offsetofend(struct vfio_eeh_pe_get_addr, info);
>>+ if (copy_from_user(&addr, (void __user *)arg, minsz))
>>+ return -EFAULT;
>>+ if (addr.argsz < minsz)
>>+ return -EINVAL;
>>+
>>+ ret = eeh_pe_get_addr(pdev, addr.option);
>>+ if (ret >= 0) {
>>+ addr.info = ret;
>>+ if (copy_to_user((void __user *)arg, &addr, minsz))
>>+ return -EFAULT;
>>+ ret = 0;
>>+ }
>>+
>>+ break;
>>+ }
>>+ case VFIO_EEH_PE_GET_STATE: {
>>+ struct vfio_eeh_pe_get_state state;
>>+
>>+ minsz = offsetofend(struct vfio_eeh_pe_get_state, state);
>>+ if (copy_from_user(&state, (void __user *)arg, minsz))
>>+ return -EFAULT;
>>+ if (state.argsz < minsz)
>>+ return -EINVAL;
>>+
>>+ ret = eeh_pe_get_state(pdev);
>>+ if (ret >= 0) {
>>+ state.state = ret;
>>+ if (copy_to_user((void __user *)arg, &state, minsz))
>>+ return -EFAULT;
>>+ ret = 0;
>>+ }
>>+ break;
>>+ }
>>+ case VFIO_EEH_PE_RESET: {
>>+ struct vfio_eeh_pe_reset reset;
>>+
>>+ minsz = offsetofend(struct vfio_eeh_pe_reset, option);
>>+ if (copy_from_user(&reset, (void __user *)arg, minsz))
>>+ return -EFAULT;
>>+ if (reset.argsz < minsz)
>>+ return -EINVAL;
>>+
>>+ ret = eeh_pe_reset(pdev, reset.option);
>>+ break;
>>+ }
>>+ case VFIO_EEH_PE_CONFIGURE:
>>+ ret = eeh_pe_configure(pdev);
>>+ break;
>>+ default:
>>+ ret = -EINVAL;
>>+ pr_debug("%s: Cannot handle command %d\n",
>>+ __func__, cmd);
>>+ }
>>+#else
>>+ ret = -ENOENT;
>>+#endif
>>+
>>+ return ret;
>>+}
>>+
>> static long vfio_pci_ioctl(void *device_data,
>> unsigned int cmd, unsigned long arg)
>> {
>>@@ -682,6 +795,12 @@ hot_reset_release:
>> kfree(groups);
>> return ret;
>>+ } else if (cmd == VFIO_EEH_PE_SET_OPTION ||
>>+ cmd == VFIO_EEH_PE_GET_ADDR ||
>>+ cmd == VFIO_EEH_PE_GET_STATE ||
>>+ cmd == VFIO_EEH_PE_RESET ||
>>+ cmd == VFIO_EEH_PE_CONFIGURE) {
>>+ return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
>> }
>> return -ENOTTY;
>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>index cb9023d..ef55682 100644
>>--- a/include/uapi/linux/vfio.h
>>+++ b/include/uapi/linux/vfio.h
>>@@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>+/*
>>+ * EEH functionality can be enabled or disabled on one specific device.
>>+ * Also, the DMA or IO frozen state can be removed from the frozen PE
>>+ * if required.
>>+ */
>>+struct vfio_eeh_pe_set_option {
>>+ __u32 argsz;
>
>What is this argsz thing? Is this your way of maintaining backwards
>compatibility when we introduce new fields? A new field will change
>the ioctl number, so I don't think that makes a lot of sense :).
>
>Just make the ioctl have a u32 as incoming argument. No fancy
>structs, no complicated code.
>
>The same applies for a number of structs below.
>
ok. Will do in next revision.
>>+ __u32 option;
>>+};
>>+
>>+#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>>+
>>+/*
>>+ * Each EEH PE should have unique address to be identified. The command
>>+ * helps to retrieve the address and the sharing mode of the PE.
>>+ */
>>+struct vfio_eeh_pe_get_addr {
>>+ __u32 argsz;
>>+ __u32 option;
>>+ __u32 info;
>
>Any particular reason you need the info field? Can't the return value
>of the ioctl hold this? Then you only have a single u32 argument left
>to the ioctl again.
>
ok. Will do in next revision.
>>+};
>>+
>>+#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
>>+
>>+/*
>>+ * EEH PE might have been frozen because of PCI errors. Also, it might
>>+ * be experiencing reset for error revoery. The following command helps
>>+ * to get the state.
>>+ */
>>+struct vfio_eeh_pe_get_state {
>>+ __u32 argsz;
>>+ __u32 state;
>>+};
>>+
>>+#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
>>+
>>+/*
>>+ * Reset is the major step to recover problematic PE. The following
>>+ * command helps on that.
>>+ */
>>+struct vfio_eeh_pe_reset {
>>+ __u32 argsz;
>>+ __u32 option;
>>+};
>>+
>>+#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
>>+
>>+/*
>>+ * One of the steps for recovery after PE reset is to configure the
>>+ * PCI bridges affected by the PE reset.
>>+ */
>>+#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
>>+
>> /* ***************************************************************** */
>> #endif /* _UAPIVFIO_H */
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v6 3/3] powerpc/eeh: Avoid event on passed PE
From: Gavin Shan @ 2014-05-23 0:01 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537DC9AD.3090202@suse.de>
On Thu, May 22, 2014 at 11:55:57AM +0200, Alexander Graf wrote:
>
>On 22.05.14 10:23, Gavin Shan wrote:
>>If we detects frozen state on PE that has been passed through to somebody
>>else. we needn't handle it. Instead, we rely on the device's owner to
>>detect and recover it. The patch avoid EEH event on the frozen passed PE so
>>that the device's owner can have chance to handle that.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>I think you want to fold this with patch 1/3.
>
Yes, I'll do it in next revision.
Thanks,
Gavin
>>---
>> arch/powerpc/kernel/eeh.c | 8 ++++++++
>> arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>index b90a474..aee6cc5 100644
>>--- a/arch/powerpc/kernel/eeh.c
>>+++ b/arch/powerpc/kernel/eeh.c
>>@@ -403,6 +403,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
>> if (ret > 0)
>> return ret;
>>+ /*
>>+ * If the PE isn't owned by us, we shouldn't check the
>>+ * state. Instead, let the owner handle it if the PE has
>>+ * been frozen.
>>+ */
>>+ if (eeh_pe_passed(pe))
>>+ return 0;
>>+
>> /* If we already have a pending isolation event for this
>> * slot, we know it's bad already, we don't need to check.
>> * Do this checking under a lock; as multiple PCI devices
>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>index 1b5982f..03a3ed2 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
>> opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
>> OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
>> ret = EEH_NEXT_ERR_NONE;
>>- } else if ((*pe)->state & EEH_PE_ISOLATED) {
>>+ } else if ((*pe)->state & EEH_PE_ISOLATED ||
>>+ eeh_pe_passed(*pe)) {
>> ret = EEH_NEXT_ERR_NONE;
>> } else {
>> pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Cody P Schafer @ 2014-05-22 23:54 UTC (permalink / raw)
To: Stephen Rothwell
Cc: peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <20140523094929.15f4613c@canb.auug.org.au>
On 05/22/2014 04:49 PM, Stephen Rothwell wrote:
> Hi Cody,
>
> On Thu, 22 May 2014 15:44:25 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>>
>> if (ret) {
>> if (success_expected)
>> pr_err_ratelimited("hcall failed: %d %#x %#x %d => 0x%lx (%ld) detail=0x%x failing ix=%x\n",
>> domain, offset, ix, lpar,
>> ret, ret,
>> - result_buffer.buf.detailed_rc,
>> - result_buffer.buf.failing_request_ix);
>> - return ret;
>> + result_buffer->buf.detailed_rc,
>> + result_buffer->buf.failing_request_ix);
>> + goto out_hcall;
>> }
>>
>> - *res = be64_to_cpu(result_buffer.result);
>> + *res = be64_to_cpu(result_buffer->result);
>
> not a biggie, but this last bit could be (remove the goto out_hcall and
> teh label and then)
>
> } else {
> *res = be64_to_cpu(result_buffer->result);
> }
>
I've got a slight preference toward keeping it as is, which lets all of
the non-error path code stay outside of if/else blocks (and the error
handling is kept ever so slightly more consistent).
>> +out_hcall:
>> + kfree(result_buffer);
>> +out_resb:
>> + kfree(request_buffer);
>> +out_reqb:
>> return ret;
>> }
>>
>
> otherwise looks good to me.
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Stephen Rothwell @ 2014-05-22 23:49 UTC (permalink / raw)
To: Cody P Schafer
Cc: peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <1400798673-27522-1-git-send-email-cody@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
Hi Cody,
On Thu, 22 May 2014 15:44:25 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
> if (ret) {
> if (success_expected)
> pr_err_ratelimited("hcall failed: %d %#x %#x %d => 0x%lx (%ld) detail=0x%x failing ix=%x\n",
> domain, offset, ix, lpar,
> ret, ret,
> - result_buffer.buf.detailed_rc,
> - result_buffer.buf.failing_request_ix);
> - return ret;
> + result_buffer->buf.detailed_rc,
> + result_buffer->buf.failing_request_ix);
> + goto out_hcall;
> }
>
> - *res = be64_to_cpu(result_buffer.result);
> + *res = be64_to_cpu(result_buffer->result);
not a biggie, but this last bit could be (remove the goto out_hcall and
teh label and then)
} else {
*res = be64_to_cpu(result_buffer->result);
}
> +out_hcall:
> + kfree(result_buffer);
> +out_resb:
> + kfree(request_buffer);
> +out_reqb:
> return ret;
> }
>
otherwise looks good to me.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 1/1][v3] powerpc/perf: Adjust callchain based on DWARF debug info
From: Sukadev Bhattiprolu @ 2014-05-22 23:26 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: linuxppc-dev, Anton Blanchard, linux-kernel, michael,
Ulrich.Weigand, Maynard Johnson
powerpc/perf: Adjust callchain based on DWARF debug info
When saving the callchain on Power, the kernel conservatively saves excess
entries in the callchain. A few of these entries are needed in some cases
but not others. We should use the DWARF debug information to determine
when the entries are needed.
Eg: the value in the link register (LR) is needed only when it holds the
return address of a function. At other times it must be ignored.
If the unnecessary entries are not ignored, we end up with duplicate arcs
in the call-graphs.
Use the DWARF debug information to determine if any callchain entries
should be ignored when building call-graphs.
Callgraph before the patch:
14.67% 2234 sprintft libc-2.18.so [.] __random
|
--- __random
|
|--61.12%-- __random
| |
| |--97.15%-- rand
| | do_my_sprintf
| | main
| | generic_start_main.isra.0
| | __libc_start_main
| | 0x0
| |
| --2.85%-- do_my_sprintf
| main
| generic_start_main.isra.0
| __libc_start_main
| 0x0
|
--38.88%-- rand
|
|--94.01%-- rand
| do_my_sprintf
| main
| generic_start_main.isra.0
| __libc_start_main
| 0x0
|
--5.99%-- do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
0x0
Callgraph after the patch:
14.67% 2234 sprintft libc-2.18.so [.] __random
|
--- __random
|
|--95.93%-- rand
| do_my_sprintf
| main
| generic_start_main.isra.0
| __libc_start_main
| 0x0
|
--4.07%-- do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
0x0
TODO: For split-debug info objects like glibc, we can only determine
the call-frame-address only when both .eh_frame and .debug_info
sections are available. We should be able to determin the CFA
even without the .eh_frame section.
Fix suggested by Anton Blanchard.
Thanks to valuable input on DWARF debug information from Ulrich Weigand.
Reported-by: Maynard Johnson <maynard@us.ibm.com>
Tested-by: Maynard Johnson <maynard@us.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
Changelog[v3]
[Jiri Olsa] Rename function to arch_skip_callchain_idx() to be
consistent with behavior.
[Jiri Olsa] Add '__maybe_unused' tags for unused parameters.
Changelog[v2]:
- Add missing dwfl_end()
- Fix merge conflicts due to some unwind code
tools/perf/arch/powerpc/Makefile | 1 +
tools/perf/arch/powerpc/util/skip-callchain-idx.c | 266 +++++++++++++++++++++
tools/perf/config/Makefile | 4 +
tools/perf/util/callchain.h | 13 +
tools/perf/util/machine.c | 16 +-
5 files changed, 298 insertions(+), 2 deletions(-)
create mode 100644 tools/perf/arch/powerpc/util/skip-callchain-idx.c
diff --git a/tools/perf/arch/powerpc/Makefile b/tools/perf/arch/powerpc/Makefile
index 744e629..b92219b 100644
--- a/tools/perf/arch/powerpc/Makefile
+++ b/tools/perf/arch/powerpc/Makefile
@@ -3,3 +3,4 @@ PERF_HAVE_DWARF_REGS := 1
LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o
endif
LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/header.o
+LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/skip-callchain-idx.o
diff --git a/tools/perf/arch/powerpc/util/skip-callchain-idx.c b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
new file mode 100644
index 0000000..a7c23a4
--- /dev/null
+++ b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
@@ -0,0 +1,266 @@
+/*
+ * Use DWARF Debug information to skip unnecessary callchain entries.
+ *
+ * Copyright (C) 2014 Sukadev Bhattiprolu, IBM Corporation.
+ * Copyright (C) 2014 Ulrich Weigand, IBM Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <inttypes.h>
+#include <dwarf.h>
+#include <elfutils/libdwfl.h>
+
+#include "util/thread.h"
+#include "util/callchain.h"
+
+/*
+ * When saving the callchain on Power, the kernel conservatively saves
+ * excess entries in the callchain. A few of these entries are needed
+ * in some cases but not others. If the unnecessary entries are not
+ * ignored, we end up with duplicate arcs in the call-graphs. Use
+ * DWARF debug information to skip over any unnecessary callchain
+ * entries.
+ *
+ * See function header for arch_adjust_callchain() below for more details.
+ *
+ * The libdwfl code in this file is based on code from elfutils
+ * (libdwfl/argp-std.c, libdwfl/tests/addrcfi.c, etc).
+ */
+static char *debuginfo_path;
+
+static const Dwfl_Callbacks offline_callbacks = {
+ .debuginfo_path = &debuginfo_path,
+ .find_debuginfo = dwfl_standard_find_debuginfo,
+ .section_address = dwfl_offline_section_address,
+};
+
+
+/*
+ * Use the DWARF expression for the Call-frame-address and determine
+ * if return address is in LR and if a new frame was allocated.
+ */
+static int check_return_reg(int ra_regno, Dwarf_Frame *frame)
+{
+ Dwarf_Op ops_mem[2];
+ Dwarf_Op dummy;
+ Dwarf_Op *ops = &dummy;
+ size_t nops;
+ int result;
+
+ result = dwarf_frame_register(frame, ra_regno, ops_mem, &ops, &nops);
+ if (result < 0) {
+ pr_debug("dwarf_frame_register() %s\n", dwarf_errmsg(-1));
+ return -1;
+ }
+
+ /*
+ * Check if return address is on the stack.
+ */
+ if (nops != 0 || ops != NULL)
+ return 0;
+
+ /*
+ * Return address is in LR. Check if a frame was allocated
+ * but not-yet used.
+ */
+ result = dwarf_frame_cfa(frame, &ops, &nops);
+ if (result < 0) {
+ pr_debug("dwarf_frame_cfa() returns %d, %s\n", result,
+ dwarf_errmsg(-1));
+ return -1;
+ }
+
+ /*
+ * If call frame address is in r1, no new frame was allocated.
+ */
+ if (nops == 1 && ops[0].atom == DW_OP_bregx && ops[0].number == 1 &&
+ ops[0].number2 == 0)
+ return 1;
+
+ /*
+ * A new frame was allocated but has not yet been used.
+ */
+ return 2;
+}
+
+/*
+ * Get the DWARF frame from the .eh_frame section.
+ */
+static Dwarf_Frame *get_eh_frame(Dwfl_Module *mod, Dwarf_Addr pc)
+{
+ int result;
+ Dwarf_Addr bias;
+ Dwarf_CFI *cfi;
+ Dwarf_Frame *frame;
+
+ cfi = dwfl_module_eh_cfi(mod, &bias);
+ if (!cfi) {
+ pr_debug("%s(): no CFI - %s\n", __func__, dwfl_errmsg(-1));
+ return NULL;
+ }
+
+ result = dwarf_cfi_addrframe(cfi, pc, &frame);
+ if (result) {
+ pr_debug("%s(): %s\n", __func__, dwfl_errmsg(-1));
+ return NULL;
+ }
+
+ return frame;
+}
+
+/*
+ * Get the DWARF frame from the .debug_frame section.
+ */
+static Dwarf_Frame *get_dwarf_frame(Dwfl_Module *mod, Dwarf_Addr pc)
+{
+ Dwarf_CFI *cfi;
+ Dwarf_Addr bias;
+ Dwarf_Frame *frame;
+ int result;
+
+ cfi = dwfl_module_dwarf_cfi(mod, &bias);
+ if (!cfi) {
+ pr_debug("%s(): no CFI - %s\n", __func__, dwfl_errmsg(-1));
+ return NULL;
+ }
+
+ result = dwarf_cfi_addrframe(cfi, pc, &frame);
+ if (result) {
+ pr_debug("%s(): %s\n", __func__, dwfl_errmsg(-1));
+ return NULL;
+ }
+
+ return frame;
+}
+
+/*
+ * Return:
+ * 0 if return address for the program counter @pc is on stack
+ * 1 if return address is in LR and no new stack frame was allocated
+ * 2 if return address is in LR and a new frame was allocated (but not
+ * yet used)
+ * -1 in case of errors
+ */
+static int check_return_addr(const char *exec_file, Dwarf_Addr pc)
+{
+ int rc = -1;
+ Dwfl *dwfl;
+ Dwfl_Module *mod;
+ Dwarf_Frame *frame;
+ int ra_regno;
+ Dwarf_Addr start = pc;
+ Dwarf_Addr end = pc;
+ bool signalp;
+
+ dwfl = dwfl_begin(&offline_callbacks);
+ if (!dwfl) {
+ pr_debug("dwfl_begin() failed: %s\n", dwarf_errmsg(-1));
+ return -1;
+ }
+
+ if (dwfl_report_offline(dwfl, "", exec_file, -1) == NULL) {
+ pr_debug("dwfl_report_offline() failed %s\n", dwarf_errmsg(-1));
+ goto out;
+ }
+
+ mod = dwfl_addrmodule(dwfl, pc);
+ if (!mod) {
+ pr_debug("dwfl_addrmodule() failed, %s\n", dwarf_errmsg(-1));
+ goto out;
+ }
+
+ /*
+ * To work with split debug info files (eg: glibc), check both
+ * .eh_frame and .debug_frame sections of the ELF header.
+ */
+ frame = get_eh_frame(mod, pc);
+ if (!frame) {
+ frame = get_dwarf_frame(mod, pc);
+ if (!frame)
+ goto out;
+ }
+
+ ra_regno = dwarf_frame_info(frame, &start, &end, &signalp);
+ if (ra_regno < 0) {
+ pr_debug("Return address register unavailable: %s\n",
+ dwarf_errmsg(-1));
+ goto out;
+ }
+
+ rc = check_return_reg(ra_regno, frame);
+
+out:
+ dwfl_end(dwfl);
+ return rc;
+}
+
+/*
+ * The callchain saved by the kernel always includes the link register (LR).
+ *
+ * 0: PERF_CONTEXT_USER
+ * 1: Program counter (Next instruction pointer)
+ * 2: LR value
+ * 3: Caller's caller
+ * 4: ...
+ *
+ * The value in LR is only needed when it holds a return address. If the
+ * return address is on the stack, we should ignore the LR value.
+ *
+ * Further, when the return address is in the LR, if a new frame was just
+ * allocated but the LR was not saved into it, then the LR contains the
+ * caller, slot 4: contains the caller's caller and the contents of slot 3:
+ * (chain->ips[3]) is undefined and must be ignored.
+ *
+ * Use DWARF debug information to determine if any entries need to be skipped.
+ *
+ * Return:
+ * index: of callchain entry that needs to be ignored (if any)
+ * -1 if no entry needs to be ignored or in case of errors
+ */
+int arch_skip_callchain_idx(struct machine *machine, struct thread *thread,
+ struct ip_callchain *chain)
+{
+ struct addr_location al;
+ struct dso *dso = NULL;
+ int rc;
+ u64 ip;
+ u64 skip_slot = -1;
+
+ if (chain->nr < 3)
+ return skip_slot;
+
+ ip = chain->ips[2];
+
+ thread__find_addr_location(thread, machine, PERF_RECORD_MISC_USER,
+ MAP__FUNCTION, ip, &al);
+
+ if (al.map)
+ dso = al.map->dso;
+
+ if (!dso) {
+ pr_debug("%" PRIx64 " dso is NULL\n", ip);
+ return skip_slot;
+ }
+
+ rc = check_return_addr(dso->long_name, ip);
+
+ pr_debug("DSO %s, nr %" PRIx64 ", ip 0x%" PRIx64 "rc %d\n",
+ dso->long_name, chain->nr, ip, rc);
+
+ if (rc == 0) {
+ /*
+ * Return address on stack. Ignore LR value in callchain
+ */
+ skip_slot = 2;
+ } else if (rc == 2) {
+ /*
+ * New frame allocated but return address still in LR.
+ * Ignore the caller's caller entry in callchain.
+ */
+ skip_slot = 3;
+ }
+ return skip_slot;
+}
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 729bbdf..8d1417d 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -48,6 +48,10 @@ ifneq ($(ARCH),$(filter $(ARCH),x86 arm))
NO_LIBDW_DWARF_UNWIND := 1
endif
+ifeq ($(ARCH),powerpc)
+ CFLAGS += -DHAVE_SKIP_CALLCHAIN_IDX
+endif
+
ifeq ($(LIBUNWIND_LIBS),)
NO_LIBUNWIND := 1
else
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index bde2b0c..5176599 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -165,4 +165,17 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
extern const char record_callchain_help[];
int parse_callchain_report_opt(const char *arg);
+
+#ifdef HAVE_SKIP_CALLCHAIN_IDX
+extern int arch_skip_callchain_idx(struct machine *machine,
+ struct thread *thread, struct ip_callchain *chain);
+#else
+static inline int arch_skip_callchain_idx(struct machine *machine __maybe_unused,
+ struct thread *thread __maybe_unused,
+ struct ip_callchain *chain __maybe_unused)
+{
+ return -1;
+}
+#endif
+
#endif /* __PERF_CALLCHAIN_H */
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7409ac8..bc93279 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1290,6 +1290,7 @@ static int machine__resolve_callchain_sample(struct machine *machine,
int chain_nr = min(max_stack, (int)chain->nr);
int i;
int err;
+ int skip_slot;
callchain_cursor_reset(&callchain_cursor);
@@ -1298,14 +1299,25 @@ static int machine__resolve_callchain_sample(struct machine *machine,
return 0;
}
+ /*
+ * Based on DWARF debug information, some architectures skip
+ * some of the callchain entries saved by the kernel.
+ */
+ skip_slot = arch_skip_callchain_idx(machine, thread, chain);
+
for (i = 0; i < chain_nr; i++) {
u64 ip;
struct addr_location al;
- if (callchain_param.order == ORDER_CALLEE)
+ if (callchain_param.order == ORDER_CALLEE) {
+ if (i == skip_slot)
+ continue;
ip = chain->ips[i];
- else
+ } else {
+ if ((int)(chain->nr - i - 1) == skip_slot)
+ continue;
ip = chain->ips[chain->nr - i - 1];
+ }
if (ip >= PERF_CONTEXT_MAX) {
switch (ip) {
--
1.7.9.5
^ permalink raw reply related
* Re: powerpc: remove checks for CONFIG_BOOK3E_MMU_TLB_STATS
From: Scott Wood @ 2014-05-22 22:37 UTC (permalink / raw)
To: Paul Bolle; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <1400608550.4912.48.camel@x220>
On Tue, May 20, 2014 at 07:55:50PM +0200, Paul Bolle wrote:
> Three checks for CONFIG_BOOK3E_MMU_TLB_STATS were added in v2.6.32. But
> the related Kconfig symbol was never added. These checks have always
> evaluated to false. Remove them.
>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
>
> ---
> Untested.
>
> A second order cleanup is now possible (ie, TLB_MISS_PROLOG_STATS and
> friends are, well, defined away). Perhaps it is preferable to include
> that cleanup in this patch.
>
> arch/powerpc/include/asm/exception-64e.h | 38 --------------------------------
> arch/powerpc/mm/tlb_low_64e.S | 10 ---------
> 2 files changed, 48 deletions(-)
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S: Assembler messages:
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:89: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:238: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:269: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:281: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:441: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:510: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:881: Error: unrecognized opcode: `tlb_miss_prolog_stats'
/home/scott/fsl/git/linux/upstream/arch/powerpc/mm/tlb_low_64e.S:918: Error: unrecognized opcode: `tlb_miss_prolog_stats'
-Scott
^ permalink raw reply
* Re: [2/2] powerpc/corenet64_smp_defconfig: enable RTC support
From: Scott Wood @ 2014-05-22 22:51 UTC (permalink / raw)
To: Shengzhou Liu; +Cc: linuxppc-dev
In-Reply-To: <1400666751-11459-2-git-send-email-Shengzhou.Liu@freescale.com>
On Wed, May 21, 2014 at 06:05:51PM +0800, Shengzhou Liu wrote:
> Enable RTC support for DS1307, DS1374, DS3232, which is
> needed on some corenet boards.
>
> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
>
> ---
> arch/powerpc/configs/corenet64_smp_defconfig | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/powerpc/configs/corenet64_smp_defconfig b/arch/powerpc/configs/corenet64_smp_defconfig
> index 63508dd..e9c9f86 100644
> --- a/arch/powerpc/configs/corenet64_smp_defconfig
> +++ b/arch/powerpc/configs/corenet64_smp_defconfig
> @@ -125,6 +125,11 @@ CONFIG_USB_EHCI_FSL=y
> CONFIG_USB_STORAGE=y
> CONFIG_MMC=y
> CONFIG_MMC_SDHCI=y
> +CONFIG_RTC_CLASS=y
> +CONFIG_RTC_DRV_CMOS=y
> +CONFIG_RTC_DRV_DS1307=y
> +CONFIG_RTC_DRV_DS1374=y
> +CONFIG_RTC_DRV_DS3232=y
> CONFIG_EDAC=y
> CONFIG_EDAC_MM_EDAC=y
> CONFIG_DMADEVICES=y
Why only corenet64 and not corenet32?
-Scott
^ permalink raw reply
* [PATCH v2] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Cody P Schafer @ 2014-05-22 22:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Cody P Schafer, Michael Ellerman
Cc: sfr, peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <537E78AD.1040409@linux.vnet.ibm.com>
Ian pointed out the use of __aligned(4096) caused rather large stack
consumption in single_24x7_request(), so use the kmem_cache
hv_page_cache (which we've already got set up for other allocations)
insead of allocating locally.
Reported-by: Ian Munsie <imunsie@au1.ibm.com>
Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
In v2:
- remove duplicate exit path
arch/powerpc/perf/hv-24x7.c | 48 +++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index e0766b8..998863b 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -294,7 +294,7 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
u16 lpar, u64 *res,
bool success_expected)
{
- unsigned long ret;
+ unsigned long ret = -ENOMEM;
/*
* request_buffer and result_buffer are not required to be 4k aligned,
@@ -304,7 +304,27 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
struct reqb {
struct hv_24x7_request_buffer buf;
struct hv_24x7_request req;
- } __packed __aligned(4096) request_buffer = {
+ } __packed *request_buffer;
+ struct resb {
+ struct hv_24x7_data_result_buffer buf;
+ struct hv_24x7_result res;
+ struct hv_24x7_result_element elem;
+ __be64 result;
+ } __packed *result_buffer;
+
+ BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
+ BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
+
+ request_buffer = kmem_cache_alloc(hv_page_cache, GFP_USER);
+
+ if (!request_buffer)
+ goto out_reqb;
+
+ result_buffer = kmem_cache_zalloc(hv_page_cache, GFP_USER);
+ if (!result_buffer)
+ goto out_resb;
+
+ *request_buffer = (struct reqb) {
.buf = {
.interface_version = HV_24X7_IF_VERSION_CURRENT,
.num_requests = 1,
@@ -320,28 +340,26 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
}
};
- struct resb {
- struct hv_24x7_data_result_buffer buf;
- struct hv_24x7_result res;
- struct hv_24x7_result_element elem;
- __be64 result;
- } __packed __aligned(4096) result_buffer = {};
-
ret = plpar_hcall_norets(H_GET_24X7_DATA,
- virt_to_phys(&request_buffer), sizeof(request_buffer),
- virt_to_phys(&result_buffer), sizeof(result_buffer));
+ virt_to_phys(request_buffer), sizeof(*request_buffer),
+ virt_to_phys(result_buffer), sizeof(*result_buffer));
if (ret) {
if (success_expected)
pr_err_ratelimited("hcall failed: %d %#x %#x %d => 0x%lx (%ld) detail=0x%x failing ix=%x\n",
domain, offset, ix, lpar,
ret, ret,
- result_buffer.buf.detailed_rc,
- result_buffer.buf.failing_request_ix);
- return ret;
+ result_buffer->buf.detailed_rc,
+ result_buffer->buf.failing_request_ix);
+ goto out_hcall;
}
- *res = be64_to_cpu(result_buffer.result);
+ *res = be64_to_cpu(result_buffer->result);
+out_hcall:
+ kfree(result_buffer);
+out_resb:
+ kfree(request_buffer);
+out_reqb:
return ret;
}
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox