svn commit: r273214 - in head/sys: amd64/vmm/intel modules/vmm

[John Baldwin]

On Monday, October 27, 2014 11:36:41 AM Warner Losh wrote:
> On Oct 27, 2014, at 10:54 AM, John Baldwin <jhb at freebsd.org> wrote:
> > On Friday, October 17, 2014 01:20:50 PM Warner Losh wrote:
> >> Author: imp
> >> Date: Fri Oct 17 13:20:49 2014
> >> New Revision: 273214
> >> URL: https://svnweb.freebsd.org/changeset/base/273214
> >> 
> >> Log:
> >>  Fix build to not bogusly always rebuild vmm.ko.
> >>  
> >>  Rename vmx_assym.s to vmx_assym.h to reflect that file's actual use
> >>  and update vmx_support.S's include to match. Add vmx_assym.h to the
> >>  SRCS to that it gets properly added to the dependency list. Add
> >>  vmx_support.S to SRCS as well, so it gets built and needs fewer
> >>  special-case goo. Remove now-redundant special-case goo. Finally,
> >>  vmx_genassym.o doesn't need to depend on a hand expanded ${_ILINKS}
> >>  explicitly, that's all taken care of by beforedepend.
> >>  
> >>  With these items fixed, we no longer build vmm.ko every single time
> >>  through the modules on a KERNFAST build.
> > 
> > So I cheered for this before, but it appears to be broken. :(
> > 
> > Namely, I rebuilt world + kernel on my laptop this weekend (it was about a
> > month old).  My normal setup builds kernels with NO_KERNELCLEAN=yes.  On my
> > next reboot when I started a bhyve VM I promptly got a panic due to a page
> > 
> > fault in this assembly code:
> > 	/*
> > 	
> > 	 * If 'vmx->eptgen[curcpu]' is not identical to 'pmap->pm_eptgen'
> > 	 * then we must invalidate all mappings associated with this EPTP.
> > 	 */
> > 	
> > 	movq	PM_EPTGEN(%r11), %r10
> > 	cmpq	%r10, VMX_EPTGEN(%rsi, %rax, 8)
> > 	je	guest_restore
> > 
> > (The 'cmpq' instruction)
> > 
> > This change came to mind, so I blew away the 'vmm' module directory and
> > rebuilt my kernel.  Comparing the assembly of this instruction before and
> > after used different values for VMX_EPTGEN.  In other words, the
> > NO_KERNELCLEAN=yes build failed to regenerate vmx_assym.h and the build
> > used stale values.
> 
> Is there a way to force this condition for testing?

You could checkout an older tree (probably before the recent merge of AMD SVM
support) and build vmm.ko, then svn update and see if vmx_assym and
vmx_support.o are updated.

Actually, this was simpler:

% cd sys/modules/vmm
% make depend
% make vmx_assym.h  # reports nothing to do
% touch machine/vmm.h  # vmx_genassym.c includes this
% make vmx_assym.h  # should rebuild, but doesn't

> > In particular, if you examine the generated .depend file, you will find
> > that there are no dependencies recorded for vmx_genassym.o, so it is
> > never rebuilt if any of the headers it includes are changed.  In my case
> > the panic happened to be one that was easily diagnosed, but I could
> > imagine stale assym headers causing very odd crashes that would be quite
> > hard to track down.  I think these changes should be reverted if we can't
> > fix the dependencies of the associated object files they are generated
> > from. :(
> 
> Give me a bit and I’ll fix it. There’s a number of implicit dependencies
> that don’t get recorded in the .depend file, iirc, so that’s not completely
> conclusive. Not building, though is kinda a big hint that something’s
> amiss.

I think the thing here is that for the assym files we don't record any
dependency info at all.  The main kernel build does record dependencies
for genassym.o in .depend, so it must be doable.

In kern.pre.mk:

GEN_CFILES= $S/$M/$M/genassym.c ${MFILES:T:S/.m$/.c/}

and those are then explicitly passed to mkdep in kern.post.mk.

So this fixes it:

Index: Makefile
===================================================================
--- Makefile	(revision 273555)
+++ Makefile	(working copy)
@@ -4,6 +4,7 @@ KMOD=	vmm
 
 SRCS=	opt_acpi.h opt_ddb.h device_if.h bus_if.h pci_if.h
 SRCS+=	vmx_assym.h svm_assym.h
+DPSRCS=	vmx_genassym.c svm_genassym.c
 
 CFLAGS+= -DVMM_KEEP_STATS -DSMP
 CFLAGS+= -I${.CURDIR}/../../amd64/vmm

I'll try to track down all the other assym files and fix them as well.

> However, -DNO_CLEAN has always been a very-sharp edged tool that will cut
> you in a number of ways, so there’s no rush to back this out.

This is the first time in many years that NO_KERNELCLEAN=yes has been a
problem for me.  (worlds sometimes have issues, but kernels rarely do).
Also, usually when it breaks it fails to compile, it doesn't compile and
then panic. :(

-- 
John Baldwin