On Mon, Jan 09, 2023 at 06:51:27PM -0500, Dave Voutila wrote:
>
> This ok with folks? Had OK's for the original diff but double checking
> before I commit.
>
This is only half of what you need to do to stop guests from using
unwanted/unsupported instructions. Removing the CPUID feature flag bit
only lets the guest know "don't use this instruction" but if they ignore
that and use it anyway, well... shrug.
If you want to remove support for WAITPKG, we'll need to enable
TPAUSE/UMWAIT exiting, and then inject #UD if we ever exit for that
reason. I think there are a few examples of this in vmm.c already.
Other than that, this diff can go in but we need to do the other part
too.
-ml
> Dave Voutila <d...@sisu.io> writes:
>
> > Philip Guenther <guent...@gmail.com> writes:
> >
> >> On Sat, Jan 7, 2023 at 11:04 AM Dave Voutila <d...@sisu.io> wrote:
> >>
> >> Bringing this to tech@ to increase my chance of someone testing my
> >> diff.
> >>
> >> As reported in this thread on misc@ [1], I believe newer Intel hardware
> >> may be experiencing issues hosting Linux guests under vmm/vmd. It looks
> >> like there are some newer instructions Intel added (TPAUSE specifically)
> >> that also involve some new MSR(s).
> >>
> >> I don't have 12th gen Intel hardware to test this on (I think that's
> >> Alder Lake). I'd like to mask this feature from vmm guests since it's
> >> related to an MSR we don't yet pass through or emulate and has to do
> >> with the TSC (which has it's own challenges in vmm).
> >>
> >> For someone testing, you should be able to grab an Alpine Linux iso
> >> (-virt flavor) and boot it with vmd with the diff. (Without it should
> >> "hang" and spike CPU or just die.) Also check that WAITPKG shows up in
> >> your dmesg on the cpu feature output.
> >>
> >> This seem like it'll obviously work, but I guess it seems to me that this
> >> "opt-out" approach is generally
> >> unsafe/unstable and vmd should consider actively switching to "opt-in" on
> >> all these CPUID feature bits. I mean,
> >> what bits are defined in the SEFF first-leaf EDX that _do_ work with vmd?
> >>
> >
> > Great point (I think you mean ECX). Here's an updated diff that flips it
> > to a whitelist so Intel/AMD don't burn me with these new bits in the
> > future. This better?
> >
> >
> > diff refs/heads/master refs/heads/vmm-tsleep
> > commit - bfce157fda90a812e1a99aa179a4c42f12ebfa24
> > commit + 5b434c89250e1901340c11c8f9c380dc18d0ae91
> > blob - 001a437045be145322be30288c1f47d63fb07634
> > blob + 0bd908e273a1c0e6324e1bc9f8c8ca921555c86f
> > --- sys/arch/amd64/amd64/identcpu.c
> > +++ sys/arch/amd64/amd64/identcpu.c
> > @@ -208,6 +208,7 @@ const struct {
> > { SEFF0ECX_AVX512VBMI, "AVX512VBMI" },
> > { SEFF0ECX_UMIP, "UMIP" },
> > { SEFF0ECX_PKU, "PKU" },
> > + { SEFF0ECX_WAITPKG, "WAITPKG" },
> > }, cpu_seff0_edxfeatures[] = {
> > { SEFF0EDX_AVX512_4FNNIW, "AVX512FNNIW" },
> > { SEFF0EDX_AVX512_4FMAPS, "AVX512FMAPS" },
> > blob - cbde6cf9b02fc882a8ed17aa6adb5c43249e0302
> > blob + b26bd32e2d9ea7386b1f58960dea40b787d6a341
> > --- sys/arch/amd64/include/specialreg.h
> > +++ sys/arch/amd64/include/specialreg.h
> > @@ -201,6 +201,7 @@
> > #define SEFF0ECX_AVX512VBMI 0x00000002 /* AVX-512 vector bit inst */
> > #define SEFF0ECX_UMIP 0x00000004 /* UMIP support */
> > #define SEFF0ECX_PKU 0x00000008 /* Page prot keys for user
> > mode */
> > +#define SEFF0ECX_WAITPKG 0x00000010 /* UMONITOR/UMWAIT/TPAUSE insns */
> > /* SEFF EDX bits */
> > #define SEFF0EDX_AVX512_4FNNIW 0x00000004 /* AVX-512 neural network
> > insns */
> > #define SEFF0EDX_AVX512_4FMAPS 0x00000008 /* AVX-512 mult accum single
> > prec */
> > blob - 6b4802abf4b508495cdbc961bd799d3fa83b9c36
> > blob + 032444b05e19d7fbec96a0d11b5b340f668c0917
> > --- sys/arch/amd64/include/vmmvar.h
> > +++ sys/arch/amd64/include/vmmvar.h
> > @@ -672,8 +672,10 @@ struct vm_mprotect_ept_params {
> > SEFF0EBX_AVX512IFMA | SEFF0EBX_AVX512PF | \
> > SEFF0EBX_AVX512ER | SEFF0EBX_AVX512CD | \
> > SEFF0EBX_AVX512BW | SEFF0EBX_AVX512VL)
> > -#define VMM_SEFF0ECX_MASK ~(SEFF0ECX_AVX512VBMI)
> >
> > +/* ECX mask contains the bits to include */
> > +#define VMM_SEFF0ECX_MASK (SEFF0ECX_PREFETCHWT1 | SEFF0ECX_UMIP |
> > SEFF0ECX_PKU)
> > +
> > /* EDX mask contains the bits to include */
> > #define VMM_SEFF0EDX_MASK (SEFF0EDX_MD_CLEAR)
> >
> > blob - 310208ac4cdb262aaedfa9b78d869fd5911607b2
> > blob + ccf1164fd658a69dc383e1602ae0ce1f269de4e4
> > --- sys/arch/i386/i386/machdep.c
> > +++ sys/arch/i386/i386/machdep.c
> > @@ -1038,6 +1038,7 @@ const struct cpu_cpuid_feature cpu_seff0_ecxfeatures[]
> > { SEFF0ECX_UMIP, "UMIP" },
> > { SEFF0ECX_AVX512VBMI, "AVX512VBMI" },
> > { SEFF0ECX_PKU, "PKU" },
> > + { SEFF0ECX_WAITPKG, "WAITPKG" },
> > };
> >
> > const struct cpu_cpuid_feature cpu_seff0_edxfeatures[] = {
> > blob - 392b4ff412e2dd3c4c48ed6c9c84aa2358721c6a
> > blob + 7ce77ca3fdc6bd1a51571dd0b5dbf5afc311a138
> > --- sys/arch/i386/include/specialreg.h
> > +++ sys/arch/i386/include/specialreg.h
> > @@ -190,6 +190,7 @@
> > #define SEFF0ECX_AVX512VBMI 0x00000002 /* AVX-512 vector bit inst */
> > #define SEFF0ECX_UMIP 0x00000004 /* UMIP support */
> > #define SEFF0ECX_PKU 0x00000008 /* Page prot keys for user
> > mode */
> > +#define SEFF0ECX_WAITPKG 0x00000010 /* UMONITOR/UMWAIT/TPAUSE insns */
> > /* SEFF EDX bits */
> > #define SEFF0EDX_AVX512_4FNNIW 0x00000004 /* AVX-512 neural network
> > insns */
> > #define SEFF0EDX_AVX512_4FMAPS 0x00000008 /* AVX-512 mult accum single
> > prec */
