| Message ID | 1456236764-1569-3-git-send-email-Suravee.Suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
Hi, On 03/12/2016 08:22 PM, Peter Zijlstra wrote: > On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote: >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> >> First, this patch move arch/x86/events/amd/iommu.h to >> arch/x86/include/asm/perf/amd/iommu.h so that we easily include >> it in both perf-amd-iommu and amd-iommu drivers. >> >> Then, we consolidate declaration of AMD IOMMU performance counter >> APIs into one file. > > These seem two independent thingies; should this therefore not be 2 > patches? > >> Reviewed-by: Joerg Roedel <jroedel@suse.de> >> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> --- >> arch/x86/events/amd/iommu.c | 2 +- >> arch/x86/events/amd/iommu.h | 40 --------------------------------- >> arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++ > > That seems somewhat excessive. Not only do you create > arch/x86/include/asm/perf/ you then put another directory on top of > that. > The original header files (arch/x86/events/amd/iommu.h and drivers/iommu/amd_iommu_proto.h) has duplicate function declarations. So, with the new header file being in the arch/x86/include/asm/perf/amd/iommu.h, we can just have one function declaration. So, you just want to separate the file moving part and the part that removes of the duplication? Thanks, Suravee
Hi, On 3/14/16 16:58, Peter Zijlstra wrote: > On Mon, Mar 14, 2016 at 12:26:00PM +0700, Suravee Suthikulpanit wrote: >> Hi, >> >> On 03/12/2016 08:22 PM, Peter Zijlstra wrote: >>> On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote: >>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >>>> >>>> First, this patch move arch/x86/events/amd/iommu.h to >>>> arch/x86/include/asm/perf/amd/iommu.h so that we easily include >>>> it in both perf-amd-iommu and amd-iommu drivers. >>>> >>>> Then, we consolidate declaration of AMD IOMMU performance counter >>>> APIs into one file. >>> >>> These seem two independent thingies; should this therefore not be 2 >>> patches? >>> >>>> Reviewed-by: Joerg Roedel <jroedel@suse.de> >>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >>>> --- >>>> arch/x86/events/amd/iommu.c | 2 +- >>>> arch/x86/events/amd/iommu.h | 40 --------------------------------- >>>> arch/x86/include/asm/perf/amd/iommu.h | 42 +++++++++++++++++++++++++++++++++++ >>> >>> That seems somewhat excessive. Not only do you create >>> arch/x86/include/asm/perf/ you then put another directory on top of >>> that. >>> >> >> The original header files (arch/x86/events/amd/iommu.h and >> drivers/iommu/amd_iommu_proto.h) has duplicate function declarations. So, >> with the new header file being in the arch/x86/include/asm/perf/amd/iommu.h, >> we can just have one function declaration. >> >> So, you just want to separate the file moving part and the part that removes >> of the duplication? > > I'm fine with a new header, it just seems putting it in a two deep > direcotry hierarchy of its own that seems excessive. > Basically, we are trying to match the current Perf hierarchy for AMD IOMMU (arch/x86/events/amd/iommu.c). I can put it into arch/x86/include/asm/perf_amd_iommu.h. What would you prefer? Thanks, Suravee
Hi Peter/Boris/Joerg, On 3/14/16 23:39, Peter Zijlstra wrote: > On Mon, Mar 14, 2016 at 03:19:45PM +0100, Borislav Petkov wrote: >> On Mon, Mar 14, 2016 at 08:37:02PM +0700, Suravee Suthikulpanit wrote: >>> Basically, we are trying to match the current Perf hierarchy for AMD IOMMU >>> (arch/x86/events/amd/iommu.c). I can put it into >>> arch/x86/include/asm/perf_amd_iommu.h. What would you prefer? >> >> Yeah, I was going to say the same thing - match the hierarchy so that >> there are no confusions between paths. Makes sense to me. > > Well there's still the 'perf' vs' events' thing, but also what other > files did you want to put there? > > For now I think I prefer a filename without extra directories; we can > always move files about if there's more use later. > > Also, since its being used by both events/amd/iommu.c and > drivers/iommu/amd_iommu.c you can also chose a name in the latter > namespace. > Actually, I also found that there is currently the include/linux/amd-iommu.h, which contains extern function declarations defined in drivers/iommu/amd_iommu_init.c and drivers/iommu/amd_iommu_v2.c. What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h into the include/linux/amd-iommu.h? I do not see the point of having to separate things out into two files. Joerg, since you were maintaining that file, do you have any objection? Thanks, Suravee
Hi On 03/15/2016 05:53 PM, Peter Zijlstra wrote: > On Tue, Mar 15, 2016 at 11:40:17AM +0100, Borislav Petkov wrote: >> On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote: >>> What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h >>> into the include/linux/amd-iommu.h? I do not see the point of having to >>> separate things out into two files. >> >> Except that this header has x86-specific stuff and include/linux/ is >> arch-agnostic. > > Which would suggest that header is placed wrong, because I would expect > the amd-iommu to really be rather x86 specific. Or is AMD making ARM > parts for which this is useful too? > Actually the exposed APIs (in both files) are from the AMD IOMMU driver, which is not necessary x86-specific. They mostly use struct pci_dev, which is also arch-agnostic. It is correct that the current IOMMU IP is only available in x86 systems. However, if AMD plans to use the IOMMU IP in the ARM-based processors in the future, putting these into include/linux/amd-iommu.h would work better. Thanks, Suravee
Hi Boris, On 03/18/2016 04:04 PM, Borislav Petkov wrote: > On Fri, Mar 18, 2016 at 02:07:25PM +0700, Suravee Suthikulpanit wrote: >> Actually the exposed APIs (in both files) are from the AMD IOMMU driver, >> which is not necessary x86-specific. They mostly use struct pci_dev, which >> is also arch-agnostic. It is correct that the current IOMMU IP is only >> available in x86 systems. However, if AMD plans to use the IOMMU IP in the >> ARM-based processors in the future, putting these into >> include/linux/amd-iommu.h would work better. > > Let's wait until AMD does that then. Moving the header is the easiest part. > But the whole point is that since we are moving it to consolidate these duplicated declarations, I think we should just put it in the most common place. The include/linux/amd-iommu.h file is already there. It's not like we have to create a brand new file, and then having to move it later. Regards, Suravee
On 03/18/2016 04:29 PM, Borislav Petkov wrote: > On Fri, Mar 18, 2016 at 04:09:33PM +0700, Suravee Suthikulpanit wrote: >> But the whole point is that since we are moving it to consolidate these >> duplicated declarations, I think we should just put it in the most common >> place. The include/linux/amd-iommu.h file is already there. It's not like we >> have to create a brand new file, and then having to move it later. > > Strictly speaking, it was wrong to put it there in the first place as it > is x86-specific. > If not here, where would you prefer? Consolidating it to arch/x86/include/asm/amd-iommu.h)? And if that's the case, should I also move include/linux/amd-iommu.h as well? Thanks, Suravee
diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c index 9da0d16..fb4aa7b 100644 --- a/arch/x86/events/amd/iommu.c +++ b/arch/x86/events/amd/iommu.c @@ -15,9 +15,9 @@ #include <linux/module.h> #include <linux/cpumask.h> #include <linux/slab.h> +#include <asm/perf/amd/iommu.h> #include "../../kernel/cpu/perf_event.h" -#include "iommu.h" #define COUNTER_SHIFT 16 diff --git a/arch/x86/events/amd/iommu.h b/arch/x86/events/amd/iommu.h deleted file mode 100644 index 845d173..0000000 --- a/arch/x86/events/amd/iommu.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright (C) 2013 Advanced Micro Devices, Inc. - * - * Author: Steven Kinney <Steven.Kinney@amd.com> - * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit@amd.com> - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - */ - -#ifndef _PERF_EVENT_AMD_IOMMU_H_ -#define _PERF_EVENT_AMD_IOMMU_H_ - -/* iommu pc mmio region register indexes */ -#define IOMMU_PC_COUNTER_REG 0x00 -#define IOMMU_PC_COUNTER_SRC_REG 0x08 -#define IOMMU_PC_PASID_MATCH_REG 0x10 -#define IOMMU_PC_DOMID_MATCH_REG 0x18 -#define IOMMU_PC_DEVID_MATCH_REG 0x20 -#define IOMMU_PC_COUNTER_REPORT_REG 0x28 - -/* maximun specified bank/counters */ -#define PC_MAX_SPEC_BNKS 64 -#define PC_MAX_SPEC_CNTRS 16 - -/* iommu pc reg masks*/ -#define IOMMU_BASE_DEVID 0x0000 - -/* amd_iommu_init.c external support functions */ -extern bool amd_iommu_pc_supported(void); - -extern u8 amd_iommu_pc_get_max_banks(u16 devid); - -extern u8 amd_iommu_pc_get_max_counters(u16 devid); - -extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, - u8 fxn, u64 *value, bool is_write); - -#endif /*_PERF_EVENT_AMD_IOMMU_H_*/ diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h new file mode 100644 index 0000000..72f64b7 --- /dev/null +++ b/arch/x86/include/asm/perf/amd/iommu.h @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2013 Advanced Micro Devices, Inc. + * + * Author: Steven Kinney <Steven.Kinney@amd.com> + * Author: Suravee Suthikulpanit <Suraveee.Suthikulpanit@amd.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef _PERF_EVENT_AMD_IOMMU_H_ +#define _PERF_EVENT_AMD_IOMMU_H_ + +/* iommu pc mmio region register indexes */ +#define IOMMU_PC_COUNTER_REG 0x00 +#define IOMMU_PC_COUNTER_SRC_REG 0x08 +#define IOMMU_PC_PASID_MATCH_REG 0x10 +#define IOMMU_PC_DOMID_MATCH_REG 0x18 +#define IOMMU_PC_DEVID_MATCH_REG 0x20 +#define IOMMU_PC_COUNTER_REPORT_REG 0x28 + +/* maximum specified bank/counters */ +#define PC_MAX_SPEC_BNKS 64 +#define PC_MAX_SPEC_CNTRS 16 + +/* iommu pc reg masks*/ +#define IOMMU_BASE_DEVID 0x0000 + +/* amd_iommu_init.c external support functions */ +bool amd_iommu_pc_supported(void); + +u8 amd_iommu_pc_get_max_banks(u16 devid); + +u8 amd_iommu_pc_get_max_counters(u16 devid); + +int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value); + +int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, + u64 *value, bool is_write); + +#endif /*_PERF_EVENT_AMD_IOMMU_H_*/ diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c index 013bdff..d30f4b2 100644 --- a/drivers/iommu/amd_iommu_init.c +++ b/drivers/iommu/amd_iommu_init.c @@ -27,6 +27,8 @@ #include <linux/amd-iommu.h> #include <linux/export.h> #include <linux/iommu.h> +#include <asm/perf/amd/iommu.h> + #include <asm/pci-direct.h> #include <asm/iommu.h> #include <asm/gart.h> diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h index 0bd9eb3..ac2da91 100644 --- a/drivers/iommu/amd_iommu_proto.h +++ b/drivers/iommu/amd_iommu_proto.h @@ -55,13 +55,6 @@ extern int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, int pasid, extern int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, int pasid); extern struct iommu_domain *amd_iommu_get_v2_domain(struct pci_dev *pdev); -/* IOMMU Performance Counter functions */ -extern bool amd_iommu_pc_supported(void); -extern u8 amd_iommu_pc_get_max_banks(u16 devid); -extern u8 amd_iommu_pc_get_max_counters(u16 devid); -extern int amd_iommu_pc_get_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, - u64 *value, bool is_write); - #ifdef CONFIG_IRQ_REMAP extern int amd_iommu_create_irq_domain(struct amd_iommu *iommu); #else