Prev: [tip:perf/core] perf trace: Clean up #includes
Next: [0/4] 2.6.27.50-stable review
From: Linus Torvalds on 6 Aug 2010 13:20
On Thu, Aug 5, 2010 at 1:37 PM, H. Peter Anvin <hpa(a)linux.intel.com> wrote:
>
> �git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-asm-for-linus
>
> H. Peter Anvin (7):
> x86, asm: Clean up and simplify set_64bit()

Hmm. This seems to cause compiler warnings for me on x86-64:

drivers/pci/intr_remapping.c: In function �modify_irte�:
drivers/pci/intr_remapping.c:314: warning: passing argument 1 of
�set_64bit� from incompatible pointer type
/home/torvalds/v2.6/linux/arch/x86/include/asm/cmpxchg_64.h:6: note:
expected �volatile u64 *� but argument is of type �long unsigned int
*�

and I'm not clear on the reason for that function prototype change.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 6 Aug 2010 13:50
On 08/06/2010 10:17 AM, Linus Torvalds wrote:
> On Thu, Aug 5, 2010 at 1:37 PM, H. Peter Anvin <hpa(a)linux.intel.com> wrote:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-asm-for-linus
>>
>> H. Peter Anvin (7):
>> x86, asm: Clean up and simplify set_64bit()
>
> Hmm. This seems to cause compiler warnings for me on x86-64:
>
> drivers/pci/intr_remapping.c: In function 'modify_irte':
> drivers/pci/intr_remapping.c:314: warning: passing argument 1 of
> 'set_64bit' from incompatible pointer type
> /home/torvalds/v2.6/linux/arch/x86/include/asm/cmpxchg_64.h:6: note:
> expected 'volatile u64 *' but argument is of type 'long unsigned int
> *'
>
> and I'm not clear on the reason for that function prototype change.
>
> Linus

On 64 bits the intent was to be consistent with the tightened-up 32-bit
code. Perhaps that was a mistake, but it seems a lot more consistent to
me to have "u64" be the type

On the 32-bit side, it's not so much a function prototype change as the
previous version of the code actually explicitly cast it to an array of
unsigned ints... it has the advantage that it can accept any type, but
the disadvantage that is can accept *ANY* type. The problem of course
is the above, since gcc will issue warnings for u64 (unsigned long long)
version the otherwise-identical "unsigned long".

It's worth noting that in this particular case the code itself looks
like this:

set_64bit((unsigned long *)&irte->low, irte_modified->low);
set_64bit((unsigned long *)&irte->high, irte_modified->high);

.... where the existing cast is there because irte->low and irte->high
are types __u64. In other words, with the "more logical" u64 prototype
the casts should just get removed.

On the other hand I should have seen this sooner, obviously.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 6 Aug 2010 14:10
On Fri, Aug 6, 2010 at 10:45 AM, H. Peter Anvin <hpa(a)zytor.com> wrote:
>
> It's worth noting that in this particular case the code itself looks
> like this:
>
> � � � �set_64bit((unsigned long *)&irte->low, irte_modified->low);
> � � � �set_64bit((unsigned long *)&irte->high, irte_modified->high);
>
> ... where the existing cast is there because irte->low and irte->high
> are types __u64. �In other words, with the "more logical" u64 prototype
> the casts should just get removed.

Ok, right you are. I'll just remove the casts, since that makes the
code look better. Maybe it will cause warnings on some other plaform,
but I do agree that maybe the right thing to do is to just say
"set_64bit() should have taken a u64 * to begin with".

> On the other hand I should have seen this sooner, obviously.

Yeah, I didn't look at the code, I just reacted to "hmm, this results
in new warnings, not good".

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 6 Aug 2010 14:20
On 08/06/2010 10:45 AM, H. Peter Anvin wrote:
>
> It's worth noting that in this particular case the code itself looks
> like this:
>
> set_64bit((unsigned long *)&irte->low, irte_modified->low);
> set_64bit((unsigned long *)&irte->high, irte_modified->high);
>
> ... where the existing cast is there because irte->low and irte->high
> are types __u64. In other words, with the "more logical" u64 prototype
> the casts should just get removed.
>

Looking through the build I'm currently running, so far, it is smoking
out a bunch of unnecessary casts and wrappers, for example, in kvm/mmu.c:

static void __set_spte(u64 *sptep, u64 spte)
{
#ifdef CONFIG_X86_64
set_64bit((unsigned long *)sptep, spte);
#else
set_64bit((unsigned long long *)sptep, spte);
#endif
}

.... which just becomes the much cleaner ...

static void __set_spte(u64 *sptep, u64 spte)
{
set_64bit(sptep, spte);
}

I'll go through all these as this build finishes and give you an updated
tree to pull, ok?

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [tip:perf/core] perf trace: Clean up #includes
Next: [0/4] 2.6.27.50-stable review