| Message ID | 20240515005952.3410568-5-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX MMU prep series part 1 | expand |
On 15/05/2024 12:59 pm, Rick Edgecombe wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > Introduce a "gfn_shared_mask" field in the kvm_arch structure to record GPA > shared bit and provide address conversion helpers for TDX shared bit of > GPA. > > TDX designates a specific GPA bit as the shared bit, which can be either > bit 51 or bit 47 based on configuration. > > This GPA shared bit indicates whether the corresponding physical page is > shared (if shared bit set) or private (if shared bit cleared). > > - GPAs with shared bit set will be mapped by VMM into conventional EPT, > which is pointed by shared EPTP in TDVMCS, resides in host VMM memory > and is managed by VMM. > - GPAs with shared bit cleared will be mapped by VMM firstly into a > mirrored EPT, which resides in host VMM memory. Changes of the mirrored > EPT are then propagated into a private EPT, which resides outside of host > VMM memory and is managed by TDX module. > > Add the "gfn_shared_mask" field to the kvm_arch structure for each VM with > a default value of 0. It will be set to the position of the GPA shared bit > in GFN through TD specific initialization code. > > Provide helpers to utilize the gfn_shared_mask to determine whether a GPA > is shared or private, retrieve the GPA shared bit value, and insert/strip > shared bit to/from a GPA. I am seriously thinking whether we should just abandon this whole kvm_gfn_shared_mask() thing. We already have enough mechanisms around private memory and the mapping of it: 1) Xarray to query whether a given GFN is private or shared; 2) fault->is_private to indicate whether a faulting address is private or shared; 3) sp->is_private to indicate whether a "page table" is only for private mapping; Consider this as 4) -- I also like to have a kvm->arch.has_mirrored_pt (or a better name) as I replied here: https://lore.kernel.org/kvm/20240515005952.3410568-17-rick.p.edgecombe@intel.com/T/#m49b37658f03e786c6aa43719cbf748215170980d So I believe we really already have enough mechanisms in the *COMMON* code for private page/mapping support. I intend to believe the whole GPA shared bit thing can be hidden in TDX specific operations. If there's really a need to apply/strip GPA shared bit in the common code, we can do via kvm_x86_ops callback (I'll review other patches to see). And btw, I think ... [...] > + > +/* > + * default or SEV-SNP TDX: where S = (47 or 51) - 12 > + * gfn_shared_mask 0 S bit > + * is_private_gpa() always false true if GPA has S bit clear ... this @is_private_gpa(), and ... > + * gfn_to_shared() nop set S bit > + * gfn_to_private() nop clear S bit > + * > + * fault.is_private means that host page should be gotten from guest_memfd > + * is_private_gpa() means that KVM MMU should invoke private MMU hooks. > + */ ... this invoking MMU hooks based on @is_private_gpa() makes no sense, because clearly for SEV-SNP @is_priavate_gpa() isn't report the fact when the GPA is indeed private, and the MMU hooks should be invoked based on whether the faulting GPA is private or not, but not this @is_private_gpa().
On Thu, 2024-05-16 at 10:34 +1200, Huang, Kai wrote: > > > On 15/05/2024 12:59 pm, Rick Edgecombe wrote: > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > Introduce a "gfn_shared_mask" field in the kvm_arch structure to record GPA > > shared bit and provide address conversion helpers for TDX shared bit of > > GPA. > > > > TDX designates a specific GPA bit as the shared bit, which can be either > > bit 51 or bit 47 based on configuration. > > > > This GPA shared bit indicates whether the corresponding physical page is > > shared (if shared bit set) or private (if shared bit cleared). > > > > - GPAs with shared bit set will be mapped by VMM into conventional EPT, > > which is pointed by shared EPTP in TDVMCS, resides in host VMM memory > > and is managed by VMM. > > - GPAs with shared bit cleared will be mapped by VMM firstly into a > > mirrored EPT, which resides in host VMM memory. Changes of the mirrored > > EPT are then propagated into a private EPT, which resides outside of > > host > > VMM memory and is managed by TDX module. > > > > Add the "gfn_shared_mask" field to the kvm_arch structure for each VM with > > a default value of 0. It will be set to the position of the GPA shared bit > > in GFN through TD specific initialization code. > > > > Provide helpers to utilize the gfn_shared_mask to determine whether a GPA > > is shared or private, retrieve the GPA shared bit value, and insert/strip > > shared bit to/from a GPA. > > I am seriously thinking whether we should just abandon this whole > kvm_gfn_shared_mask() thing. > > We already have enough mechanisms around private memory and the mapping > of it: > > 1) Xarray to query whether a given GFN is private or shared; > 2) fault->is_private to indicate whether a faulting address is private > or shared; > 3) sp->is_private to indicate whether a "page table" is only for private > mapping; You mean drop the helpers, or the struct kvm member? I think we still need the shared bit position stored somewhere. memslots, Xarray, etc need to operate on the GFN without the shared it.
On 16/05/2024 11:21 am, Edgecombe, Rick P wrote: > On Thu, 2024-05-16 at 10:34 +1200, Huang, Kai wrote: >> >> >> On 15/05/2024 12:59 pm, Rick Edgecombe wrote: >>> From: Isaku Yamahata <isaku.yamahata@intel.com> >>> >>> Introduce a "gfn_shared_mask" field in the kvm_arch structure to record GPA >>> shared bit and provide address conversion helpers for TDX shared bit of >>> GPA. >>> >>> TDX designates a specific GPA bit as the shared bit, which can be either >>> bit 51 or bit 47 based on configuration. >>> >>> This GPA shared bit indicates whether the corresponding physical page is >>> shared (if shared bit set) or private (if shared bit cleared). >>> >>> - GPAs with shared bit set will be mapped by VMM into conventional EPT, >>> which is pointed by shared EPTP in TDVMCS, resides in host VMM memory >>> and is managed by VMM. >>> - GPAs with shared bit cleared will be mapped by VMM firstly into a >>> mirrored EPT, which resides in host VMM memory. Changes of the mirrored >>> EPT are then propagated into a private EPT, which resides outside of >>> host >>> VMM memory and is managed by TDX module. >>> >>> Add the "gfn_shared_mask" field to the kvm_arch structure for each VM with >>> a default value of 0. It will be set to the position of the GPA shared bit >>> in GFN through TD specific initialization code. >>> >>> Provide helpers to utilize the gfn_shared_mask to determine whether a GPA >>> is shared or private, retrieve the GPA shared bit value, and insert/strip >>> shared bit to/from a GPA. >> >> I am seriously thinking whether we should just abandon this whole >> kvm_gfn_shared_mask() thing. >> >> We already have enough mechanisms around private memory and the mapping >> of it: >> >> 1) Xarray to query whether a given GFN is private or shared; >> 2) fault->is_private to indicate whether a faulting address is private >> or shared; >> 3) sp->is_private to indicate whether a "page table" is only for private >> mapping; > > You mean drop the helpers, or the struct kvm member? I think we still need the > shared bit position stored somewhere. memslots, Xarray, etc need to operate on > the GFN without the shared it. The struct member, and the whole thing. The shared bit is only included in the faulting address, and we can strip that away upon handle_ept_violation(). One thing I can think of is we still need to append the shared bit to the actual GFN when we setup the shared page table mapping. For that I am thinking whether we can do in TDX specific code. Anyway, I don't think the 'gfn_shared_mask' is necessarily good at this stage.
On Thu, 2024-05-16 at 11:31 +1200, Huang, Kai wrote: > > > On 16/05/2024 11:21 am, Edgecombe, Rick P wrote: > > On Thu, 2024-05-16 at 10:34 +1200, Huang, Kai wrote: > > > > > > > > > On 15/05/2024 12:59 pm, Rick Edgecombe wrote: > > > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > > > > > Introduce a "gfn_shared_mask" field in the kvm_arch structure to record > > > > GPA > > > > shared bit and provide address conversion helpers for TDX shared bit of > > > > GPA. > > > > > > > > TDX designates a specific GPA bit as the shared bit, which can be either > > > > bit 51 or bit 47 based on configuration. > > > > > > > > This GPA shared bit indicates whether the corresponding physical page is > > > > shared (if shared bit set) or private (if shared bit cleared). > > > > > > > > - GPAs with shared bit set will be mapped by VMM into conventional EPT, > > > > which is pointed by shared EPTP in TDVMCS, resides in host VMM > > > > memory > > > > and is managed by VMM. > > > > - GPAs with shared bit cleared will be mapped by VMM firstly into a > > > > mirrored EPT, which resides in host VMM memory. Changes of the > > > > mirrored > > > > EPT are then propagated into a private EPT, which resides outside > > > > of > > > > host > > > > VMM memory and is managed by TDX module. > > > > > > > > Add the "gfn_shared_mask" field to the kvm_arch structure for each VM > > > > with > > > > a default value of 0. It will be set to the position of the GPA shared > > > > bit > > > > in GFN through TD specific initialization code. > > > > > > > > Provide helpers to utilize the gfn_shared_mask to determine whether a > > > > GPA > > > > is shared or private, retrieve the GPA shared bit value, and > > > > insert/strip > > > > shared bit to/from a GPA. > > > > > > I am seriously thinking whether we should just abandon this whole > > > kvm_gfn_shared_mask() thing. > > > > > > We already have enough mechanisms around private memory and the mapping > > > of it: > > > > > > 1) Xarray to query whether a given GFN is private or shared; > > > 2) fault->is_private to indicate whether a faulting address is private > > > or shared; > > > 3) sp->is_private to indicate whether a "page table" is only for private > > > mapping; > > > > You mean drop the helpers, or the struct kvm member? I think we still need > > the > > shared bit position stored somewhere. memslots, Xarray, etc need to operate > > on > > the GFN without the shared it. > > The struct member, and the whole thing. The shared bit is only included > in the faulting address, and we can strip that away upon > handle_ept_violation(). > > One thing I can think of is we still need to append the shared bit to > the actual GFN when we setup the shared page table mapping. For that I > am thinking whether we can do in TDX specific code. > > Anyway, I don't think the 'gfn_shared_mask' is necessarily good at this > stage. Sorry, still not clear. We need to strip the bit away, so we need to know what bit it is. The proposal is to not remember it on struct kvm, so where do we get it? Actually, we used to allow it to be selected (via GPAW), but now we could determine it based on EPT level and MAXPA. So we could possibly recalculate it in some helper... But it seems you are suggesting to do away with the concept of knowing what the shared bit is.
On 16/05/2024 11:38 am, Edgecombe, Rick P wrote: > On Thu, 2024-05-16 at 11:31 +1200, Huang, Kai wrote: >> >> >> On 16/05/2024 11:21 am, Edgecombe, Rick P wrote: >>> On Thu, 2024-05-16 at 10:34 +1200, Huang, Kai wrote: >>>> >>>> >>>> On 15/05/2024 12:59 pm, Rick Edgecombe wrote: >>>>> From: Isaku Yamahata <isaku.yamahata@intel.com> >>>>> >>>>> Introduce a "gfn_shared_mask" field in the kvm_arch structure to record >>>>> GPA >>>>> shared bit and provide address conversion helpers for TDX shared bit of >>>>> GPA. >>>>> >>>>> TDX designates a specific GPA bit as the shared bit, which can be either >>>>> bit 51 or bit 47 based on configuration. >>>>> >>>>> This GPA shared bit indicates whether the corresponding physical page is >>>>> shared (if shared bit set) or private (if shared bit cleared). >>>>> >>>>> - GPAs with shared bit set will be mapped by VMM into conventional EPT, >>>>> which is pointed by shared EPTP in TDVMCS, resides in host VMM >>>>> memory >>>>> and is managed by VMM. >>>>> - GPAs with shared bit cleared will be mapped by VMM firstly into a >>>>> mirrored EPT, which resides in host VMM memory. Changes of the >>>>> mirrored >>>>> EPT are then propagated into a private EPT, which resides outside >>>>> of >>>>> host >>>>> VMM memory and is managed by TDX module. >>>>> >>>>> Add the "gfn_shared_mask" field to the kvm_arch structure for each VM >>>>> with >>>>> a default value of 0. It will be set to the position of the GPA shared >>>>> bit >>>>> in GFN through TD specific initialization code. >>>>> >>>>> Provide helpers to utilize the gfn_shared_mask to determine whether a >>>>> GPA >>>>> is shared or private, retrieve the GPA shared bit value, and >>>>> insert/strip >>>>> shared bit to/from a GPA. >>>> >>>> I am seriously thinking whether we should just abandon this whole >>>> kvm_gfn_shared_mask() thing. >>>> >>>> We already have enough mechanisms around private memory and the mapping >>>> of it: >>>> >>>> 1) Xarray to query whether a given GFN is private or shared; >>>> 2) fault->is_private to indicate whether a faulting address is private >>>> or shared; >>>> 3) sp->is_private to indicate whether a "page table" is only for private >>>> mapping; >>> >>> You mean drop the helpers, or the struct kvm member? I think we still need >>> the >>> shared bit position stored somewhere. memslots, Xarray, etc need to operate >>> on >>> the GFN without the shared it. >> >> The struct member, and the whole thing. The shared bit is only included >> in the faulting address, and we can strip that away upon >> handle_ept_violation(). >> >> One thing I can think of is we still need to append the shared bit to >> the actual GFN when we setup the shared page table mapping. For that I >> am thinking whether we can do in TDX specific code. >> >> Anyway, I don't think the 'gfn_shared_mask' is necessarily good at this >> stage. > > Sorry, still not clear. We need to strip the bit away, so we need to know what > bit it is. The proposal is to not remember it on struct kvm, so where do we get > it? The TDX specific code can get it when TDX guest is created. > > Actually, we used to allow it to be selected (via GPAW), but now we could > determine it based on EPT level and MAXPA. So we could possibly recalculate it > in some helper... > > But it seems you are suggesting to do away with the concept of knowing what the > shared bit is. What I am suggesting is essentially to replace this kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just return the shared bit), assuming the common code somehow still need it (e.g., setting up the SPTE for shared mapping, which must include the shared bit to the GPA). The advantage of this we can get rid of the concept of 'gfn_shared_mask' in the MMU common code. All GFNs referenced in the common code is the actual GFN (w/o the shared bit).
On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote: > > > > Sorry, still not clear. We need to strip the bit away, so we need to know > > what > > bit it is. The proposal is to not remember it on struct kvm, so where do we > > get > > it? > > The TDX specific code can get it when TDX guest is created. The TDX specific code sets it. It knows GPAW/shared bit location. > > > > > Actually, we used to allow it to be selected (via GPAW), but now we could > > determine it based on EPT level and MAXPA. So we could possibly recalculate > > it > > in some helper... > > > > But it seems you are suggesting to do away with the concept of knowing what > > the > > shared bit is. > > What I am suggesting is essentially to replace this > kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just > return the shared bit), assuming the common code somehow still need it > (e.g., setting up the SPTE for shared mapping, which must include the > shared bit to the GPA). > > The advantage of this we can get rid of the concept of 'gfn_shared_mask' > in the MMU common code. All GFNs referenced in the common code is the > actual GFN (w/o the shared bit). When it is actually being used as the shared bit instead of as a way to check if a guest is a TD, what is the problem? I think the shared_mask serves a real (small) purpose, but it is misused for a bunch of other stuff. If we move that other stuff to new helpers, the shared mask will still be needed for it's original job. What is the benefit of the x86_ops over a static inline?
On 16/05/2024 11:59 am, Edgecombe, Rick P wrote: > On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote: >>> >>> Sorry, still not clear. We need to strip the bit away, so we need to know >>> what >>> bit it is. The proposal is to not remember it on struct kvm, so where do we >>> get >>> it? >> >> The TDX specific code can get it when TDX guest is created. > > The TDX specific code sets it. It knows GPAW/shared bit location. > >> >>> >>> Actually, we used to allow it to be selected (via GPAW), but now we could >>> determine it based on EPT level and MAXPA. So we could possibly recalculate >>> it >>> in some helper... >>> >>> But it seems you are suggesting to do away with the concept of knowing what >>> the >>> shared bit is. >> >> What I am suggesting is essentially to replace this >> kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just >> return the shared bit), assuming the common code somehow still need it >> (e.g., setting up the SPTE for shared mapping, which must include the >> shared bit to the GPA). >> >> The advantage of this we can get rid of the concept of 'gfn_shared_mask' >> in the MMU common code. All GFNs referenced in the common code is the >> actual GFN (w/o the shared bit). > > When it is actually being used as the shared bit instead of as a way to check if > a guest is a TD, what is the problem? I think the shared_mask serves a real > (small) purpose, but it is misused for a bunch of other stuff. If we move that > other stuff to new helpers, the shared mask will still be needed for it's > original job. > > What is the benefit of the x86_ops over a static inline? I don't have strong objection if the use of kvm_gfn_shared_mask() is contained in smaller areas that truly need it. Let's discuss in relevant patch(es). However I do think the helpers like below makes no sense (for SEV-SNP): +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) +{ + gfn_t mask = kvm_gfn_shared_mask(kvm); + + return mask && !(gpa_to_gfn(gpa) & mask); +}
On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote: > > I don't have strong objection if the use of kvm_gfn_shared_mask() is > contained in smaller areas that truly need it. Let's discuss in > relevant patch(es). > > However I do think the helpers like below makes no sense (for SEV-SNP): > > +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) > +{ > + gfn_t mask = kvm_gfn_shared_mask(kvm); > + > + return mask && !(gpa_to_gfn(gpa) & mask); > +} You mean the name? SNP doesn't have a concept of "private GPA" IIUC. The C bit is more like an permission bit. So SNP doesn't have private GPAs, and the function would always return false for SNP. So I'm not sure it's too horrible. If it's the name, can you suggest something?
On 16/05/2024 12:19 pm, Edgecombe, Rick P wrote: > On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote: >> >> I don't have strong objection if the use of kvm_gfn_shared_mask() is >> contained in smaller areas that truly need it. Let's discuss in >> relevant patch(es). >> >> However I do think the helpers like below makes no sense (for SEV-SNP): >> >> +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) >> +{ >> + gfn_t mask = kvm_gfn_shared_mask(kvm); >> + >> + return mask && !(gpa_to_gfn(gpa) & mask); >> +} > > You mean the name? SNP doesn't have a concept of "private GPA" IIUC. The C bit > is more like an permission bit. So SNP doesn't have private GPAs, and the > function would always return false for SNP. So I'm not sure it's too horrible. Hmm.. Why SNP doesn't have private GPAs? They are crypto-protected and KVM cannot access directly correct? > > If it's the name, can you suggest something? The name make sense, but it has to reflect the fact that a given GPA is truly private (crypto-protected, inaccessible to KVM).
On Thu, 2024-05-16 at 12:25 +1200, Huang, Kai wrote: > > > On 16/05/2024 12:19 pm, Edgecombe, Rick P wrote: > > On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote: > > > > > > I don't have strong objection if the use of kvm_gfn_shared_mask() is > > > contained in smaller areas that truly need it. Let's discuss in > > > relevant patch(es). > > > > > > However I do think the helpers like below makes no sense (for SEV-SNP): > > > > > > +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) > > > +{ > > > + gfn_t mask = kvm_gfn_shared_mask(kvm); > > > + > > > + return mask && !(gpa_to_gfn(gpa) & mask); > > > +} > > > > You mean the name? SNP doesn't have a concept of "private GPA" IIUC. The C > > bit > > is more like an permission bit. So SNP doesn't have private GPAs, and the > > function would always return false for SNP. So I'm not sure it's too > > horrible. > > Hmm.. Why SNP doesn't have private GPAs? They are crypto-protected and > KVM cannot access directly correct? I suppose a GPA could be pointing to memory that is private. But I think in SNP it is more the memory that is private. Now I see more how it could be confusing. > > > > > If it's the name, can you suggest something? > > The name make sense, but it has to reflect the fact that a given GPA is > truly private (crypto-protected, inaccessible to KVM). If this was a function that tested whether memory is private and took a GPA, I would call it is_private_mem() or something. Because it's testing the memory and takes a GPA, not testing the GPA. Usually a function name should be about what the function does, not what arguments it takes. I can't think of a better name, but point taken that it is not ideal. I'll try to think of something.
On 16/05/2024 12:35 pm, Edgecombe, Rick P wrote: > On Thu, 2024-05-16 at 12:25 +1200, Huang, Kai wrote: >> >> >> On 16/05/2024 12:19 pm, Edgecombe, Rick P wrote: >>> On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote: >>>> >>>> I don't have strong objection if the use of kvm_gfn_shared_mask() is >>>> contained in smaller areas that truly need it. Let's discuss in >>>> relevant patch(es). >>>> >>>> However I do think the helpers like below makes no sense (for SEV-SNP): >>>> >>>> +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) >>>> +{ >>>> + gfn_t mask = kvm_gfn_shared_mask(kvm); >>>> + >>>> + return mask && !(gpa_to_gfn(gpa) & mask); >>>> +} >>> >>> You mean the name? SNP doesn't have a concept of "private GPA" IIUC. The C >>> bit >>> is more like an permission bit. So SNP doesn't have private GPAs, and the >>> function would always return false for SNP. So I'm not sure it's too >>> horrible. >> >> Hmm.. Why SNP doesn't have private GPAs? They are crypto-protected and >> KVM cannot access directly correct? > > I suppose a GPA could be pointing to memory that is private. But I think in SNP > it is more the memory that is private. Now I see more how it could be confusing. > >> >>> >>> If it's the name, can you suggest something? >> >> The name make sense, but it has to reflect the fact that a given GPA is >> truly private (crypto-protected, inaccessible to KVM). > > If this was a function that tested whether memory is private and took a GPA, I > would call it is_private_mem() or something. Because it's testing the memory and > takes a GPA, not testing the GPA. Usually a function name should be about what > the function does, not what arguments it takes. > > I can't think of a better name, but point taken that it is not ideal. I'll try > to think of something. > 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. 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.
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.
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. 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.
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. >
On Wed, 2024-05-15 at 18:20 -0700, Rick Edgecombe 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. What about: bool kvm_on_private_root(const struct kvm *kvm, gpa_t gpa); Since SNP doesn't have a private root, it can't get confused for SNP. For TDX it's a little weirder. We usually want to know if the GPA is to the private half. Whether it's on a separate root or not is not really important to the callers. But they could infer that if it's on a private root it must be a private GPA. Otherwise: bool kvm_is_private_gpa_bits(const struct kvm *kvm, gpa_t gpa); The bits indicates it's checking actual bits in the GPA and not the private/shared state of the GFN.
On 17/05/2024 11:08 am, Edgecombe, Rick P wrote: > On Wed, 2024-05-15 at 18:20 -0700, Rick Edgecombe 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. > > What about: > bool kvm_on_private_root(const struct kvm *kvm, gpa_t gpa); > > Since SNP doesn't have a private root, it can't get confused for SNP. For TDX > it's a little weirder. We usually want to know if the GPA is to the private > half. Whether it's on a separate root or not is not really important to the > callers. But they could infer that if it's on a private root it must be a > private GPA. > > > Otherwise: > bool kvm_is_private_gpa_bits(const struct kvm *kvm, gpa_t gpa); > > The bits indicates it's checking actual bits in the GPA and not the > private/shared state of the GFN. The kvm_on_private_root() is better to me, assuming this helper wants to achieve two goals: 1) whether a given GPA is private; 2) and when it is, whether to use private table; And AFAICT we still want this implementation: + gfn_t mask = kvm_gfn_shared_mask(kvm); + + return mask && !(gpa_to_gfn(gpa) & mask); What I don't quite like is we use ... !(gpa_to_gfn(gpa) & mask); ... to tell whether a GPA is private, because it is TDX specific logic cause it doesn't tell on SNP whether the GPA is private. But as you said it certainly makes sense to say "we won't use a private table for this GPA" when the VM doesn't have a private table at all. So it's also fine to me. But my question is "why we need this helper at all". As I expressed before, my concern is we already have too many mechanisms around private/shared memory/mapping, and I am wondering whether we can get rid of kvm_gfn_shared_mask() completely. E.g, why we cannot do: static bool kvm_use_private_root(struct kvm *kvm) { return kvm->arch.vm_type == VM_TYPE_TDX; } Or, static bool kvm_use_private_root(struct kvm *kvm) { return kvm->arch.use_private_root; } Or, assuming we would love to keep the kvm_gfn_shared_mask(): static bool kvm_use_private_root(struct kvm *kvm) { return !!kvm_gfn_shared_mask(kvm); } And then: In fault handler: if (fault->is_private && kvm_use_private_root(kvm)) // use private root else // use shared/normal root When you zap: bool private_gpa = kvm_mem_is_private(kvm, gfn); if (private_gpa && kvm_use_private_root(kvm)) // zap private root else // zap shared/normal root. The benefit of this is we can clearly split the logic of: 1) whether a GPN is private, and 2) whether to use private table for private GFN But it's certainly possible that I am missing something, though. Do you see any problem of above? Again, my main concern is whether we should just get rid of the kvm_gfn_shared_mask() completely (so we won't be able to abuse to use it) due to we already having so many mechanisms around private/shared memory/mapping here. But I also understand we anyway will need to add the shared bit back when we setup page table or teardown of it, but for this purpose we can also use: kvm_x86_ops->get_shared_gfn_mask(kvm) So to me the kvm_shared_gfn_mask() is at least not mandatory. Anyway, it's not a very strong opinion from me that we should remove the kvm_shared_gfn_mask(), assuming we won't abuse to use it just for convenience in common code. I hope I have expressed my view clearly. And consider this as just my 2 cents.
We agreed to remove the abuse of kvm_gfn_shared_mask() and look at it again. I was just checking back in on the name of the other function as I said I would. Never-the-less... On Fri, 2024-05-17 at 12:37 +1200, Huang, Kai wrote: > The kvm_on_private_root() is better to me, assuming this helper wants to > achieve two goals: > > 1) whether a given GPA is private; > 2) and when it is, whether to use private table; > > And AFAICT we still want this implementation: > > + gfn_t mask = kvm_gfn_shared_mask(kvm); > + > + return mask && !(gpa_to_gfn(gpa) & mask); No, like this: static inline bool kvm_on_private_root(const struct kvm *kvm, gpa_t gpa) { gfn_t mask = kvm_gfn_shared_mask(kvm); return kvm_has_private_root(kvm) && !(gpa_to_gfn(gpa) & mask); } > > What I don't quite like is we use ... > > !(gpa_to_gfn(gpa) & mask); > > ... to tell whether a GPA is private, because it is TDX specific logic > cause it doesn't tell on SNP whether the GPA is private. These helpers are where we hide what will functionally be the same as "if tdx". The other similar ones literally check for KVM_X86_TDX_VM. > > But as you said it certainly makes sense to say "we won't use a private > table for this GPA" when the VM doesn't have a private table at all. So > it's also fine to me. > > But my question is "why we need this helper at all". > > As I expressed before, my concern is we already have too many mechanisms > around private/shared memory/mapping, Everyone is in agreement here, you don't need to make the point again. > and I am wondering whether we can > get rid of kvm_gfn_shared_mask() completely. You mentioned... > > E.g, why we cannot do: > > static bool kvm_use_private_root(struct kvm *kvm) > { > return kvm->arch.vm_type == VM_TYPE_TDX; > } > > Or, > static bool kvm_use_private_root(struct kvm *kvm) > { > return kvm->arch.use_private_root; > } > > Or, assuming we would love to keep the kvm_gfn_shared_mask(): > > static bool kvm_use_private_root(struct kvm *kvm) > { > return !!kvm_gfn_shared_mask(kvm); > } > > And then: > > In fault handler: > > if (fault->is_private && kvm_use_private_root(kvm)) > // use private root > else > // use shared/normal root > > When you zap: > > bool private_gpa = kvm_mem_is_private(kvm, gfn); > > if (private_gpa && kvm_use_private_root(kvm)) > // zap private root > else > // zap shared/normal root. > I think you are trying to say not to abuse kvm_gfn_shared_mask() as is currently done in this logic. But we already agreed on this. So not sure. > The benefit of this is we can clearly split the logic of: > > 1) whether a GPN is private, and > 2) whether to use private table for private GFN > > But it's certainly possible that I am missing something, though. > > Do you see any problem of above? > > Again, my main concern is whether we should just get rid of the > kvm_gfn_shared_mask() completely Sigh... > (so we won't be able to abuse to use > it) due to we already having so many mechanisms around private/shared > memory/mapping here. > > But I also understand we anyway will need to add the shared bit back > when we setup page table or teardown of it, but for this purpose we can > also use: > > kvm_x86_ops->get_shared_gfn_mask(kvm) > > So to me the kvm_shared_gfn_mask() is at least not mandatory. Up the thread we have: On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote: > > What is the benefit of the x86_ops over a static inline? > I don't have strong objection if the use of kvm_gfn_shared_mask() is > contained in smaller areas that truly need it. Let's discuss in > relevant patch(es). So.. same question. > > Anyway, it's not a very strong opinion from me that we should remove the > kvm_shared_gfn_mask() This is a shock! > , assuming we won't abuse to use it just for > convenience in common code. > > I hope I have expressed my view clearly. > > And consider this as just my 2 cents. I don't think we can get rid of the shared mask. Even if we relied on kvm_mem_is_private() to determine if a GPA is private or shared, at absolute minimum we need to add the shared bit when we are zapping a GFN or mapping it. Let's table the discussion until we have some code to look again.
>> E.g, why we cannot do: >> >> static bool kvm_use_private_root(struct kvm *kvm) >> { >> return kvm->arch.vm_type == VM_TYPE_TDX; >> } >> >> Or, >> static bool kvm_use_private_root(struct kvm *kvm) >> { >> return kvm->arch.use_private_root; >> } >> >> Or, assuming we would love to keep the kvm_gfn_shared_mask(): >> >> static bool kvm_use_private_root(struct kvm *kvm) >> { >> return !!kvm_gfn_shared_mask(kvm); >> } >> >> And then: >> >> In fault handler: >> >> if (fault->is_private && kvm_use_private_root(kvm)) >> // use private root >> else >> // use shared/normal root >> >> When you zap: >> >> bool private_gpa = kvm_mem_is_private(kvm, gfn); >> >> if (private_gpa && kvm_use_private_root(kvm)) >> // zap private root >> else >> // zap shared/normal root. >> > > I think you are trying to say not to abuse kvm_gfn_shared_mask() as is currently > done in this logic. But we already agreed on this. So not sure. To be clear: We agreed on this in general, but not on this kvm_on_private_root(). It's obvious that you still want to "use kvm_gfn_shared_mask() to determine whether a GPA is private" for this helper but I don't like it. In fact I don't see why we even need this helper. I think I am just too obsessed on avoiding using kvm_gfn_shared_mask() so I'll stop commenting/replying on this. [...] > > I don't think we can get rid of the shared mask. Even if we relied on > kvm_mem_is_private() to determine if a GPA is private or shared, at absolute > minimum we need to add the shared bit when we are zapping a GFN or mapping it. No we cannot, but we can avoid using it here. > > Let's table the discussion until we have some code to look again. 100% agreed.
On Fri, 2024-05-17 at 16:26 +1200, Huang, Kai wrote: > > I think I am just too obsessed on avoiding using kvm_gfn_shared_mask() > > so I'll stop commenting/replying on this. I think you just need to stick with it and discuss it a little more. The pattern seems to go: 1. You comment somewhere saying you want to get rid of kvm_gfn_shared_mask() 2. I ask about how it can work 3. We don't get to the bottom of it 4. Go to step 1 I think you are seeing bad code, but the communication is leaving me seriously confused. The rework Isaku and I were doing in the other thread still includes a shared mask in the core MMU code, so it's still open at this point.
On Thu, 2024-05-16 at 13:52 +0800, Yan Zhao wrote: > > 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. In the new changes we are working on in the other thread this helper is moved into arch/x86/kvm/vmx/common.h for only Intel side use, and renamed: gpa_on_private_root(). It should address the SNP confusion concerns at least. On the private and mirrored point, the mixing of private and mirrored in the current code is definitely confusing. I think changing the names like that (private_mirror), could make it easier to understand, even if it creates longer lines. I tried to create some abstraction where the MMU understood the concept of general mirroring EPT roots, then checked a helper to see if the vm_type mirrored "private" memory before calling out to all the private helpers. I thought it would let us pretend more of this stuff was generic. But it was turning out a bit silly. So think I will just stick with updating the names for the next revision.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index aabf1648a56a..d2f924f1d579 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1519,6 +1519,8 @@ struct kvm_arch { */ #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1) struct kvm_mmu_memory_cache split_desc_cache; + + gfn_t gfn_shared_mask; }; struct kvm_vm_stat { diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index 3c7a88400cbb..dac13a2d944f 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -321,4 +321,37 @@ static inline gpa_t kvm_translate_gpa(struct kvm_vcpu *vcpu, return gpa; return translate_nested_gpa(vcpu, gpa, access, exception); } + +/* + * default or SEV-SNP TDX: where S = (47 or 51) - 12 + * gfn_shared_mask 0 S bit + * is_private_gpa() always false true if GPA has S bit clear + * gfn_to_shared() nop set S bit + * gfn_to_private() nop clear S bit + * + * fault.is_private means that host page should be gotten from guest_memfd + * is_private_gpa() means that KVM MMU should invoke private MMU hooks. + */ +static inline gfn_t kvm_gfn_shared_mask(const struct kvm *kvm) +{ + return kvm->arch.gfn_shared_mask; +} + +static inline gfn_t kvm_gfn_to_shared(const struct kvm *kvm, gfn_t gfn) +{ + return gfn | kvm_gfn_shared_mask(kvm); +} + +static inline gfn_t kvm_gfn_to_private(const struct kvm *kvm, gfn_t gfn) +{ + return gfn & ~kvm_gfn_shared_mask(kvm); +} + +static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa) +{ + gfn_t mask = kvm_gfn_shared_mask(kvm); + + return mask && !(gpa_to_gfn(gpa) & mask); +} + #endif