Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <fa586ba6-0fb3-cf70-9e4b-97ca4fec82d6@redhat.com>
Date:   Tue, 19 Sep 2023 16:41:54 +1000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] KVM: arm64: Fix soft-lockup on relaxing PTE permission
Content-Language: en-US
To:     Oliver Upton <oliver.upton@linux.dev>
Cc:     kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org,
        maz@kernel.org, james.morse@arm.com, suzuki.poulose@arm.com,
        yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org,
        qperret@google.com, ricarkol@google.com, tabba@google.com,
        bgardon@google.com, zhenyzha@redhat.com, yihyu@redhat.com,
        shan.gavin@gmail.com
References: <20230904072826.1468907-1-gshan@redhat.com>
 <ZPWPoEgBETeI1um1@linux.dev>
 <0f93a015-4f10-b53e-f67f-a84db43ca533@redhat.com>
 <ZPduJ08GKaKXwIhM@linux.dev>
 <bfdafdc5-4abf-a387-0857-e8cb84e4b3d7@redhat.com>
 <ZPipBOzdM9lj/uO9@linux.dev>
From:   Gavin Shan <gshan@redhat.com>
In-Reply-To: <ZPipBOzdM9lj/uO9@linux.dev>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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