Re: [RFC PATCH 07/11] kvm: powerpc: book3s: pr: move PR related tracepoints to a separate header

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Alexander Graf <agraf@suse.de> writes:

> On 27.09.2013, at 15:06, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
>>> 
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>> 
>>>> This patch moves PR related tracepoints to a separate header. This
>>>> enables in converting PR to a kernel module which will be done in
>>>> later patches
>>>> 
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>> ---
>>>> arch/powerpc/kvm/book3s_64_mmu_host.c |   2 +-
>>>> arch/powerpc/kvm/book3s_mmu_hpte.c    |   2 +-
>>>> arch/powerpc/kvm/book3s_pr.c          |   3 +-
>>>> arch/powerpc/kvm/trace.h              | 234 +--------------------------
>>>> arch/powerpc/kvm/trace_pr.h           | 297 ++++++++++++++++++++++++++++++++++
>>>> 5 files changed, 308 insertions(+), 230 deletions(-)
>>>> create mode 100644 arch/powerpc/kvm/trace_pr.h
>>>> 
>>>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_host.c b/arch/powerpc/kvm/book3s_64_mmu_host.c
>>>> index 329a978..fd5b393 100644
>>>> --- a/arch/powerpc/kvm/book3s_64_mmu_host.c
>>>> +++ b/arch/powerpc/kvm/book3s_64_mmu_host.c
>>>> @@ -27,7 +27,7 @@
>>>> #include <asm/machdep.h>
>>>> #include <asm/mmu_context.h>
>>>> #include <asm/hw_irq.h>
>>>> -#include "trace.h"
>>>> +#include "trace_pr.h"
>>>> 
>>>> #define PTE_SIZE 12
>>>> 
>>>> diff --git a/arch/powerpc/kvm/book3s_mmu_hpte.c b/arch/powerpc/kvm/book3s_mmu_hpte.c
>>>> index d2d280b..4556168 100644
>>>> --- a/arch/powerpc/kvm/book3s_mmu_hpte.c
>>>> +++ b/arch/powerpc/kvm/book3s_mmu_hpte.c
>>>> @@ -28,7 +28,7 @@
>>>> #include <asm/mmu_context.h>
>>>> #include <asm/hw_irq.h>
>>>> 
>>>> -#include "trace.h"
>>>> +#include "trace_pr.h"
>>>> 
>>>> #define PTE_SIZE	12
>>>> 
>>>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>>>> index 2a97279..99d0839 100644
>>>> --- a/arch/powerpc/kvm/book3s_pr.c
>>>> +++ b/arch/powerpc/kvm/book3s_pr.c
>>>> @@ -41,7 +41,8 @@
>>>> #include <linux/vmalloc.h>
>>>> #include <linux/highmem.h>
>>>> 
>>>> -#include "trace.h"
>>>> +#define CREATE_TRACE_POINTS
>>>> +#include "trace_pr.h"
>>>> 
>>>> /* #define EXIT_DEBUG */
>>>> /* #define DEBUG_EXT */
>>>> diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h
>>>> index a088e9a..7d5a136 100644
>>>> --- a/arch/powerpc/kvm/trace.h
>>>> +++ b/arch/powerpc/kvm/trace.h
>>>> @@ -85,6 +85,12 @@ TRACE_EVENT(kvm_ppc_instr,
>>>> 	{41, "HV_PRIV"}
>>>> #endif
>>>> 
>>>> +#ifndef CONFIG_KVM_BOOK3S_PR
>>>> +/*
>>>> + * For pr we define this in trace_pr.h since it pr can be built as
>>>> + * a module
>>> 
>>> Not sure I understand the need. If the config option is available, so
>>> should the struct field. Worst case that happens with HV is that we
>>> get empty shadow_srr1 values in our trace, no?
>> 
>> That is not the real reason. trace.h get built as part of kvm.ko or as
>> part of kernel. These trace functions actually get called from
>> kvm-pr.ko. To make they build i would either need EXPORT_SYMBOL or move
>> the definition of them to kvm-pr.ko. I did the later and moved only pr
>> related traces to kvm-pr.ko
>
> I fail to see why we wouldn't have a trace_hv.h file then, as that can
> also be built as a module, no? And at that point I don't see why we
> would need any conditionals at all in trace.h anymore, as it would
> only cover generic code.

Currently HV module is not using any tracepoints. Once it start using
tracepoints we would have trace_hv.h

-aneesh