Re: [PATCH] KVM: arm64: Fix soft-lockup on relaxing PTE permission

[Gavin Shan]

Hi Oliver,

On 9/7/23 02:29, Oliver Upton wrote:
> On Wed, Sep 06, 2023 at 08:26:24AM +1000, Gavin Shan wrote:
>
> [...]
>
> > It seems I didn't make it clear enough. The reason why I had the concern
> > to avoid reading ctr_el0 is we read ctr_el0 for twice in the following path,
> > but I doubt if anybody cares. Since it's a hot path, each bit of performance
> > gain will count.
> >
> >    invalidate_icache_guest_page
> >    __invalidate_icache_guest_page   // first read on ctr_el0, with your code changes
> >    icache_inval_pou(va, va + size)
> >    invalidate_icache_by_line
> >      icache_line_size               // second read on ctr_el0
>
> That can be addressed by shoving the check deep into
> invalidate_icache_by_line, which would benefit _all_ use cases of
> I-cache invalidation by VA. I haven't completely made up my mind about
> that, though, because of the consequences of a global invalidation.

Yes, of course.

> > > > @size is guranteed to be PAGE_SIZE or PMD_SIZE aligned. Maybe
> > > > we can just aggressively do something like below, disregarding the icache thrashing.
> > > > In this way, the code is further simplified.
> > > >
> > > >       if (size > PAGE_SIZE) {
> > > >           icache_inval_all_pou();
> > > >       } else {
> > > >           icache_inval_pou((unsigned long)va,
> > > >                            (unsigned long)va + size);
> > > >       }                                                          // parantheses is still needed
> > >
> > > This could work too but we already have a kernel heuristic for limiting
> > > the amount of broadcast invalidations, which is MAX_TLBI_OPS. I don't
> > > want to introduce a second, KVM-specific hack to address the exact same
> > > thing.
> >
> > Ok. I was confused at the first glance since TLB isn't relevant to icache.
> > I think it's fine to reuse MAX_TLBI_OPS here, but a comment may be needed.
> > Oliver, could you please send a formal patch for your changes?
>
> Yeah, I think I may have said it before, but this thing needs to be
> called 'MAX_DVM_OPS'. I-cache invalidations and TLB invalidations become
> DVMOps (Distributed Virtual Memory) in terms of CHI, which pile up at the
> miscellaneous node in the mesh.
>
> Give me a day or two to convince myself of the right way to go about
> this and I'll send out what I have.

Ok. 'MAX_DVM_OPS' sounds good and it's a new name to me anyway. Oliver,
please let me know if you don't have time for this and need me to file
the formal patches, based on your codes :)

Thanks,
Gavin