| Message ID | 20230209024022.3371768-2-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
The subject doesn't match what the patch does; intercepting CR4.LAM_SUP isn't done by this patch. How about: Virtualize CR4.LAM_SUP and in the changelog, explain a bit why CR4.LAM_SUP isn't pass-thru'd and why no change to kvm/vmx_set_cr4() is needed. On Thu, Feb 09, 2023 at 10:40:14AM +0800, Robert Hoo wrote: >Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while reserve s/(bit 28)// >it in __cr4_reserved_bits() by feature testing. > >Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> >Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
On Thu, 2023-02-09 at 17:21 +0800, Chao Gao wrote: > The subject doesn't match what the patch does; intercepting > CR4.LAM_SUP isn't done by this patch. How about: > > Virtualize CR4.LAM_SUP All right, although I think this patch is all about intercepting CR4.LAM_SUP. Additional handling on CR4 bits intercepting in kvm/vmx_set_cr4() isn't always necessary. > > and in the changelog, Do you mean in cover letter? or in this patch's description here? > explain a bit why CR4.LAM_SUP isn't > pass-thru'd and why no change to kvm/vmx_set_cr4() is needed. OK. Existing kvm/vmx_set_cr4() can handle CR4.LAM_SUP well, no additional code need to be added. If we take a look at kvm/vmx_set_cr4() body, besides the ultimate goal of vmcs_writel(CR4_READ_SHADOW, cr4); vmcs_writel(GUEST_CR4, hw_cr4); other hunks are about constructing/adjust cr4/hw_cr4. Those are for the CR4 bits that has dependency on other features/system status (e.g. paging), while CR4.LAM_SUP doesn't. > > On Thu, Feb 09, 2023 at 10:40:14AM +0800, Robert Hoo wrote: > > Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while > > reserve > > s/(bit 28)// > > > it in __cr4_reserved_bits() by feature testing. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
On 2/9/2023 10:40 AM, Robert Hoo wrote: > Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while reserve > it in __cr4_reserved_bits() by feature testing. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com> As Sean pointed out in[*], this Reviewed-by is for other purpose, please remove all of them in this series. [*] Re: [PATCH 7/7] x86/kvm: Expose LASS feature to VM guest - Sean Christopherson (kernel.org) <https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com/><https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com/> [...]
On Fri, 2023-02-10 at 11:29 +0800, Yang, Weijiang wrote: > On 2/9/2023 10:40 AM, Robert Hoo wrote: > > Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while > > reserve > > it in __cr4_reserved_bits() by feature testing. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com> > > As Sean pointed out in[*], this Reviewed-by is for other purpose, > please > remove all of > > them in this series. No. Sean meant another thing. This reviewed-by is from Jingqi as usual. But for this patch specific, I think I can drop Jingqi's reviewed-by, as this patch changed fundamentally from last version. > > [*] Re: [PATCH 7/7] x86/kvm: Expose LASS feature to VM guest - Sean > Christopherson (kernel.org) > < > https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com/><https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com/ > > > > [...] >
On Fri, Feb 10, 2023, Robert Hoo wrote: > On Fri, 2023-02-10 at 11:29 +0800, Yang, Weijiang wrote: > > On 2/9/2023 10:40 AM, Robert Hoo wrote: > > > Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while > > > reserve > > > it in __cr4_reserved_bits() by feature testing. > > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com> > > > > As Sean pointed out in[*], this Reviewed-by is for other purpose, > > please > > remove all of > > > > them in this series. > > No. Sean meant another thing. Correct, what I object to is Intel _requiring_ a Reviewed-by before posting. And while I'm certainly not going to refuse patches that have been reviewed internally, I _strongly_ prefer reviews be on-list so that they are public and recorded. Being able to go back and look at the history and evolution of patches is valuable, and the discussion itself is often beneficial to non-participants, e.g. people that are new-ish to KVM and/or aren't familiar with the feature being enabled can often learn new things and avoid similar pitfalls of their own. Rather than spend cycles getting through internal review, I would much prefer developers spend their time writing tests and validating their code before posting. Obviously there's a risk that foregoing internal review will result in low quality submissions, but I think the LASS series proves that mandatory reviews doesn't necessarily help on that front. On the other hand, writing and running tests naturally enforces a minimum level of quality. I am happy to help with changelogs, comments, naming, etc. E.g. I don't get frustrated when someone who is new to kernel development or for whom English is not their first language writes an imperfect changelog. I get frustrated when there's seemingly no attempt to justify _why_ a patch/KVM does something, and I get really grumpy when blatantly buggy code is posted with no tests.
This patch removes CR4.LAM_SUP from cr4_reserved_bits to allows the setting of X86_CR4_LAM_SUP by guest if the hardware platform supports the feature. The interception of CR4 is decided by CR4 guest/host mask and CR4 read shadow. The patch tiltle is not accurate. On 2/9/2023 10:40 AM, Robert Hoo wrote: > Remove CR4.LAM_SUP (bit 28) from default CR4_RESERVED_BITS, while reserve > it in __cr4_reserved_bits() by feature testing. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 3 ++- > arch/x86/kvm/x86.h | 2 ++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index f35f1ff4427b..4684896698f4 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -125,7 +125,8 @@ > | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \ > | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \ > | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \ > - | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP)) > + | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \ > + | X86_CR4_LAM_SUP)) > > #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR) > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > index 9de72586f406..8ec5cc983062 100644 > --- a/arch/x86/kvm/x86.h > +++ b/arch/x86/kvm/x86.h > @@ -475,6 +475,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type); > __reserved_bits |= X86_CR4_VMXE; \ > if (!__cpu_has(__c, X86_FEATURE_PCID)) \ > __reserved_bits |= X86_CR4_PCIDE; \ > + if (!__cpu_has(__c, X86_FEATURE_LAM)) \ > + __reserved_bits |= X86_CR4_LAM_SUP; \ > __reserved_bits; \ > }) >
On Tue, 2023-02-14 at 09:27 +0800, Binbin Wu wrote: > This patch removes CR4.LAM_SUP from cr4_reserved_bits to allows the > setting of X86_CR4_LAM_SUP by guest Yes > if the hardware platform supports > the feature. More precisely, if guest_cpuid_has() LAM feature. QEMU could turn feature off even if underlying host/KVM tells supporting it. > > The interception of CR4 is decided by CR4 guest/host mask and CR4 > read > shadow. > My interpretation is that "intercept CR4.x bit" is the opposite of "guest own CR4.x bit". Both of them are implemented via CR4 guest/host mask and CR4 shadow, whose combination decides corresponding CR4.x bit access causes VM exit or not. When we changes some bits in CR4_RESERVED_BITS and __cr4_reserved_bits(), we changes vcpu->arch.cr4_guest_owned_bits which eventually forms the effective vmcs_writel(CR4_GUEST_HOST_MASK).
On 2/14/2023 2:11 PM, Robert Hoo wrote: > On Tue, 2023-02-14 at 09:27 +0800, Binbin Wu wrote: >> The interception of CR4 is decided by CR4 guest/host mask and CR4 >> read >> shadow. >> > My interpretation is that "intercept CR4.x bit" is the opposite of > "guest own CR4.x bit". > Both of them are implemented via CR4 guest/host mask and CR4 shadow, > whose combination decides corresponding CR4.x bit access causes VM exit > or not. > When we changes some bits in CR4_RESERVED_BITS and > __cr4_reserved_bits(), we changes vcpu->arch.cr4_guest_owned_bits which > eventually forms the effective vmcs_writel(CR4_GUEST_HOST_MASK). > According to the code of set_cr4_guest_host_mask, vcpu->arch.cr4_guest_owned_bits is a subset of KVM_POSSIBLE_CR4_GUEST_BITS, and X86_CR4_LAM_SUP is not included in KVM_POSSIBLE_CR4_GUEST_BITS. No matter change CR4_RESERVED_BITS or not, X86_CR4_LAM_SUP will always be set in CR4_GUEST_HOST_MASK.
On Tue, 2023-02-14 at 17:00 +0800, Binbin Wu wrote: > According to the code of set_cr4_guest_host_mask, > vcpu->arch.cr4_guest_owned_bits is a subset of > KVM_POSSIBLE_CR4_GUEST_BITS, > and X86_CR4_LAM_SUP is not included in KVM_POSSIBLE_CR4_GUEST_BITS. > No matter change CR4_RESERVED_BITS or not, X86_CR4_LAM_SUP will > always be set in CR4_GUEST_HOST_MASK. > > set_cr4_guest_host_mask(): vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS & ~vcpu->arch.cr4_guest_rsvd_bits; kvm_vcpu_after_set_cpuid(): vcpu->arch.cr4_guest_rsvd_bits = __cr4_reserved_bits(guest_cpuid_has, vcpu);
On Tue, 2023-02-14 at 20:24 +0800, Robert Hoo wrote: > On Tue, 2023-02-14 at 17:00 +0800, Binbin Wu wrote: > > According to the code of set_cr4_guest_host_mask, > > vcpu->arch.cr4_guest_owned_bits is a subset of > > KVM_POSSIBLE_CR4_GUEST_BITS, > > and X86_CR4_LAM_SUP is not included in KVM_POSSIBLE_CR4_GUEST_BITS. > > No matter change CR4_RESERVED_BITS or not, X86_CR4_LAM_SUP will > > always be set in CR4_GUEST_HOST_MASK. yes, bits set to 1 in a guest/host mask means it's owned by the host. > > > > > > set_cr4_guest_host_mask(): > vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS & > ~vcpu->arch.cr4_guest_rsvd_bits; > > kvm_vcpu_after_set_cpuid(): > vcpu->arch.cr4_guest_rsvd_bits = > __cr4_reserved_bits(guest_cpuid_has, vcpu);
On 2/14/2023 8:24 PM, Robert Hoo wrote: > On Tue, 2023-02-14 at 17:00 +0800, Binbin Wu wrote: >> According to the code of set_cr4_guest_host_mask, >> vcpu->arch.cr4_guest_owned_bits is a subset of >> KVM_POSSIBLE_CR4_GUEST_BITS, >> and X86_CR4_LAM_SUP is not included in KVM_POSSIBLE_CR4_GUEST_BITS. >> No matter change CR4_RESERVED_BITS or not, X86_CR4_LAM_SUP will >> always be set in CR4_GUEST_HOST_MASK. >> >> > set_cr4_guest_host_mask(): > vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS & > ~vcpu->arch.cr4_guest_rsvd_bits; My point is when X86_CR4_LAM_SUP is not set in KVM_POSSIBLE_CR4_GUEST_BITS, CR4.LAM_SUP is definitely owned by host, regardless of the value of cr4_guest_rsvd_bits. > > kvm_vcpu_after_set_cpuid(): > vcpu->arch.cr4_guest_rsvd_bits = > __cr4_reserved_bits(guest_cpuid_has, vcpu); >
On Thu, 2023-02-16 at 13:31 +0800, Binbin Wu wrote: > On 2/14/2023 8:24 PM, Robert Hoo wrote: > > On Tue, 2023-02-14 at 17:00 +0800, Binbin Wu wrote: > > > According to the code of set_cr4_guest_host_mask, > > > vcpu->arch.cr4_guest_owned_bits is a subset of > > > KVM_POSSIBLE_CR4_GUEST_BITS, > > > and X86_CR4_LAM_SUP is not included in > > > KVM_POSSIBLE_CR4_GUEST_BITS. > > > No matter change CR4_RESERVED_BITS or not, X86_CR4_LAM_SUP will > > > always be set in CR4_GUEST_HOST_MASK. > > > > > > > > > > set_cr4_guest_host_mask(): > > vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS & > > ~vcpu->arch.cr4_guest_rsvd_bits; > > My point is when X86_CR4_LAM_SUP is not set in > KVM_POSSIBLE_CR4_GUEST_BITS, > CR4.LAM_SUP is definitely owned by host, regardless of the value of > cr4_guest_rsvd_bits. > Yes, you can disregard that reply. We were talking each's own points:) Neither is wrong. Chao talked to me afterwards, that your points are: we can say by default, without this patch, CR4.LAM_SUP were intercepted. so why redundantly name this patch "Intercept CR4.LAM_SUP". That's true, but intercepted as reserved bit. I'm revising the subject in v5. > > > > > kvm_vcpu_after_set_cpuid(): > > vcpu->arch.cr4_guest_rsvd_bits = > > __cr4_reserved_bits(guest_cpuid_has, vcpu); > >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index f35f1ff4427b..4684896698f4 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -125,7 +125,8 @@ | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \ | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \ | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \ - | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP)) + | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \ + | X86_CR4_LAM_SUP)) #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR) diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index 9de72586f406..8ec5cc983062 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -475,6 +475,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type); __reserved_bits |= X86_CR4_VMXE; \ if (!__cpu_has(__c, X86_FEATURE_PCID)) \ __reserved_bits |= X86_CR4_PCIDE; \ + if (!__cpu_has(__c, X86_FEATURE_LAM)) \ + __reserved_bits |= X86_CR4_LAM_SUP; \ __reserved_bits; \ })