| Message ID | 20230914063325.85503-3-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Fix guest xsave area allocation size from fpu_user_cfg.default_size > to > fpu_kernel_cfg.default_size so that the xsave area size is consistent > with fpstate->size set in __fpstate_reset(). > > With the fix, guest fpstate size is sufficient for KVM supported > guest > xfeatures. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> There is no fix (Fixes: ...) here, right? I think this change is needed to make sure KVM guests can support supervisor features. But KVM CET support (to follow in future patches) will be the first one, right? The side effect will be that KVM guest FPUs now get guaranteed room for PASID as well as CET. I think I remember you mentioned that due to alignment requirements, there shouldn't usually be any size change though? It might be nice to add that in the log, if I'm remembering correctly.
On 9/15/2023 6:45 AM, Edgecombe, Rick P wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >> Fix guest xsave area allocation size from fpu_user_cfg.default_size >> to >> fpu_kernel_cfg.default_size so that the xsave area size is consistent >> with fpstate->size set in __fpstate_reset(). >> >> With the fix, guest fpstate size is sufficient for KVM supported >> guest >> xfeatures. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > There is no fix (Fixes: ...) here, right? Ooh, I got it lost during rebase, thanks! > I think this change is needed > to make sure KVM guests can support supervisor features. But KVM CET > support (to follow in future patches) will be the first one, right? Exactly, the existing code takes only user xfeatures into account, and we have more and more CPU features rely on supervisor xstates. > The side effect will be that KVM guest FPUs now get guaranteed room for > PASID as well as CET. I think I remember you mentioned that due to > alignment requirements, there shouldn't usually be any size change > though? Yes, IIUC the precondition is AMX feature is enabled for guest so that the CET supervisor state actually resides in the gap to next 64byte aligned address, so no actual size expansion. > It might be nice to add that in the log, if I'm remembering > correctly. Sure, thanks!
On Fri, 2023-09-15 at 10:45 +0800, Yang, Weijiang wrote: > On 9/15/2023 6:45 AM, Edgecombe, Rick P wrote: > > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > > > Fix guest xsave area allocation size from > > > fpu_user_cfg.default_size > > > to > > > fpu_kernel_cfg.default_size so that the xsave area size is > > > consistent > > > with fpstate->size set in __fpstate_reset(). > > > > > > With the fix, guest fpstate size is sufficient for KVM supported > > > guest > > > xfeatures. > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > There is no fix (Fixes: ...) here, right? > > Ooh, I got it lost during rebase, thanks! > > > I think this change is needed > > to make sure KVM guests can support supervisor features. But KVM > > CET > > support (to follow in future patches) will be the first one, right? > > Exactly, the existing code takes only user xfeatures into account, > and we have more > and more CPU features rely on supervisor xstates. If KVM is not using any supervisor features, then pre CET KVM support I think the current code is more correct.
On Thu, Sep 14, 2023, Yang Weijiang wrote: > Fix guest xsave area allocation size from fpu_user_cfg.default_size to > fpu_kernel_cfg.default_size so that the xsave area size is consistent > with fpstate->size set in __fpstate_reset(). > > With the fix, guest fpstate size is sufficient for KVM supported guest > xfeatures. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kernel/fpu/core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > index a86d37052a64..a42d8ad26ce6 100644 > --- a/arch/x86/kernel/fpu/core.c > +++ b/arch/x86/kernel/fpu/core.c > @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) > struct fpstate *fpstate; > unsigned int size; > > - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); > + size = fpu_kernel_cfg.default_size + > + ALIGN(offsetof(struct fpstate, regs), 64); Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg? At the very least, this looks wrong when paired with the above: gfpu->uabi_size = sizeof(struct kvm_xsave); if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size)) gfpu->uabi_size = fpu_user_cfg.default_size;
On 10/21/2023 8:39 AM, Sean Christopherson wrote: > On Thu, Sep 14, 2023, Yang Weijiang wrote: >> Fix guest xsave area allocation size from fpu_user_cfg.default_size to >> fpu_kernel_cfg.default_size so that the xsave area size is consistent >> with fpstate->size set in __fpstate_reset(). >> >> With the fix, guest fpstate size is sufficient for KVM supported guest >> xfeatures. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> --- >> arch/x86/kernel/fpu/core.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c >> index a86d37052a64..a42d8ad26ce6 100644 >> --- a/arch/x86/kernel/fpu/core.c >> +++ b/arch/x86/kernel/fpu/core.c >> @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) >> struct fpstate *fpstate; >> unsigned int size; >> >> - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); >> + size = fpu_kernel_cfg.default_size + >> + ALIGN(offsetof(struct fpstate, regs), 64); > Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg? > At the very least, this looks wrong when paired with the above: > > gfpu->uabi_size = sizeof(struct kvm_xsave); > if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size)) > gfpu->uabi_size = fpu_user_cfg.default_size; Hi, Sean, Not sure what's your concerns. From my understanding fpu_kernel_cfg.default_size should include all enabled xfeatures in host (XCR0 | XSS), this is also expected for supporting all guest enabled xfeatures. gfpu->uabi_size only includes enabled user xfeatures which are operated via KVM uABIs(KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2), so the two sizes are relatively independent since guest supervisor xfeatures are saved/restored via GET/SET_MSRS interfaces.
On Tue, Oct 24, 2023, Weijiang Yang wrote: > On 10/21/2023 8:39 AM, Sean Christopherson wrote: > > On Thu, Sep 14, 2023, Yang Weijiang wrote: > > > Fix guest xsave area allocation size from fpu_user_cfg.default_size to > > > fpu_kernel_cfg.default_size so that the xsave area size is consistent > > > with fpstate->size set in __fpstate_reset(). > > > > > > With the fix, guest fpstate size is sufficient for KVM supported guest > > > xfeatures. > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > --- > > > arch/x86/kernel/fpu/core.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > > > index a86d37052a64..a42d8ad26ce6 100644 > > > --- a/arch/x86/kernel/fpu/core.c > > > +++ b/arch/x86/kernel/fpu/core.c > > > @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) > > > struct fpstate *fpstate; > > > unsigned int size; > > > - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); > > > + size = fpu_kernel_cfg.default_size + > > > + ALIGN(offsetof(struct fpstate, regs), 64); > > Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg? > > At the very least, this looks wrong when paired with the above: > > > > gfpu->uabi_size = sizeof(struct kvm_xsave); > > if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size)) > > gfpu->uabi_size = fpu_user_cfg.default_size; > > Hi, Sean, > Not sure what's your concerns. > From my understanding fpu_kernel_cfg.default_size should include all enabled > xfeatures in host (XCR0 | XSS), this is also expected for supporting all > guest enabled xfeatures. gfpu->uabi_size only includes enabled user xfeatures > which are operated via KVM uABIs(KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2), > so the two sizes are relatively independent since guest supervisor xfeatures > are saved/restored via GET/SET_MSRS interfaces. Ah, right, I keep forgetting that KVM's ABI can't use XRSTOR because it forces the compacted format. This part still looks odd to me: gfpu->xfeatures = fpu_user_cfg.default_features; gfpu->perm = fpu_user_cfg.default_features; but I'm probably just not understanding something in the other patches changes yet.
On 10/25/2023 12:32 AM, Sean Christopherson wrote: > On Tue, Oct 24, 2023, Weijiang Yang wrote: >> On 10/21/2023 8:39 AM, Sean Christopherson wrote: >>> On Thu, Sep 14, 2023, Yang Weijiang wrote: >>>> Fix guest xsave area allocation size from fpu_user_cfg.default_size to >>>> fpu_kernel_cfg.default_size so that the xsave area size is consistent >>>> with fpstate->size set in __fpstate_reset(). >>>> >>>> With the fix, guest fpstate size is sufficient for KVM supported guest >>>> xfeatures. >>>> >>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >>>> --- >>>> arch/x86/kernel/fpu/core.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c >>>> index a86d37052a64..a42d8ad26ce6 100644 >>>> --- a/arch/x86/kernel/fpu/core.c >>>> +++ b/arch/x86/kernel/fpu/core.c >>>> @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) >>>> struct fpstate *fpstate; >>>> unsigned int size; >>>> - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); >>>> + size = fpu_kernel_cfg.default_size + >>>> + ALIGN(offsetof(struct fpstate, regs), 64); >>> Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg? >>> At the very least, this looks wrong when paired with the above: >>> >>> gfpu->uabi_size = sizeof(struct kvm_xsave); >>> if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size)) >>> gfpu->uabi_size = fpu_user_cfg.default_size; >> Hi, Sean, >> Not sure what's your concerns. >> From my understanding fpu_kernel_cfg.default_size should include all enabled >> xfeatures in host (XCR0 | XSS), this is also expected for supporting all >> guest enabled xfeatures. gfpu->uabi_size only includes enabled user xfeatures >> which are operated via KVM uABIs(KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2), >> so the two sizes are relatively independent since guest supervisor xfeatures >> are saved/restored via GET/SET_MSRS interfaces. > Ah, right, I keep forgetting that KVM's ABI can't use XRSTOR because it forces > the compacted format. > > This part still looks odd to me: > > gfpu->xfeatures = fpu_user_cfg.default_features; > gfpu->perm = fpu_user_cfg.default_features; I guess when the kernel FPU code was overhauled, the supervisor xstates were not taken into account for guest supported xfeaures, so the first line looks reasonable until supervisor xfeatures are landing. And for the second line, per current design, the user mode can only control user xfeatures via arch_prctl() kernel uAPI, so it also makes sense to initialize perm with fpu_user_cfg.default_features too. But in this CET KVM series I'd like to expand the former to support all guest enabled xfeatures, i.e., both user and supervisor xfeaures, and keep the latter as-is since there seems no reason to allow userspace to alter supervisor xfeatures. > but I'm probably just not understanding something in the other patches changes yet.
On Tue, 2023-10-24 at 09:32 -0700, Sean Christopherson wrote: > On Tue, Oct 24, 2023, Weijiang Yang wrote: > > On 10/21/2023 8:39 AM, Sean Christopherson wrote: > > > On Thu, Sep 14, 2023, Yang Weijiang wrote: > > > > Fix guest xsave area allocation size from fpu_user_cfg.default_size to > > > > fpu_kernel_cfg.default_size so that the xsave area size is consistent > > > > with fpstate->size set in __fpstate_reset(). > > > > > > > > With the fix, guest fpstate size is sufficient for KVM supported guest > > > > xfeatures. > > > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > > --- > > > > arch/x86/kernel/fpu/core.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > > > > index a86d37052a64..a42d8ad26ce6 100644 > > > > --- a/arch/x86/kernel/fpu/core.c > > > > +++ b/arch/x86/kernel/fpu/core.c > > > > @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) > > > > struct fpstate *fpstate; > > > > unsigned int size; > > > > - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); > > > > + size = fpu_kernel_cfg.default_size + > > > > + ALIGN(offsetof(struct fpstate, regs), 64); > > > Shouldn't all the other calculations in this function also switch to fpu_kernel_cfg? > > > At the very least, this looks wrong when paired with the above: > > > > > > gfpu->uabi_size = sizeof(struct kvm_xsave); > > > if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size)) > > > gfpu->uabi_size = fpu_user_cfg.default_size; > > > > Hi, Sean, > > Not sure what's your concerns. > > From my understanding fpu_kernel_cfg.default_size should include all enabled > > xfeatures in host (XCR0 | XSS), this is also expected for supporting all > > guest enabled xfeatures. gfpu->uabi_size only includes enabled user xfeatures > > which are operated via KVM uABIs(KVM_GET_XSAVE/KVM_SET_XSAVE/KVM_GET_XSAVE2), > > so the two sizes are relatively independent since guest supervisor xfeatures > > are saved/restored via GET/SET_MSRS interfaces. > > Ah, right, I keep forgetting that KVM's ABI can't use XRSTOR because it forces > the compacted format. > > This part still looks odd to me: > > gfpu->xfeatures = fpu_user_cfg.default_features; That should be indeed fpu_kernel_cfg.default_features. This variable is also currently hardly used, it only tracks which dynamic userspace features are enabled and KVM only uses it once (in fpu_enable_guest_xfd_features) > gfpu->perm = fpu_user_cfg.default_features; This variable I think is currently only set and never read. Note that current->group_leader->thread.fpu.guest_perm is actually initialized to fpu_kernel_cfg.default_features but the kernel components of it masked in the corresponding prctl (ARCH_GET_XCOMP_SUPP/ARCH_GET_XCOMP_GUEST_PERM/ARCH_REQ_XCOMP_GUEST_PERM). So I think that we also should use fpu_kernel_cfg.default_features here for the sake of not having uninitilized variable on 32 bit kernels, because the whole FPU permission thing I see is implemented for 64 bit kernels only. Or even better IMHO is to remove both variables and in fpu_enable_guest_xfd_features, just mask the xfeatures with the XFEATURE_MASK_USER_DYNAMIC instead. Best regards, Maxim Levitsky > > but I'm probably just not understanding something in the other patches changes yet. >
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index a86d37052a64..a42d8ad26ce6 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) struct fpstate *fpstate; unsigned int size; - size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); + size = fpu_kernel_cfg.default_size + + ALIGN(offsetof(struct fpstate, regs), 64); + fpstate = vzalloc(size); if (!fpstate) return false;
Fix guest xsave area allocation size from fpu_user_cfg.default_size to fpu_kernel_cfg.default_size so that the xsave area size is consistent with fpstate->size set in __fpstate_reset(). With the fix, guest fpstate size is sufficient for KVM supported guest xfeatures. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kernel/fpu/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)