Re: [Qemu-devel] [PATCH] Support running QEMU on Valgrind

[Alexander Graf]

On 30.10.2011, at 15:30, Stefan Weil wrote:

> Am 30.10.2011 14:41, schrieb Alexander Graf:
>> On 30.10.2011, at 13:07, Stefan Weil wrote:
>>> Valgrind is a tool which can automatically detect many kinds of bugs.
>>> 
>>> Running QEMU on Valgrind with x86_64 hosts was not possible because
>>> Valgrind aborts when memalign is called with an alignment larger than
>>> 1 MiB. QEMU normally uses 2 MiB on Linux x86_64.
>>> 
>>> Now the alignment is reduced to the page size when QEMU is running on
>>> Valgrind.
>>> 
>>> valgrind.h is a copy from Valgrind svn trunk r12226 with trailing
>>> whitespace stripped but otherwise unmodified, so it still raises lots
>>> of errors when checked with scripts/checkpatch.pl.
>> 
>> Can't we just require valgrind header files to be around when kvm is 
>> enabled? I would rather not copy code from other projects. Alternatively you 
>> could take the header and shrink it down to maybe 5 lines of inline asm code 
>> that would be a lot more readable :). You're #ifdef'ing on x86_64 already 
>> anyways.
> 
> The patch is currently required for x86_64 hosts running Linux.
> I estimate that this is one of the most frequently used QEMU host platforms,
> and in most cases, KVM will be configured because this is the default
> and also because it is reasonable for this platform.
> 
> How many of these hosts will have the Valgrind header around?
> I estimate less than 20 %, so configure would have to test whether
> valgrind.h is available or not. I think providing valgrind.h is
> a much better (and simpler) solution.

Hrm. I see your point.

> Stripping valgrind.h is not a good idea: the file is specially designed
> to be included in other projects like QEMU. As soon as the 2 MiB alignment
> is used for other hosts (ppc64, ...), you would have to take more and more
> from the original code. The file was not designed to be readable.
> Although it contains lots of comments which improve readability,
> there remains code which is less easy to read. I cite one of those
> comments:
> 
> /* The following defines the magic code sequences which the JITter
>   spots and handles magically.  Don't look too closely at them as
>   they will rot your brain.
> 
> Instead of rotting my brain, I prefer using a copy of the original code.

Could we maybe use a git submodule to point to the valgrind repo and fetch it 
from there?

> 
>> 
>>> 
>>> It is included here to avoid a dependency on Valgrind.
>>> 
>>> Signed-off-by: Stefan Weil <address@hidden>
>>> ---
>>> oslib-posix.c | 8 +-
>>> valgrind.h | 4060 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 4066 insertions(+), 2 deletions(-)
>>> create mode 100644 valgrind.h
>>> 
>>> diff --git a/oslib-posix.c b/oslib-posix.c
>>> index dbc8ee8..e503e58 100644
>>> --- a/oslib-posix.c
>>> +++ b/oslib-posix.c
>>> @@ -36,10 +36,14 @@ extern int daemon(int, int);
>>> #endif
>>> 
>>> #if defined(__linux__) && defined(__x86_64__)
>>> - /* Use 2MB alignment so transparent hugepages can be used by KVM */
>>> + /* Use 2 MiB alignment so transparent hugepages can be used by KVM.
>>> + Valgrind does not support alignments larger than 1 MiB,
>>> + therefore we need special code which handles running on Valgrind. */
>>> # define QEMU_VMALLOC_ALIGN (512 * 4096)
>>> +# include "valgrind.h" /* RUNNING_ON_VALGRIND */
>> 
>> I would prefer to just have a global variable we keep the alignment in that 
>> gets populated on initialization. That way we don't have to query valgrind 
>> or potentially query the kernel on every memalign and keep everything on a 
>> single spot.
> 
> As long as this is the only use of a Valgrind interface,
> I don't know which other single spot would be better.
> For a short time I thought about adding something to
> qemu-common.h, but I don't think that would be a good idea.

Oh, I'm not talking about the #include. What I meant was the QEMU_MALLOC_ALIGN 
constant. I don't see why that should be a define. Why not make it:

size_t vmalloc_align;

void some_init_func() {
    vmalloc_align = getpagesize();

#if defined(__linux__) && defined(__x86_64__)
    /* comment about 2MB page size, THP and valgrind */
    if (!RUNNING_ON_VALGRIND) {
        vmalloc_align = (512 * 4096);
    }
#endif
}

Then this piece of code would be the only one that cares about the 
initialization of the alignment. During runtime, every user (admittedly, there 
is only one, but it still is nicer separation) of the alignment constant would 
just use the global and be done. No checks.

> 
> memalign is not time critical. The extra code added by RUNNING_ON_VALGRIND is 
> small
> (a few machine instructions, see comment in valgrind.h), and it is only 
> executed when
> a large memory block is allocated. How many calls of qemu_vmalloc with at 
> least 2 MiB
> do you expect?

I'm not thinking of time critical, but complexity. It's a lot easier to read 
qemu_vmalloc if we don't have valgrind special cases in there.

> 
> There remains an issue with my patch which I don't like:
> I dislike the large number of files in QEMU's root directory
> and that I was going to add one more. That's something
> which should be addressed in a different patch as soon
> as there is a consensus that many files in a directory
> are a problem and which project directory structure would
> be better.

We already do have a subdirectory for linux headers. So I'd say either just add 
this header into a directory destined for external headers or poke at the git 
submodule approach. The former is probably easier :). And I don't think we need 
a huge amount of consensus about the name of that directory. Just call it 
something.


Alex