Re: [Qemu-ppc] [Qemu-devel] [PATCH] ppc: Yet another fix for the huge page support detection mechanism

[Greg Kurz]

On Mon, 18 Jul 2016 11:21:41 +0200
Thomas Huth <address@hidden> wrote:

> On 15.07.2016 11:28, Greg Kurz wrote:
> > On Fri, 15 Jul 2016 10:10:25 +0200
> > Thomas Huth <address@hidden> wrote:
> >   
> >> Commit 86b50f2e1bef ("Disable huge page support if it is not available
> >> for main RAM") already made sure that huge page support is not announced
> >> to the guest if the normal RAM of non-NUMA configurations is not backed
> >> by a huge page filesystem. However, there is one more case that can go
> >> wrong: NUMA is enabled, but the RAM of the NUMA nodes are not configured
> >> with huge page support (and only the memory of a DIMM is configured with
> >> it). When QEMU is started with the following command line for example,
> >> the Linux guest currently crashes because it is trying to use huge pages
> >> on a memory region that does not support huge pages:
> >>
> >>  qemu-system-ppc64 -enable-kvm ... -m 1G,slots=4,maxmem=32G -object \
> >>    
> >> memory-backend-file,policy=default,mem-path=/hugepages,size=1G,id=mem-mem1 
> >> \
> >>    -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
> >>    -numa node,nodeid=0 -numa node,nodeid=1
> >>
> >> To fix this issue, we've got to make sure to disable huge page support,
> >> too, when there is a NUMA node that is not using a memory backend with
> >> huge page support.
> >>
> >> Fixes: 86b50f2e1befc33407bdfeb6f45f7b0d2439a740  
> > 
> > According to http://patchwork.ozlabs.org/patch/584741/ , it is best worded
> > 
> > "Broken in commit 86b50f2e1bef"  
> 
> Using the "Fixes:" syntax is a well-known practise with the Linux kernel
> (see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=HEAD#n187),
> so I don't see the point why we should introduce another syntax for QEMU
> here. And if we do, it should be documented on
> http://qemu-project.org/Contribute/SubmitAPatch at least.
> 
>  Thomas
> 

I had used this syntax in the first place at the time, but Markus's and
David's comments lured me into believing there was a consensus against
it.

FWIW, I personally prefer the Linux kernel "Fixes:" syntax :)

Cheers.

--
Greg