On Mon, Aug 11, 2025 at 1:34 PM Sean Christopherson <[email protected]> wrote: > > On Thu, Aug 07, 2025, Sagi Shahar wrote: > > From: Isaku Yamahata <[email protected]> > > > > Let kvm_init_vm_address_properties() initialize vm->arch.{s_bit, tag_mask} > > similar to SEV. > > > > Set shared bit position based on guest maximum physical address width > > instead of maximum physical address width, because that is what KVM > > uses, > > "because KVM does it" is not an acceptable explanation. > > > refer to setup_tdparams_eptp_controls(), and because maximum physical > > address width can be different. > > > > In the case of SRF, guest maximum physical address width is 48 because SRF > > does not support 5-level EPT, even though the maximum physical address > > width is 52. > > Referencing a specific Intel microarchitecture is not proper justification for > why something is supported/legal/correct. Using its three-letter acronym is > just icing on the cake. > > > Co-developed-by: Adrian Hunter <[email protected]> > > Signed-off-by: Adrian Hunter <[email protected]> > > Signed-off-by: Isaku Yamahata <[email protected]> > > Signed-off-by: Sagi Shahar <[email protected]> > > --- > > tools/testing/selftests/kvm/lib/x86/processor.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/lib/x86/processor.c > > b/tools/testing/selftests/kvm/lib/x86/processor.c > > index d082d429e127..5718b5911b0a 100644 > > --- a/tools/testing/selftests/kvm/lib/x86/processor.c > > +++ b/tools/testing/selftests/kvm/lib/x86/processor.c > > @@ -1166,10 +1166,19 @@ void kvm_get_cpu_address_width(unsigned int > > *pa_bits, unsigned int *va_bits) > > > > void kvm_init_vm_address_properties(struct kvm_vm *vm) > > { > > + uint32_t gpa_bits = kvm_cpu_property(X86_PROPERTY_GUEST_MAX_PHY_ADDR); > > + > > if (is_sev_vm(vm)) { > > vm->arch.sev_fd = open_sev_dev_path_or_exit(); > > vm->arch.c_bit = > > BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT)); > > vm->gpa_tag_mask = vm->arch.c_bit; > > + } else if (vm->type == KVM_X86_TDX_VM) { > > Please add an is_tdx_vm() helper. > > > + TEST_ASSERT(gpa_bits == 48 || gpa_bits == 52, > > + "TDX: bad X86_PROPERTY_GUEST_MAX_PHY_ADDR value: > > %u", gpa_bits); > > + vm->arch.sev_fd = -1; > > + vm->arch.s_bit = 1ULL << (gpa_bits - 1); > > + vm->arch.c_bit = 0; > > The VM is zero-initialized, no need to set c_bit. > > > + vm->gpa_tag_mask = vm->arch.s_bit; > > } else { > > vm->arch.sev_fd = -1; > > I think it makes sense to set sev_fd to -1 by default instead of duplicating > the > non-SEV logic into all non-SEV paths. SEV VMs will write it twice, but > that's a > non-issue. E.g. > > vm->arch.sev_fd = -1; > > if (is_sev_vm(vm)) { > vm->arch.sev_fd = open_sev_dev_path_or_exit(); > vm->arch.c_bit = > BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT)); > vm->gpa_tag_mask = vm->arch.c_bit; > } else if (is_tdx_vm(vm)) { > TEST_ASSERT(gpa_bits == 48 || gpa_bits == 52, > "TDX: bad X86_PROPERTY_GUEST_MAX_PHY_ADDR value: > %u", gpa_bits); > vm->arch.s_bit = 1ULL << (gpa_bits - 1); > vm->gpa_tag_mask = vm->arch.s_bit; > } >
Thanks for all the suggestions. I will incorporate them in the next version.
