Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
On 30.11.2022 10:26, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 02:12:10PM -0800, Stefano Stabellini wrote:
>> On Tue, 29 Nov 2022, Roger Pau Monne wrote:
>>> The hvc machinery registers both a console and a tty device based on
>>> the hv ops provided by the specific implementation. Those two
>>> interfaces however have different locks, and there's no single locks
>>> that's shared between the tty and the console implementations, hence
>>> the driver needs to protect itself against concurrent accesses.
>>> Otherwise concurrent calls using the split interfaces are likely to
>>> corrupt the ring indexes, leaving the console unusable.
>>>
>>> Introduce a lock to xencons_info to serialize accesses to the shared
>>> ring. This is only required when using the shared memory console,
>>> concurrent accesses to the hypercall based console implementation are
>>> not an issue.
>>>
>>> Note the conditional logic in domU_read_console() is slightly modified
>>> so the notify_daemon() call can be done outside of the locked region:
>>> it's an hypercall and there's no need for it to be done with the lock
>>> held.
>>>
>>> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen
>>> console')
>>> Signed-off-by: Roger Pau Monné
>>> ---
>>> While the write handler (domU_write_console()) is used by both the
>>> console and the tty ops, that's not the case for the read side
>>> (domU_read_console()). It's not obvious to me whether we could get
>>> concurrent poll calls from the poll_get_char tty hook, hence stay on
>>> the safe side also serialize read accesses in domU_read_console().
>>
>> I think domU_read_console doesn't need it. struct hv_ops and struct
>> console are both already locked although independently locked.
>>
>> I think we shouldn't add an unrequired lock there.
>
> Not all accesses are done using the tty lock. There's a path using
> tty_find_polling_driver() in kgdboc.c that directly calls into the
> ->poll_get_char() hook without any locks apparently taken.
Simply by the name of the file I'm inclined to say that debugger code
not respecting locks may be kind of intentional (but would then need
to be accompanied by certain other precautions there).
Jan
Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices
On 22.10.2021 08:47, Juergen Gross wrote: > Today the non-essential pv devices are hard coded in the xenbus driver > and this list is lacking multiple entries. > > This series reworks the detection logic of non-essential devices by > adding a flag for that purpose to struct xenbus_driver. I'm wondering whether it wouldn't better be the other way around: The (hopefully few) essential ones get flagged, thus also making it more prominent during patch review that a flag gets added (and justification provided), instead of having to spot the lack of a flag getting set. Jan
[PATCH v2 5/9] xen/x86: make "earlyprintk=xen" work for HVM/PVH DomU
xenboot_write_console() is dealing with these quite fine so I don't see
why xenboot_console_setup() would return -ENOENT in this case.
Adjust documentation accordingly.
Signed-off-by: Jan Beulich
Reviewed-by: Juergen Gross
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1266,7 +1266,7 @@
The VGA and EFI output is eventually overwritten by
the real console.
- The xen output can only be used by Xen PV guests.
+ The xen option can only be used in Xen domains.
The sclp output can only be used on s390.
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -618,10 +618,8 @@ static int __init xenboot_console_setup(
{
static struct xencons_info xenboot;
- if (xen_initial_domain())
+ if (xen_initial_domain() || !xen_pv_domain())
return 0;
- if (!xen_pv_domain())
- return -ENODEV;
return xencons_info_pv_init(&xenboot, 0);
}
[PATCH v2 3/9] xen/x86: make "earlyprintk=xen" work better for PVH Dom0
The xen_hvm_early_write() path better wouldn't be taken in this case;
while port 0xE9 can be used, the hypercall path is quite a bit more
efficient. Put that first, as it may also work for DomU-s (see also
xen_raw_console_write()).
While there also bail from the function when the first
domU_write_console() failed - later ones aren't going to succeed.
Signed-off-by: Jan Beulich
Reviewed-by: Juergen Gross
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -632,17 +632,16 @@ static void xenboot_write_console(struct
unsigned int linelen, off = 0;
const char *pos;
+ if (dom0_write_console(0, string, len) >= 0)
+ return;
+
if (!xen_pv_domain()) {
xen_hvm_early_write(0, string, len);
return;
}
- dom0_write_console(0, string, len);
-
- if (xen_initial_domain())
+ if (domU_write_console(0, "(early) ", 8) < 0)
return;
-
- domU_write_console(0, "(early) ", 8);
while (off < len && NULL != (pos = strchr(string+off, '\n'))) {
linelen = pos-string+off;
if (off + linelen > len)
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 17.09.2021 11:36, Roman Skakun wrote: > I use Xen PV display. In my case, PV display backend(Dom0) allocates > contiguous buffer via DMA-API to > to implement zero-copy between Dom0 and DomU. Why does the buffer need to be allocated by Dom0? If it was allocated by DomU, it could use grants to give the backend direct (zero-copy) access. Jan
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 15.09.2021 15:37, Roman Skakun wrote: >>> From: Roman Skakun >>> >>> It is possible when default IO TLB size is not >>> enough to fit a long buffers as described here [1]. >>> >>> This patch makes a way to set this parameter >>> using cmdline instead of recompiling a kernel. >>> >>> [1] https://www.xilinx.com/support/answers/72694.html >> >> I'm not convinced the swiotlb use describe there falls under "intended >> use" - mapping a 1280x720 framebuffer in a single chunk? > > I had the same issue while mapping DMA chuck ~4MB for gem fb when > using xen vdispl. > I got the next log: > [ 142.030421] rcar-fcp fea2f000.fcp: swiotlb buffer is full (sz: > 3686400 bytes), total 32768 (slots), used 32 (slots) > > It happened when I tried to map bounce buffer, which has a large size. > The default size if 128(IO_TLB_SEGSIZE) * 2048(IO_TLB_SHIFT) = 262144 > bytes, but we requested 3686400 bytes. > When I change IO_TLB_SEGSIZE to 2048. (2048(IO_TLB_SEGSIZE) * > 2048(IO_TLB_SHIFT) = 4194304bytes). > It makes possible to retrieve a bounce buffer for requested size. > After changing this value, the problem is gone. But the question remains: Why does the framebuffer need to be mapped in a single giant chunk? >> In order to be sure to catch all uses like this one (including ones >> which make it upstream in parallel to yours), I think you will want >> to rename the original IO_TLB_SEGSIZE to e.g. IO_TLB_DEFAULT_SEGSIZE. > > I don't understand your point. Can you clarify this? There's a concrete present example: I have a patch pending adding another use of IO_TLB_SEGSIZE. This use would need to be replaced like you do here in several places. The need for the additional replacement would be quite obvious (from a build failure) if you renamed the manifest constant. Without renaming, it'll take someone running into an issue on a live system, which I consider far worse. This is because a simple re-basing of one of the patches on top of the other will not point out the need for the extra replacement, nor would a test build (with both patches in place). Jan
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 14.09.2021 17:10, Roman Skakun wrote:
> From: Roman Skakun
>
> It is possible when default IO TLB size is not
> enough to fit a long buffers as described here [1].
>
> This patch makes a way to set this parameter
> using cmdline instead of recompiling a kernel.
>
> [1] https://www.xilinx.com/support/answers/72694.html
I'm not convinced the swiotlb use describe there falls under "intended
use" - mapping a 1280x720 framebuffer in a single chunk? (As an aside,
the bottom of this page is also confusing, as following "Then we can
confirm the modified swiotlb size in the boot log:" there is a log
fragment showing the same original size of 64Mb.
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -237,7 +237,7 @@ void __init plat_swiotlb_setup(void)
> swiotlbsize = 64 * (1<<20);
> #endif
> swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> - swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> + swiotlb_nslabs = ALIGN(swiotlb_nslabs, swiotlb_io_seg_size());
In order to be sure to catch all uses like this one (including ones
which make it upstream in parallel to yours), I think you will want
to rename the original IO_TLB_SEGSIZE to e.g. IO_TLB_DEFAULT_SEGSIZE.
> @@ -81,15 +86,30 @@ static unsigned int max_segment;
> static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
>
> static int __init
> -setup_io_tlb_npages(char *str)
> +setup_io_tlb_params(char *str)
> {
> + unsigned long tmp;
> +
> if (isdigit(*str)) {
> - /* avoid tail segment of size < IO_TLB_SEGSIZE */
> - default_nslabs =
> - ALIGN(simple_strtoul(str, &str, 0), IO_TLB_SEGSIZE);
> + default_nslabs = simple_strtoul(str, &str, 0);
> }
> if (*str == ',')
> ++str;
> +
> + /* get max IO TLB segment size */
> + if (isdigit(*str)) {
> + tmp = simple_strtoul(str, &str, 0);
> + if (tmp)
> + io_tlb_seg_size = ALIGN(tmp, IO_TLB_SEGSIZE);
>From all I can tell io_tlb_seg_size wants to be a power of 2. Merely
aligning to a multiple of IO_TLB_SEGSIZE isn't going to be enough.
Jan
[PATCH 3/9] xen/x86: make "earlyprintk=xen" work better for PVH Dom0
The xen_hvm_early_write() path better wouldn't be taken in this case;
while port 0xE9 can be used, the hypercall path is quite a bit more
efficient. Put that first, as it may also work for DomU-s (see also
xen_raw_console_write()).
While there also bail from the function when the first
domU_write_console() failed - later ones aren't going to succeed.
Signed-off-by: Jan Beulich
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -632,17 +632,16 @@ static void xenboot_write_console(struct
unsigned int linelen, off = 0;
const char *pos;
+ if (dom0_write_console(0, string, len) >= 0)
+ return;
+
if (!xen_pv_domain()) {
xen_hvm_early_write(0, string, len);
return;
}
- dom0_write_console(0, string, len);
-
- if (xen_initial_domain())
+ if (domU_write_console(0, "(early) ", 8) < 0)
return;
-
- domU_write_console(0, "(early) ", 8);
while (off < len && NULL != (pos = strchr(string+off, '\n'))) {
linelen = pos-string+off;
if (off + linelen > len)
[PATCH 5/9] xen/x86: make "earlyprintk=xen" work for HVM/PVH DomU
xenboot_write_console() is dealing with these quite fine so I don't see
why xenboot_console_setup() would return -ENOENT in this case.
Adjust documentation accordingly.
Signed-off-by: Jan Beulich
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1266,7 +1266,7 @@
The VGA and EFI output is eventually overwritten by
the real console.
- The xen output can only be used by Xen PV guests.
+ The xen option can only be used in Xen domains.
The sclp output can only be used on s390.
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -618,10 +618,8 @@ static int __init xenboot_console_setup(
{
static struct xencons_info xenboot;
- if (xen_initial_domain())
+ if (xen_initial_domain() || !xen_pv_domain())
return 0;
- if (!xen_pv_domain())
- return -ENODEV;
return xencons_info_pv_init(&xenboot, 0);
}
Re: [PATCH v2] xen/hvc: replace BUG_ON() with negative return value
On 07.07.2021 11:10, Juergen Gross wrote: > Xen frontends shouldn't BUG() in case of illegal data received from > their backends. So replace the BUG_ON()s when reading illegal data from > the ring page with negative return values. > > Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -86,7 +86,11 @@ static int __write_console(struct xencons_info *xencons, > cons = intf->out_cons; > prod = intf->out_prod; > mb(); /* update queue values before going on */ Largely unrelated note: While in general the barriers here may want switching to virt_*mb(), this particular one looks to be too heavy anyway: a read barrier is all that's needed here afaict, just like there's only a write barrier between ring contents and producer writing in __write_console(). And btw, since I've got puzzled by the linuxppc-dev@ in the recipients list, I did look up relevant entries in ./MAINTAINERS. Shouldn't the file be part of "XEN HYPERVISOR INTERFACE"? Jan
[PATCH] hvc_xen: avoid uninitialized variable warning
Older compilers don't recognize that "v" can't be used uninitialized;
other code using hvm_get_parameter() zeros the value too, so follow
suit here.
Signed-off-by: Jan Beulich
---
drivers/tty/hvc/hvc_xen.c |2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
--- 4.1-rc5/drivers/tty/hvc/hvc_xen.c
+++ 4.1-rc5-xen-unint-warnings/drivers/tty/hvc/hvc_xen.c
@@ -302,7 +302,7 @@ static int xen_initial_domain_console_in
static void xen_console_update_evtchn(struct xencons_info *info)
{
if (xen_hvm_domain()) {
- uint64_t v;
+ uint64_t v = 0;
int err;
err = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 4/6] x86: Add clear_page_nocache
>>> On 13.08.12 at 13:43, "Kirill A. Shutemov"
>>> wrote:
> On Thu, Aug 09, 2012 at 04:22:04PM +0100, Jan Beulich wrote:
>> >>> On 09.08.12 at 17:03, "Kirill A. Shutemov"
>> >>> wrote:
>
> ...
>
>> > ---
>> > arch/x86/include/asm/page.h |2 ++
>> > arch/x86/include/asm/string_32.h |5 +
>> > arch/x86/include/asm/string_64.h |5 +
>> > arch/x86/lib/Makefile|1 +
>> > arch/x86/lib/clear_page_nocache_32.S | 30 ++
>> > arch/x86/lib/clear_page_nocache_64.S | 29 +
>>
>> Couldn't this more reasonably go into clear_page_{32,64}.S?
>
> We don't have clear_page_32.S.
Sure, but you're introducing a file anyway. Fold the new code into
the existing file for 64-bit, and create a new, similarly named one
for 32-bit.
>> >+ xorl %eax,%eax
>> >+ movl $4096/64,%ecx
>> >+ .p2align 4
>> >+.Lloop:
>> >+ decl%ecx
>> >+#define PUT(x) movnti %eax,x*8(%edi) ; movnti %eax,x*8+4(%edi)
>>
>> Is doing twice as much unrolling as on 64-bit really worth it?
>
> Moving 64 bytes per cycle is faster on Sandy Bridge, but slower on
> Westmere. Any preference? ;)
If it's not a clear win, I'd favor the 8-stores-per-cycle variant,
matching x86-64.
Jan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 4/6] x86: Add clear_page_nocache
>>> On 09.08.12 at 17:03, "Kirill A. Shutemov"
>>> wrote:
> From: Andi Kleen
>
> Add a cache avoiding version of clear_page. Straight forward integer variant
> of the existing 64bit clear_page, for both 32bit and 64bit.
While on 64-bit this is fine, I fail to see how you avoid using the
SSE2 instruction on non-SSE2 systems.
> Also add the necessary glue for highmem including a layer that non cache
> coherent architectures that use the virtual address for flushing can
> hook in. This is not needed on x86 of course.
>
> If an architecture wants to provide cache avoiding version of clear_page
> it should to define ARCH_HAS_USER_NOCACHE to 1 and implement
> clear_page_nocache() and clear_user_highpage_nocache().
>
> Signed-off-by: Andi Kleen
> Signed-off-by: Kirill A. Shutemov
> ---
> arch/x86/include/asm/page.h |2 ++
> arch/x86/include/asm/string_32.h |5 +
> arch/x86/include/asm/string_64.h |5 +
> arch/x86/lib/Makefile|1 +
> arch/x86/lib/clear_page_nocache_32.S | 30 ++
> arch/x86/lib/clear_page_nocache_64.S | 29 +
Couldn't this more reasonably go into clear_page_{32,64}.S?
> arch/x86/mm/fault.c |7 +++
> 7 files changed, 79 insertions(+), 0 deletions(-)
> create mode 100644 arch/x86/lib/clear_page_nocache_32.S
> create mode 100644 arch/x86/lib/clear_page_nocache_64.S
>...
>--- /dev/null
>+++ b/arch/x86/lib/clear_page_nocache_32.S
>@@ -0,0 +1,30 @@
>+#include
>+#include
>+
>+/*
>+ * Zero a page avoiding the caches
>+ * rdipage
Wrong comment.
>+ */
>+ENTRY(clear_page_nocache)
>+ CFI_STARTPROC
>+ mov%eax,%edi
You need to pick a different register here (e.g. %edx), since
%edi has to be preserved by all functions called from C.
>+ xorl %eax,%eax
>+ movl $4096/64,%ecx
>+ .p2align 4
>+.Lloop:
>+ decl%ecx
>+#define PUT(x) movnti %eax,x*8(%edi) ; movnti %eax,x*8+4(%edi)
Is doing twice as much unrolling as on 64-bit really worth it?
Jan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 6/6] x86: switch the 64bit uncached page clear to SSE/AVX v2
>>> On 09.08.12 at 17:03, "Kirill A. Shutemov" >>> wrote: > ENTRY(clear_page_nocache) > CFI_STARTPROC > - xorl %eax,%eax > - movl $4096/64,%ecx > + push %rdi > + call kernel_fpu_begin > + pop%rdi You use CFI annotations elsewhere, so why don't you use pushq_cfi/popq_cfi here? Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: build warning in Linus' tree
>>> On 18.11.10 at 04:55, Benjamin Herrenschmidt >>> wrote: > On Mon, 2010-10-25 at 17:03 +1100, Stephen Rothwell wrote: >> Hi Arnaud, >> >> On Sun, 24 Oct 2010 23:47:09 -0400 Arnaud Lacombe wrote: >> > >> > The following patch should fix this warning. >> >> I think the following is preferable. Not tested yet, I will test >> tomorrow and submit properly then. > > I agree. Tho I don't understand the original problem. Both definitions > have no parenthesis for me and I don't see warnings, or am I missing > some changes still in -next that cause this ? There had been parentheses for a short while, but them causing problems for at least one other arch, they got removed again. Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: tree build failure
>>> Hollis Blanchard 15.10.09 00:57 >>> >On Fri, 2009-10-09 at 12:14 -0700, Hollis Blanchard wrote: >> Rusty's version of BUILD_BUG_ON() does indeed fix the build break, and >> also exposes the bug in kvmppc_account_exit_stat(). So to recap: >> >> original: built but didn't work >> Jan's: doesn't build >> Rusty's: builds and works >> >> Where do you want to go from here? > >Jan, what are your thoughts? Your BUILD_BUG_ON patch has broken the >build, and we still need to fix it. My perspective is that it just uncovered already existing brokenness. And honestly, I won't be able to get to look into this within the next days. (And btw., when I run into issues with other people's code changes, quite frequently I'm told to propose a patch, so I'm also having some philosophical problem understanding why I can't simply expect the same when people run into issues with changes I made, especially in cases like this where it wasn't me introducing the broken code.) So, if this can wait for a couple of days, I can try to find time to look into this. Otherwise, I'd rely on someone running into the actual issue to implement a solution. Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: tree build failure
>>> Hollis Blanchard 02.10.09 17:48 >>>
>On Wed, 2009-09-30 at 07:35 +0100, Jan Beulich wrote:
>> The one Rusty suggested the other day may help here. I don't like it
>> as a drop-in replacement for BUILD_BUG_ON() though (due to it
>> deferring the error generated to the linking stage), I'd rather view
>> this as an improvement to MAYBE_BUILD_BUG_ON() (which should
>> then be used here).
>
>Can you be more specific?
>
>I have no idea what Rusty suggested where. I can't even guess what
I'm attaching Rusty's response I was referring to.
>MAYBE_BUILD_BUG_ON() is supposed to do (sounds like a terrible name).
Agreed - but presumably better than just deleting the bogus instances
altogether...
Jan
--- Begin Message ---
On Wed, 19 Aug 2009 01:29:25 am Jan Beulich wrote:
> gcc permitting variable length arrays makes the current construct
> used for BUILD_BUG_ON() useless, as that doesn't produce any diagnostic
> if the controlling expression isn't really constant. Instead, this
> patch makes it so that a bit field gets used here. Consequently, those
> uses where the condition isn't really constant now also need fixing.
>
> Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
> MAYBE_BUILD_BUG_ON() really just serves documentation purposes - even
> if the expression is compile time constant (__builtin_constant_p()
> yields true), the array is still deemed of variable length by gcc, and
> hence the whole expression doesn't have the intended effect.
>
> Signed-off-by: Jan Beulich
We used to use an undefined symbol here; diagnostics are worse but it catches
more stuff.
Perhaps a hybrid is the way to go?
#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#else
/* If it's a constant, catch it at compile time, otherwise at link time. */
extern int __build_bug_on_failed;
#define BUILD_BUG_ON(condition) \
do {\
((void)sizeof(char[1 - 2*!!(condition)])); \
if (condition) __build_bug_on_failed = 1; \
} while(0)
#endif
Thanks,
Rusty.
--- End Message ---
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: tree build failure
>>> Hollis Blanchard 30.09.09 01:39 >>> >On Tue, 2009-09-29 at 10:28 +0100, Jan Beulich wrote: >> >>> Hollis Blanchard 09/29/09 2:00 AM >>> >> >First, I think there is a real bug here, and the code should read like >> >this (to match the comment): >> >/* type has to be known at build time for optimization */ >> >-BUILD_BUG_ON(__builtin_constant_p(type)); >> >+BUILD_BUG_ON(!__builtin_constant_p(type)); >> > >> >However, I get the same build error *both* ways, i.e. >> >__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or >> >the new BUILD_BUG_ON() macro isn't working... >> >> No, at this point of the compilation process it's neither zero nor one, >> it's simply considered non-constant by the compiler at that stage >> (this builtin is used for optimization, not during parsing, and the >> error gets generated when the body of the function gets parsed, >> not when code gets generated from it). > >I think I see what you're saying. Do you have a fix to suggest? The one Rusty suggested the other day may help here. I don't like it as a drop-in replacement for BUILD_BUG_ON() though (due to it deferring the error generated to the linking stage), I'd rather view this as an improvement to MAYBE_BUILD_BUG_ON() (which should then be used here). Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: tree build failure
>>> roel kluin 29.09.09 11:51 >>> >On Tue, Sep 29, 2009 at 11:28 AM, Jan Beulich wrote: >>>>> Hollis Blanchard 09/29/09 2:00 AM >>> >>>First, I think there is a real bug here, and the code should read like >>>this (to match the comment): >>>/* type has to be known at build time for optimization */ >>>-BUILD_BUG_ON(__builtin_constant_p(type)); >>>+BUILD_BUG_ON(!__builtin_constant_p(type)); >>> >>>However, I get the same build error *both* ways, i.e. >>>__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or >>>the new BUILD_BUG_ON() macro isn't working... >> >> No, at this point of the compilation process it's neither zero nor one, >> it's simply considered non-constant by the compiler at that stage >> (this builtin is used for optimization, not during parsing, and the >> error gets generated when the body of the function gets parsed, >> not when code gets generated from it). >> >> Jan > >then maybe > >if(__builtin_constant_p(type)) >BUILD_BUG_ON(1); > >would work? Definitely not - this would result in the compiler *always* generating an error. Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: tree build failure
>>> Hollis Blanchard 09/29/09 2:00 AM >>> >First, I think there is a real bug here, and the code should read like >this (to match the comment): >/* type has to be known at build time for optimization */ >-BUILD_BUG_ON(__builtin_constant_p(type)); >+BUILD_BUG_ON(!__builtin_constant_p(type)); > >However, I get the same build error *both* ways, i.e. >__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or >the new BUILD_BUG_ON() macro isn't working... No, at this point of the compilation process it's neither zero nor one, it's simply considered non-constant by the compiler at that stage (this builtin is used for optimization, not during parsing, and the error gets generated when the body of the function gets parsed, not when code gets generated from it). Jan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH, ppc64] improve dedotify()
This completely untested patch is intended to be a suggestion only:
Code inspection for an entirely different purpose made me stumble
across this, and I think that modifying the string table of an ELF
object is a bad idea, since there's nothing disallowing a linker to
merge strings inside the table, which would result in this code
possibly, but unintentionally screwing up other symbol names.
Besides that, the presented alternative is both smaller and faster.
Signed-off-by: Jan Beulich <[EMAIL PROTECTED]>
---
arch/powerpc/kernel/module_64.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
--- linux-2.6.24-rc7/arch/powerpc/kernel/module_64.c2007-02-04
19:44:54.0 +0100
+++ 2.6.24-rc7-ppc64-dedotify/arch/powerpc/kernel/module_64.c 2008-01-08
13:32:33.0 +0100
@@ -154,16 +154,14 @@ static void dedotify_versions(struct mod
}
/* Undefined symbols which refer to .funcname, hack to funcname */
-static void dedotify(Elf64_Sym *syms, unsigned int numsyms, char *strtab)
+static void dedotify(Elf64_Sym *syms, unsigned int numsyms, const char *strtab)
{
unsigned int i;
for (i = 1; i < numsyms; i++) {
- if (syms[i].st_shndx == SHN_UNDEF) {
- char *name = strtab + syms[i].st_name;
- if (name[0] == '.')
- memmove(name, name+1, strlen(name));
- }
+ if (syms[i].st_shndx == SHN_UNDEF
+ && strtab[syms[i].st_name] == '.')
+ syms[i].st_name++;
}
}
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
