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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZPipBOzdM9lj/uO9@linux.dev>
Date:   Wed, 6 Sep 2023 16:29:56 +0000
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Gavin Shan <gshan@...hat.com>
Cc:     kvmarm@...ts.linux.dev, linux-kernel@...r.kernel.org,
        maz@...nel.org, james.morse@....com, suzuki.poulose@....com,
        yuzenghui@...wei.com, catalin.marinas@....com, will@...nel.org,
        qperret@...gle.com, ricarkol@...gle.com, tabba@...gle.com,
        bgardon@...gle.com, zhenyzha@...hat.com, yihyu@...hat.com,
        shan.gavin@...il.com
Subject: Re: [PATCH] KVM: arm64: Fix soft-lockup on relaxing PTE permission

Gavin,

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.

> > > @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.

-- 
Thanks,
Oliver