Re: [PATCH 7/8] Add 32 bit VDSO time support for 32 bit kernel

[Andy Lutomirski]

On Mon, Feb 3, 2014 at 2:01 PM, Stefani Seibold <stefani@xxxxxxxxxxx> wrote:
> Am Montag, den 03.02.2014, 08:36 -0800 schrieb Andy Lutomirski:
>> On Sun, Feb 2, 2014 at 11:44 PM, Stefani Seibold <stefani@xxxxxxxxxxx> wrote:
>> > Am Sonntag, den 02.02.2014, 16:12 -0800 schrieb Andy Lutomirski:
>> >> On Sun, Feb 2, 2014 at 1:39 PM, Stefani Seibold <stefani@xxxxxxxxxxx> wrote:
>> >> > Am Sonntag, den 02.02.2014, 08:46 -0800 schrieb Andy Lutomirski:
>> >> >> On Sun, Feb 2, 2014 at 3:27 AM,  <stefani@xxxxxxxxxxx> wrote:
>> >> >> > From: Stefani Seibold <stefani@xxxxxxxxxxx>
>> >> >> >
>> >> >> > This patch add the time support for 32 bit a VDSO to a 32 bit kernel.
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> Can you address the review comments from last time around?  For
>> >> >> example, this still seems to have redundant vvar and hpet mappings, it
>> >> >> doesn't use the VVAR macro, it moves the 32-bit compat vDSO, etc.
>> >> >>
>> >> >
>> >> > I will address the compat VDSO issue.
>> >> >
>> >> > But the VVAR macro will be not a part of this patch set. If you depend
>> >> > on this, feel free to create one. From my point of view this is not
>> >> > feasible without a macro hacking, because the address accessing the vvar
>> >> > area differs in kernel and VDSO user mode.
>> >>
>> >> Sorry, but "I will make the code messier for no apparent reason and I
>> >> will not offer to fix it in the same series" gets my NAK.
>> >>
>> >> Hint: I'm talking about two or three lines of code in vvar.h.
>> >>
>> >
>> > A hint back: if you threat me with a NAK for a requested code sequence
>> > which currently no user, this is far away from professional. I am not
>> > your trainee.
>> >
>> > BTW: If it is so easy, send me the two or three lines and i will merge
>> > it ;-)
>>
>> Something to the effect of:
>>
>> #elif defined(BUILD_VDSO32)
>> #define VVAR(name) (*vvar_ ## name)
>> #endif
>>
>> Should do the trick.
>
> You are wrong...
>
> #ifdef BUILD_VDSO32
>
> #define DECLARE_VVAR(offset, type, name) \
>         extern type vvar_ ## name __attribute__((visibility("hidden")));
>
> #define VVAR(name) (vvar_ ## name)
>
> #else
>
> /* Base address of vvars.  This is not ABI. */
> #ifdef CONFIG_X86_64
> #define VVAR_ADDRESS (-10*1024*1024 - 4096)
> #else
> extern char __vvar_page;
>
> #define VVAR_ADDRESS (&__vvar_page)
> #endif
>
> This would do the trick!
>
> But for 64 bit to 32 bit conversation layer in vclock_gettime.c there is
> still a
>
> struct arch_vsyscall_gtod_data arch_vvar_vsyscall_gtod_data
>         __attribute__((visibility("hidden")));
> #define gtod (&arch_vvar_vsyscall_gtod_data)
>
> needed, because vvar_vsyscall_gtod_data is the 32 bit version, which
> would result in incorrect access of the struct members. So my code can't
> use this VVAR macro.

For 32-on-64, I must have read your code wrong.  Are you sticking two
copies of the same struct with different layout into the vvar page?
If so, wouldn't it be better to only have the variant with a common
layout and use it for all versions of the vdso?

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/