On Tue, Feb 23, 2016 at 08:12:36AM -0600, Suravee Suthikulpanit wrote:
> From: Suravee Suthikulpanit <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> 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.
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Suravee Suthikulpanit <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >>
> >>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 <[email protected]>
> >>Signed-off-by: Suravee Suthikulpanit <[email protected]>
> >>---
> >> 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.
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Suravee Suthikulpanit <[email protected]>
>>>> ---
>>>> 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
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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
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.
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
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.
>
Works for me. Thanks!
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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
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?
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
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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
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 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
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
On Fri, Mar 18, 2016 at 05:06:23PM +0700, Suravee Suthikulpanit wrote:
> 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?
Yeah, so arch/x86/include/asm/ has all the x86-specific stuff which is
not exported to userspace, so moving stuff there makes sense to me.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
On Fri, Mar 18, 2016 at 11:39:18AM +0100, Borislav Petkov wrote:
> Yeah, so arch/x86/include/asm/ has all the x86-specific stuff which is
> not exported to userspace, so moving stuff there makes sense to me.
While the AMD IOMMU is only available on x86 today, there is nothing
x86-specific in its architecture, so I don't think its header files
belong to arch/x86.
By that reasoning we could also move all of the intel gpu stuff to
arch/x86. But we don't do it, because it doesn't make sense and because
it doesn't belong there.
Regards,
Joerg
On Fri, Mar 18, 2016 at 12:11:01PM +0100, Joerg Roedel wrote:
> On Fri, Mar 18, 2016 at 11:39:18AM +0100, Borislav Petkov wrote:
> > Yeah, so arch/x86/include/asm/ has all the x86-specific stuff which is
> > not exported to userspace, so moving stuff there makes sense to me.
>
> While the AMD IOMMU is only available on x86 today, there is nothing
> x86-specific in its architecture, so I don't think its header files
> belong to arch/x86.
>
> By that reasoning we could also move all of the intel gpu stuff to
> arch/x86. But we don't do it, because it doesn't make sense and because
> it doesn't belong there.
And according to that argument of yours, we should move everything
which is not too close to the arch it is being used on, to generic
include/linux/. Because it might possibly get used some day by other
arches. Yeah right.
And looking at include/linux - it looks like a dumping ground for
headers. :-\
I guess anything is better than putting non-generic enough stuff in
include/linux/. Maybe drivers/include or something in that direction
would be better...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.