Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1356419 > unrolled thread

Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-12 14:30 +0100
    Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-14 06:30 +0100
      Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-14 11:00 +0100
        Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-14 15:30 +0100
          Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-14 17:40 +0100
            Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-15 09:50 +0100
            Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-15 11:50 +0100
              Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Peter Zijlstra <peterz@infradead.org> - 2016-03-15 12:00 +0100
                Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-18 10:10 +0100
                  Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-18 10:30 +0100
                    Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-18 11:40 +0100
                      Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Joerg Roedel <joro@8bytes.org> - 2016-03-18 12:20 +0100
                        Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move  perf_event_amd_iommu header Borislav Petkov <bp@alien8.de> - 2016-03-18 12:40 +0100

#1356419 — Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

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.

[toc] | [next] | [standalone]

#1356947

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

[toc] | [prev] | [next] | [standalone]

#1357109

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.

[toc] | [prev] | [next] | [standalone]

#1357278

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.

[toc] | [prev] | [next] | [standalone]

#1357403

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.

[toc] | [prev] | [next] | [standalone]

#1357907

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!

[toc] | [prev] | [next] | [standalone]

#1357981

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.

[toc] | [prev] | [next] | [standalone]

#1357985

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?

[toc] | [prev] | [next] | [standalone]

#1360451

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.

[toc] | [prev] | [next] | [standalone]

#1360458

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.

[toc] | [prev] | [next] | [standalone]

#1360510

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.

[toc] | [prev] | [next] | [standalone]

#1360529

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

[toc] | [prev] | [next] | [standalone]

#1360551

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web