On Thu, May 16, 2024 at 01:40:41PM +1200, Huang, Kai wrote: > > > On 16/05/2024 1:20 pm, Edgecombe, Rick P wrote: > > On Thu, 2024-05-16 at 13:04 +1200, Huang, Kai wrote: > > > > > > I really don't see difference between ... > > > > > > is_private_mem(gpa) > > > > > > ... and > > > > > > is_private_gpa(gpa) > > > > > > If it confuses me, it can confuses other people. > > > > Again, point taken. I'll try to think of a better name. Please share if you do. > > > > > > > > The point is there's really no need to distinguish the two. The GPA is > > > only meaningful when it refers to the memory that it points to. > > > > > > So far I am not convinced we need this helper, because such info we can > > > already get from: > > > > > > 1) fault->is_private; > > > 2) Xarray which records memtype for given GFN. > > > > > > So we should just get rid of it. > > > > Kai, can you got look through the dev branch a bit more before making the same > > point on every patch? > > > > kvm_is_private_gpa() is used to set PFERR_PRIVATE_ACCESS, which in turn sets > > fault->is_private. So you are saying we can use these other things that are > > dependent on it. Look at the other callers too. > > Well, I think I didn't make myself clear. > > I don't object to have this helper. If it helps, then we can have it. > > My objection is the current implementation of it, because it is > *conceptually* wrong for SEV-SNP. > > Btw, I just look at the dev branch. > > For the common code, it is used in kvm_tdp_mmu_map() and > kvm_tdp_mmu_fast_pf_get_last_sptep() to get whether a GPA is private. > > As said above, I don't see why we need a helper with the "current > implementation" (which consults kvm_shared_gfn_mask()) for them. We can > just use fault->gfn + fault->is_private for such purpose. What about a name like kvm_is_private_and_mirrored_gpa()? Only TDX's private memory is mirrored and the common code needs a way to tell that. > It is also used in the TDX code like TDX variant handle_ept_violation() and > tdx_vcpu_init_mem_region(). For them to be honest I don't quite care > whether a helper is used. We can have a helper if we have multiple callers, > but this helper should be in TDX code, but not common MMU code. >
