* Re: [PATCH 1/1] powerpc/perf: Adjust callchain based on DWARF debug
From: Sukadev Bhattiprolu @ 2014-05-22 17:09 UTC (permalink / raw)
To: Jiri Olsa
Cc: linuxppc-dev, Anton Blanchard, linux-kernel,
Arnaldo Carvalho de Melo, michael, Ulrich.Weigand,
Maynard Johnson
In-Reply-To: <20140522100045.GA4789@krava.brq.redhat.com>
Jiri Olsa [jolsa@redhat.com] wrote:
|
| yep, that sounds more clear to me.. something like below?
|
| calling callchain_dup from within arch_adjust_callchain in case
| you want to change it and returning != 0 in this case, so
| we could free the new callchain
Agree.
|
| but it might be to much overhead in case you have the support to skip
| just one entry now right? is there a plan for more cleaning? ;)
Its only one entry AFACIT. No plans for other changes, so will rename
the function as you mention in the other message and will resend.
Thanks,
Sukadev
^ permalink raw reply
* Re: [PATCH RESEND net-next 4/9] net: systemport: use the new fixed PHY helpers
From: Sergei Shtylyov @ 2014-05-22 17:21 UTC (permalink / raw)
To: Florian Fainelli
Cc: Thomas Petazzoni, open list:OPEN FIRMWARE AND..., Aida Mynzhasova,
netdev, Richard Cochran, open list, Claudiu Manoil, Vitaly Bordug,
Grant Likely, open list:LINUX FOR POWERPC..., David Miller
In-Reply-To: <CAGVrzcbBQFhw-dUh+tFm8H5m9-fmVgutChS8wUG-F6sTBoRq-g@mail.gmail.com>
Hello.
On 05/22/2014 02:02 AM, Florian Fainelli wrote:
>>> of_phy_connect_fixed_link() is becoming obsolete, and also required
>>> platform code to register the fixed PHYs at the specified addresses for
>>> those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
>>> plus of_phy_register_fixed_link() helpers to transition over the new
>>> scheme.
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> drivers/net/ethernet/broadcom/bcmsysport.c | 17 +++++++++++++++--
>>> drivers/net/ethernet/broadcom/bcmsysport.h | 1 +
>>> 2 files changed, 16 insertions(+), 2 deletions(-)
>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
>>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> index d40c5b969e9e..dc708a888f80 100644
>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> @@ -1327,8 +1327,8 @@ static int bcm_sysport_open(struct net_device *dev)
>>> /* Read CRC forward */
>>> priv->crc_fwd = !!(umac_readl(priv, UMAC_CMD) & CMD_CRC_FWD);
>>>
>>> - priv->phydev = of_phy_connect_fixed_link(dev,
>>> bcm_sysport_adj_link,
>>> -
>>> priv->phy_interface);
>>> + priv->phydev = of_phy_connect(dev, priv->phy_dn,
>>> bcm_sysport_adj_link,
>>> + 0, priv->phy_interface);
>> The continuation line should start on the next character after ( on the
>> above line, according to the networking coding style.
> Unless I am once again not following the coding style, the patch in
> patchwork has this correctly, and so does my file locally:
> http://patchwork.ozlabs.org/patch/351323/
Don't know about your local file but the patchwork still has the
indentation wrong. -- you've used only tabs while the last several characters
should have been spaces. However, if David himself finds it correct, I guess I
just need to shut up. :-)
WBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next v2 0/9] net: of_phy_connect_fixed_link removal
From: David Miller @ 2014-05-22 19:18 UTC (permalink / raw)
To: f.fainelli
Cc: thomas.petazzoni, devicetree, aida.mynzhasova, sergei.shtylyov,
netdev, richardcochran, linux-kernel, claudiu.manoil, vbordug,
grant.likely, linuxppc-dev
In-Reply-To: <1400777271-32199-1-git-send-email-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 22 May 2014 09:47:42 -0700
> This patch set removes of_phy_connect_fixed_link() from the tree now that
> we have a better solution for dealing with fixed PHY (emulated PHY) devices
> for drivers that require them.
>
> First two patches update the 'fixed-link' Device Tree binding and drivers to
> refere to it.
>
> Patches 3 to 7 update the in-tree network drivers that use
> of_phy_connect_fixed_link()
>
> Patch 8 removes of_phy_connect_fixed_link
>
> Patch 9 removes the PowerPC code that parsed the 'fixed-link' property.
>
> Patch 9 can be merged via the net-next tree if the PowerPC folks ack it,
> but it really has to be merged after the first 8 patches in order to avoid
> breakage.
Series applied, if someone doesn't like patch #9 we can fix it up with a
followup patch or revert.
^ permalink raw reply
* Re: NUMA topology question wrt. d4edc5b6
From: Srivatsa S. Bhat @ 2014-05-22 20:48 UTC (permalink / raw)
To: Nishanth Aravamudan
Cc: Dave Hansen, Srikar Dronamraju,
linuxppc-dev@lists.ozlabs.org list, Linux MM, Aneesh Kumar K.V,
nfont, Cody P Schafer, Anton Blanchard
In-Reply-To: <20140521200451.GB5755@linux.vnet.ibm.com>
[ Adding a few more CC's ]
On 05/22/2014 01:34 AM, Nishanth Aravamudan wrote:
> Hi Srivatsa,
>
> After d4edc5b6 ("powerpc: Fix the setup of CPU-to-Node mappings during
> CPU online"), cpu_to_node() looks like:
>
> static inline int cpu_to_node(int cpu)
> {
> int nid;
>
> nid = numa_cpu_lookup_table[cpu];
>
> /*
> * During early boot, the numa-cpu lookup table might not have been
> * setup for all CPUs yet. In such cases, default to node 0.
> */
> return (nid < 0) ? 0 : nid;
> }
>
> However, I'm curious if this is correct in all cases. I have seen
> several LPARs that do not have any CPUs on node 0. In fact, because node
> 0 is statically set online in the initialization of the N_ONLINE
> nodemask, 0 is always present to Linux, whether it is present on the
> system. I'm not sure what the best thing to do here is, but I'm curious
> if you have any ideas? I would like to remove the static initialization
> of node 0, as it's confusing to users to see an empty node (particularly
> when it's completely separate in the numbering from other nodes), but
> we trip a panic (refer to:
> http://www.spinics.net/lists/linux-mm/msg73321.html).
>
Ah, I see. I didn't have any particular reason to default it to zero.
I just did that because the existing code before this patch did the same
thing. (numa_cpu_lookup_table[] is a global array, so it will be initialized
with zeros. So if we access it before populating it via numa_setup_cpu(),
it would return 0. So I retained that behaviour with the above conditional).
Will something like the below [totally untested] patch solve the boot-panic?
I understand that as of today first_online_node will still pick 0 since
N_ONLINE is initialized statically, but with your proposed change to that
init code, I guess the following patch should avoid the boot panic.
[ But note that first_online_node is hard-coded to 0, if MAX_NUMNODES is = 1.
So we'll have to fix that if powerpc can have a single node system whose node
is numbered something other than 0. Can that happen as well? ]
And regarding your question about what is the best way to fix this whole Linux
MM's assumption about node0, I'm not really sure.. since I am not really aware
of the extent to which the MM subsystem is intertwined with this assumption
and what it would take to cure that :-(
Regards,
Srivatsa S. Bhat
diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index c920215..58e6469 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -18,6 +18,7 @@ struct device_node;
*/
#define RECLAIM_DISTANCE 10
+#include <linux/nodemask.h>
#include <asm/mmzone.h>
static inline int cpu_to_node(int cpu)
@@ -30,7 +31,7 @@ static inline int cpu_to_node(int cpu)
* During early boot, the numa-cpu lookup table might not have been
* setup for all CPUs yet. In such cases, default to node 0.
*/
- return (nid < 0) ? 0 : nid;
+ return (nid < 0) ? first_online_node : nid;
}
#define parent_node(node) (node)
^ permalink raw reply related
* [PATCH 1/2] powerpc/e6500: hw tablewalk: clear TID in kernel indirect entries
From: Scott Wood @ 2014-05-22 21:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood
Previously TID was being cleared before the tlbsx, but not after. This
can lead to a multiway hit between a TLB entry with TID=0 (previously
inserted when PID=0) and a TLB entry with TID!=0 that matches PID.
This can theoretically result in undefined behavior, though we probably
get lucky due to the details of the overlap. It also results in the
inability to use multihit detection to detect other conflicting TLB
entries, as well as poorer TLB utilization due to duplicating kernel
TLB entries.
Rather than try to patch up MAS1 after tlbsx, the entire value is
saved/restored as with MAS2.
I observed a slight improvement in TLB miss performance with this patch
applied.
Signed-off-by: Scott Wood <scottwood@freescale.com>
Reported-by: Ed Swarthout <ed.swarthout@freescale.com>
---
arch/powerpc/mm/tlb_low_64e.S | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 356e8b4..3298d10 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -322,19 +322,17 @@ tlb_miss_common_e6500:
b 1b
.previous
- mfspr r15,SPRN_MAS2
+ mfspr r15,SPRN_MAS1
+ mfspr r10,SPRN_MAS2
tlbsx 0,r16
+ mtspr SPRN_MAS2,r10
mfspr r10,SPRN_MAS1
+ mtspr SPRN_MAS1,r15
+
andis. r10,r10,MAS1_VALID@h
bne tlb_miss_done_e6500
- /* Undo MAS-damage from the tlbsx */
- mfspr r10,SPRN_MAS1
- oris r10,r10,MAS1_VALID@h
- mtspr SPRN_MAS1,r10
- mtspr SPRN_MAS2,r15
-
/* Now, we need to walk the page tables. First check if we are in
* range.
*/
--
1.9.1
^ permalink raw reply related
* [PATCH 2/2] powerpc/e6500: hw tablewalk: fix recursive tlb lock on cpu 0
From: Scott Wood @ 2014-05-22 21:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1400795101-8737-1-git-send-email-scottwood@freescale.com>
Commit 82d86de25b9c99db546e17c6f7ebf9a691da557e "TLB lock recursive"
introduced a bug whereby cpu 0 uses the same value for "lock held" as
is used to indicate that the lock is free. This means that cpu 1 can
acquire the lock whenever it wants, regardless of whether cpu 0 has it
locked, which in turn means we can get duplicate TLB entries.
Add one to the CPU value to ensure we do not use zero as a "lock held"
value.
Signed-off-by: Scott Wood <scottwood@freescale.com>
---
arch/powerpc/mm/tlb_low_64e.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 3298d10..ba3ba3c 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -309,6 +309,7 @@ tlb_miss_common_e6500:
lhz r10,PACAPACAINDEX(r13)
cmpdi r15,0
cmpdi cr1,r15,1 /* set cr1.eq = 0 for non-recursive */
+ addi r10,r10,1
bne 2f
stbcx. r10,0,r11
bne 1b
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] powerpc: Don't corrupt user registers on 32-bit
From: shiva7 @ 2014-05-22 21:51 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131023084002.GA8325@iris.ozlabs.ibm.com>
Paul Mackerras wrote
> Commit de79f7b9f6 ("powerpc: Put FP/VSX and VR state into structures")
> modified load_up_fpu() and load_up_altivec() in such a way that they
> now use r7 and r8. Unfortunately, the callers of these functions on
> 32-bit machines then return to userspace via fast_exception_return,
> which doesn't restore all of the volatile GPRs, but only r1, r3 -- r6
> and r9 -- r12. This was causing userspace segfaults and other
> userspace misbehaviour on 32-bit machines.
>
> This fixes the problem by changing the register usage of load_up_fpu()
> and load_up_altivec() to avoid using r7 and r8 and instead use r6 and
> r10. This also adds comments to those functions saying which registers
> may be used.
>
> Signed-off-by: Paul Mackerras <
> paulus@
> >
> ---
> arch/powerpc/kernel/fpu.S | 14 ++++++++------
> arch/powerpc/kernel/vector.S | 15 +++++++++------
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/fpu.S b/arch/powerpc/kernel/fpu.S
> index 4dca05e..f7f5b8b 100644
> --- a/arch/powerpc/kernel/fpu.S
> +++ b/arch/powerpc/kernel/fpu.S
> @@ -106,6 +106,8 @@ _GLOBAL(store_fp_state)
> * and save its floating-point registers in its thread_struct.
> * Load up this task's FP registers from its thread_struct,
> * enable the FPU for the current task and return to the task.
> + * Note that on 32-bit this can only use registers that will be
> + * restored by fast_exception_return, i.e. r3 - r6, r10 and r11.
> */
> _GLOBAL(load_up_fpu)
> mfmsr r5
> @@ -131,10 +133,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> beq 1f
> toreal(r4)
> addi r4,r4,THREAD /* want last_task_used_math->thread */
> - addi r8,r4,THREAD_FPSTATE
> - SAVE_32FPVSRS(0, R5, R8)
> + addi r10,r4,THREAD_FPSTATE
> + SAVE_32FPVSRS(0, R5, R10)
> mffs fr0
> - stfd fr0,FPSTATE_FPSCR(r8)
> + stfd fr0,FPSTATE_FPSCR(r10)
> PPC_LL r5,PT_REGS(r4)
> toreal(r5)
> PPC_LL r4,_MSR-STACK_FRAME_OVERHEAD(r5)
> @@ -157,10 +159,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> or r12,r12,r4
> std r12,_MSR(r1)
> #endif
> - addi r7,r5,THREAD_FPSTATE
> - lfd fr0,FPSTATE_FPSCR(r7)
> + addi r10,r5,THREAD_FPSTATE
> + lfd fr0,FPSTATE_FPSCR(r10)
> MTFSF_L(fr0)
> - REST_32FPVSRS(0, R4, R7)
> + REST_32FPVSRS(0, R4, R10)
> #ifndef CONFIG_SMP
> subi r4,r5,THREAD
> fromreal(r4)
> diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
> index eacda4e..0458a9a 100644
> --- a/arch/powerpc/kernel/vector.S
> +++ b/arch/powerpc/kernel/vector.S
> @@ -64,6 +64,9 @@ _GLOBAL(store_vr_state)
> * Enables the VMX for use in the kernel on return.
> * On SMP we know the VMX is free, since we give it up every
> * switch (ie, no lazy save of the vector registers).
> + *
> + * Note that on 32-bit this can only use registers that will be
> + * restored by fast_exception_return, i.e. r3 - r6, r10 and r11.
> */
> _GLOBAL(load_up_altivec)
> mfmsr r5 /* grab the current MSR */
> @@ -89,11 +92,11 @@ _GLOBAL(load_up_altivec)
> /* Save VMX state to last_task_used_altivec's THREAD struct */
> toreal(r4)
> addi r4,r4,THREAD
> - addi r7,r4,THREAD_VRSTATE
> - SAVE_32VRS(0,r5,r7)
> + addi r6,r4,THREAD_VRSTATE
> + SAVE_32VRS(0,r5,r6)
> mfvscr vr0
> li r10,VRSTATE_VSCR
> - stvx vr0,r10,r7
> + stvx vr0,r10,r6
> /* Disable VMX for last_task_used_altivec */
> PPC_LL r5,PT_REGS(r4)
> toreal(r5)
> @@ -125,13 +128,13 @@ _GLOBAL(load_up_altivec)
> oris r12,r12,MSR_VEC@h
> std r12,_MSR(r1)
> #endif
> - addi r7,r5,THREAD_VRSTATE
> + addi r6,r5,THREAD_VRSTATE
> li r4,1
> li r10,VRSTATE_VSCR
> stw r4,THREAD_USED_VR(r5)
> - lvx vr0,r10,r7
> + lvx vr0,r10,r6
> mtvscr vr0
> - REST_32VRS(0,r4,r7)
> + REST_32VRS(0,r4,r6)
> #ifndef CONFIG_SMP
> /* Update last_task_used_altivec to 'current' */
> subi r4,r5,THREAD /* Back to 'current' */
> --
> 1.8.4.rc3
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@.ozlabs
> https://lists.ozlabs.org/listinfo/linuxppc-dev
By any chance, same corruption is happening for DEBUG_DEBUG exception ?
because I could see similar SEGV but dont have any code/program to prove it
:(
--
View this message in context: http://linuxppc.10917.n7.nabble.com/PATCH-powerpc-Don-t-corrupt-user-registers-on-32-bit-tp77443p82590.html
Sent from the linuxppc-dev mailing list archive at Nabble.com.
^ permalink raw reply
* [PATCH] arch: powerpc: kernel: fadump.c: Cleaning up inconsistent NULL checks
From: Rickard Strandqvist @ 2014-05-22 22:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel, Rickard Strandqvist
Cleaning up inconsistent NULL checks.
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
arch/powerpc/kernel/fadump.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 2230fd0..a76e7a4 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -645,7 +645,7 @@ static int __init fadump_build_cpu_notes(const struct fadump_mem_struct *fdm)
}
/* Lower 4 bytes of reg_value contains logical cpu id */
cpu = reg_entry->reg_value & FADUMP_CPU_ID_MASK;
- if (!cpumask_test_cpu(cpu, &fdh->cpu_online_mask)) {
+ if (fdh && !cpumask_test_cpu(cpu, &fdh->cpu_online_mask)) {
SKIP_TO_NEXT_CPU(reg_entry);
continue;
}
@@ -662,9 +662,11 @@ static int __init fadump_build_cpu_notes(const struct fadump_mem_struct *fdm)
}
fadump_final_note(note_buf);
- pr_debug("Updating elfcore header (%llx) with cpu notes\n",
+ if (fdh) {
+ pr_debug("Updating elfcore header (%llx) with cpu notes\n",
fdh->elfcorehdr_addr);
- fadump_update_elfcore_header((char *)__va(fdh->elfcorehdr_addr));
+ fadump_update_elfcore_header((char *)__va(fdh->elfcorehdr_addr));
+ }
return 0;
error_out:
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v4 09/11] powerpc/perf: add support for the hv 24x7 interface
From: Cody P Schafer @ 2014-05-22 22:22 UTC (permalink / raw)
To: Ian Munsie
Cc: Stephen Rothwell, Peter Zijlstra, LKML, Michael Ellerman,
Ingo Molnar, Paul Mackerras, Arnaldo Carvalho de Melo, scottwood,
linuxppc-dev
In-Reply-To: <1400744950-sup-4804@delenn.ozlabs.ibm.com>
On 05/22/2014 01:19 AM, Ian Munsie wrote:
> Hi Cody,
>
> I just tried building this with gcc 4.5, which failed with the following
> warning (treated as an error):
>
> cc1: warnings being treated as errors
> arch/powerpc/perf/hv-24x7.c: In function 'single_24x7_request':
> arch/powerpc/perf/hv-24x7.c:346:1: error: the frame size of 8192 bytes is larger than 2048 bytes
> make[3]: *** [arch/powerpc/perf/hv-24x7.o] Error 1
> make[2]: *** [arch/powerpc/perf] Error 2
>
> My .config has CONFIG_FRAME_WARN=2048 (default on 64bit), but the
> alignment constraints in this function may require 8K on the stack -
> possibly a bit large?
>
Yep, it is a bit large. In other places in hv-24x7 that use similar
firmware interfaces (with similar alignment requirements), I've used a
kmem_cache (hv_page_cache). Testing out a patch that uses that here as well.
>
> Notably for some reason this warning no longer seems to trigger on gcc
> 4.8 (or at least somewhere between 4.5-4.8), though the assembly does
> still show it aligning the buffers.
That's a bit concerning (and might be why I didn't pick it up, using gcc
4.9.0 over here). Looking at the gcc docs, it seems to indicate that
alloca() and VLAs aren't counted for -Wframe-larger-than. Perhaps gcc
decided to move locally defined structures with alignment requirements
into that same bucket? (while size of the structures is statically
determinable, the stack consumption due to alignment is [to some degree]
variable).
^ permalink raw reply
* [PATCH] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Cody P Schafer @ 2014-05-22 22:29 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>
---
arch/powerpc/perf/hv-24x7.c | 52 ++++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index e0766b8..9a7a830 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,30 @@ 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);
+ kfree(result_buffer);
+ kfree(request_buffer);
+ return ret;
+
+out_hcall:
+ kfree(result_buffer);
+out_resb:
+ kfree(request_buffer);
+out_reqb:
return ret;
}
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Stephen Rothwell @ 2014-05-22 22:38 UTC (permalink / raw)
To: Cody P Schafer
Cc: peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <1400797756-6057-1-git-send-email-cody@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 533 bytes --]
Hi Cody,
On Thu, 22 May 2014 15:29:08 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
> - *res = be64_to_cpu(result_buffer.result);
> + *res = be64_to_cpu(result_buffer->result);
> + kfree(result_buffer);
> + kfree(request_buffer);
> + return ret;
Why not just fall through here by removing the above 3 lines?
> +
> +out_hcall:
> + kfree(result_buffer);
> +out_resb:
> + kfree(request_buffer);
> +out_reqb:
> return ret;
> }
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/perf/hv-24x7: use kmem_cache instead of aligned stack allocations
From: Cody P Schafer @ 2014-05-22 22:43 UTC (permalink / raw)
To: Stephen Rothwell
Cc: peterz, linux-kernel, michael, mingo, paulus, imunsie, acme,
scottwood, linuxppc-dev
In-Reply-To: <20140523083847.4e039ddb@canb.auug.org.au>
On 05/22/2014 03:38 PM, Stephen Rothwell wrote:
> Hi Cody,
>
> On Thu, 22 May 2014 15:29:08 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>>
>> - *res = be64_to_cpu(result_buffer.result);
>> + *res = be64_to_cpu(result_buffer->result);
>> + kfree(result_buffer);
>> + kfree(request_buffer);
>> + return ret;
>
> Why not just fall through here by removing the above 3 lines?
No reason except me not noticing it.
>> +
>> +out_hcall:
>> + kfree(result_buffer);
>> +out_resb:
>> + kfree(request_buffer);
>> +out_reqb:
>> return ret;
>> }
>
^ 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
* 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
* 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
* [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: [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
* 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 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 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 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: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 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: 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: 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
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