linux-kernel - Re: [patch 05/24] perfmon: X86 generic code (x86)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c86c4470811270151i33ffaa44mb5d9bd79f9d73968@mail.gmail.com>
Date:	Thu, 27 Nov 2008 10:51:36 +0100
From:	"stephane eranian" <eranian@...glemail.com>
To:	"Thomas Gleixner" <tglx@...utronix.de>
Cc:	"Stephen Rothwell" <sfr@...b.auug.org.au>,
	"Andi Kleen" <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, mingo@...e.hu, x86@...nel.org
Subject: Re: [patch 05/24] perfmon: X86 generic code (x86)

Thomas,

On Wed, Nov 26, 2008 at 5:38 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> Stephane,
>
> On Wed, 26 Nov 2008, stephane eranian wrote:
>> > I have not yet found a good reason why it needs to use u64 instead of
>> > using what's there already.
>> >
>> There is a good reason why we cannot use unsigned long. We must make sure
>> all data structures exchanged between user mode and the kernel have fixed size.
>> This way, we can have a 32-bit tool run unmodified on top of a 64-bit kernel AND
>> we do not need trampoline code to marshall/unmarshall the parameters.
>
> That's not a good reason at all. We have in kernel interfaces and
> kernel-userspace interfaces. Making them the same is nice if it works,
> but horrible if it imposes crappy hackery like the bitops wrappers.
>
>> And yes, the abstraction for bitmask ops was introduced because of issues
>> casting u64 -> unsigned long on Big-Endian32-bit machines such as PPC32.
>
> Sorry, I think it is simply stupid.
>
> You can keep the userspace interface u64 and use unsigned long for the
> bitmasks in the kernel and take care of it in the user space interface
> code and do the BE32 conversion when you copy stuff from and to user.
>
> That's a single well defined place and does not add extra crappola
> over the kernel especially not into hot pathes like the interrupt.
>
> Why do you want to do u64 -> u32 BE32 magic on every interrupt,
> context switch etc., if you can do it once in the userspace interface ?
>
I think we agree as to why the user visible structures have to have fixed size.

Some structures have bitfields in them (no in the current patchset yet).
Here is an example:

#define PFM_PMD_BV   (256/sizeof(__u64))

struct pfarg_pmd_attr {
        __u16 reg_num;          /* which register */
        __u16 reg_set;          /* which event set */
        __u32 reg_flags;        /* REGFL flags */
        __u64 reg_value;        /* 64-bit value */
        __u64 reg_long_reset;   /* write: value to reload after notification */
        __u64 reg_short_reset;  /* write: reset after counter overflow */
        __u64 reg_random_mask;  /* write: bitmask used to limit random value */
        __u64 reg_smpl_pmds[PFM_PMD_BV];  /* write: record in sample */
        __u64 reg_reset_pmds[PFM_PMD_BV]; /* write: reset on overflow */
        __u64 reg_ovfl_swcnt;   /* write: # of overflows before switch */
        __u64 reg_smpl_eventid; /* write: opaque event identifier */
        __u64 reg_last_value;   /* read : return: PMD last reset value */
        __u64 reg_reserved[8];  /* for future use */
};

So you are advocating keeping that layout for the user level code,
i.e., the user level perfmon.h.
But then, in the kernel, you'd have a different version of the same
structure with the same name
and the size but with the bitmask defined as unsigned long instead.
All internal only bitmask
would also be unsigned long. So the structure would like as follows:

#define PFM_PMD_BV  (256/sizeof(unsigned long))
struct pfarg_pmd_attr {
        __u16 reg_num;          /* which register */
        __u16 reg_set;          /* which event set */
        __u32 reg_flags;        /* REGFL flags */
        __u64 reg_value;        /* 64-bit value */
        __u64 reg_long_reset;   /* write: value to reload after notification */
        __u64 reg_short_reset;  /* write: reset after counter overflow */
        __u64 reg_random_mask;  /* write: bitmask used to limit random value */
        unsigned long reg_smpl_pmds[PFM_PMD_BV];  /* write: record in sample */
        unsigned long reg_reset_pmds[PFM_PMD_BV]; /* write: reset on overflow */
        __u64 reg_ovfl_swcnt;   /* write: # of overflows before switch */
        __u64 reg_smpl_eventid; /* write: opaque event identifier */
        __u64 reg_last_value;   /* read : return: PMD last reset value */
        __u64 reg_reserved[8];  /* for future use */
};

Then we could not directly export include/linux/perfmon.h to user
via Kbuild.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/