Re: [PATCH 2/2] arm64: Notify on pte permission upgrades

[Alistair Popple]

Alistair Popple <apopple@xxxxxxxxxx> writes:

> Alistair Popple <apopple@xxxxxxxxxx> writes:
>
>>> On Tue, May 30, 2023 at 02:44:40PM -0700, Sean Christopherson wrote:
>>>> > KVM already has locking for invalidate_start/end - it has to check
>>>> > mmu_notifier_retry_cache() with the sequence numbers/etc around when
>>>> > it does does hva_to_pfn()
>>>> > 
>>>> > The bug is that the kvm_vcpu_reload_apic_access_page() path is
>>>> > ignoring this locking so it ignores in-progress range
>>>> > invalidations. It should spin until the invalidation clears like other
>>>> > places in KVM.
>>>> > 
>>>> > The comment is kind of misleading because drivers shouldn't be abusing
>>>> > the iommu centric invalidate_range() thing to fix missing locking in
>>>> > start/end users. :\
>>>> > 
>>>> > So if KVM could be fixed up we could make invalidate_range defined to
>>>> > be an arch specific callback to synchronize the iommu TLB.
>>>> 
>>>> And maybe rename invalidate_range() and/or invalidate_range_{start,end}() to make
>>>> it super obvious that they are intended for two different purposes?  E.g. instead
>>>> of invalidate_range(), something like invalidate_secondary_tlbs().
>>>
>>> Yeah, I think I would call it invalidate_arch_secondary_tlb() and
>>> document it as being an arch specific set of invalidations that match
>>> the architected TLB maintenance requrements. And maybe we can check it
>>> more carefully to make it be called in less places. Like I'm not sure
>>> it is right to call it from invalidate_range_end under this new
>>> definition..
>>
>> I'd be happy to look at that, although it sounds like Sean already is.

Thanks Sean for getting the KVM fix posted so quickly. I'm looking into
doing the rename now.

Do we want to do more than a simple rename and tidy up of callers
though? What I'm thinking is introducing something like an IOMMU/TLB
specific variant of notifiers (eg. struct tlb_notifier) which has the
invalidate_secondary_tlbs() callback in say struct tlb_notifier_ops
rather than leaving that in the mmu_notifier_ops.

Implementation wise we'd reuse most of the mmu_notifier code, but it
would help make the two different uses of notifiers clearer. Thoughts?

 - Alistair