On Thu, Feb 23, 2023 at 12:21 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Thu, Feb 23, 2023, Yu Zhao wrote: > > On Thu, Feb 23, 2023 at 11:47 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > On Thu, Feb 23, 2023, Yu Zhao wrote: > > > > On Thu, Feb 23, 2023 at 11:24 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > > > > > On Thu, Feb 23, 2023, Yu Zhao wrote: > > > > > > On Thu, Feb 23, 2023 at 10:09 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > > > I'll take a look at that series. clear_bit() probably won't cause any > > > > > > > > practical damage but is technically wrong because, for example, it can > > > > > > > > end up clearing the A-bit in a non-leaf PMD. (cmpxchg will just fail > > > > > > > > in this case, obviously.) > > > > > > > > > > > > > > Eh, not really. By that argument, clearing an A-bit in a huge PTE is also technically > > > > > > > wrong because the target gfn may or may not have been accessed. > > > > > > > > > > > > Sorry, I don't understand. You mean clear_bit() on a huge PTE is > > > > > > technically wrong? Yes, that's what I mean. (cmpxchg() on a huge PTE > > > > > > is not.) > > > > > > > > > > > > > The only way for > > > > > > > KVM to clear a A-bit in a non-leaf entry is if the entry _was_ a huge PTE, but was > > > > > > > replaced between the "is leaf" and the clear_bit(). > > > > > > > > > > > > I think there is a misunderstanding here. Let me be more specific: > > > > > > 1. Clearing the A-bit in a non-leaf entry is technically wrong because > > > > > > that's not our intention. > > > > > > 2. When we try to clear_bit() on a leaf PMD, it can at the same time > > > > > > become a non-leaf PMD, which causes 1) above, and therefore is > > > > > > technically wrong. > > > > > > 3. I don't think 2) could do any real harm, so no practically no problem. > > > > > > 4. cmpxchg() can avoid 2). > > > > > > > > > > > > Does this make sense? > > > > > > > > > > I understand what you're saying, but clearing an A-bit on a non-leaf PMD that > > > > > _just_ got converted from a leaf PMD is "wrong" if and only if the intented > > > > > behavior is nonsensical. > > > > > > > > Sorry, let me rephrase: > > > > 1. Clearing the A-bit in a non-leaf entry is technically wrong because > > > > we didn't make sure there is the A-bit there -- the bit we are > > > > clearing can be something else. (Yes, we know it's not, but we didn't > > > > define this behavior, e.g., a macro to designate that bit for non-leaf > > > > entries. > > > > > > Heh, by that definition, anything and everything is "technically wrong". > > > > I really don't see how what I said, in our context, > > > > "Clearing the A-bit in a non-leaf entry is technically wrong because > > we didn't make sure there is the A-bit there" > > > > can infer > > > > "anything and everything is "technically wrong"." > > > > And how what I said can be an analogy to > > > > "An Intel CPU might support SVM, even though we know no such CPUs > > exist, so requiring AMD or Hygon to enable SVM is technically wrong." > > > > BTW, here is a bug caused by clearing the A-bit in non-leaf entries in > > a different scenario: > > https://lore.kernel.org/linux-mm/20221123064510.16225-1-jgross@xxxxxxxx/ > > > > Let's just agree to disagree. > > No, because I don't want anyone to leave with the impression that relying on the > Accessed bit to uniformly exist (or not) at all levels in the TDP MMU is somehow > technically wrong. The link you posted is about running as a Xen guest, and is > in arch-agnostic code. That is wildly different than what we are talking about > here, where the targets are strictly limited to x86-64 TDP, and the existence of > the Accessed bit is architecturally defined. Yes, I see now. Sorry to say this, but this is all I needed to hear: "the existence of the Accessed bit is architecturally defined". (I didn't know and see this is what you meant.) > In this code, there are exactly two flavors of paging that can be in use, and > using clear_bit() to clear shadow_accessed_mask is safe for both, full stop.
