Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 17 Jun 2021 14:37:25 +0100
Message-ID: <87h7hwd33e.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Will Deacon <will@kernel.org>
Cc:     Yanan Wang <wangyanan55@huawei.com>,
        Quentin Perret <qperret@google.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Catalin Marinas <catalin.marinas@arm.com>,
        James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Gavin Shan <gshan@redhat.com>, wanghaibin.wang@huawei.com,
        zhukeqian1@huawei.com, yuzenghui@huawei.com
Subject: Re: [PATCH v7 4/4] KVM: arm64: Move guest CMOs to the fault handlers
In-Reply-To: <20210617132115.GA24656@willie-the-truck>
References: <20210617105824.31752-1-wangyanan55@huawei.com>
        <20210617105824.31752-5-wangyanan55@huawei.com>
        <20210617124557.GB24457@willie-the-truck>
        <87k0msd4ue.wl-maz@kernel.org>
        <20210617132115.GA24656@willie-the-truck>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 17 Jun 2021 14:21:16 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Thu, Jun 17, 2021 at 01:59:37PM +0100, Marc Zyngier wrote:
> > On Thu, 17 Jun 2021 13:45:57 +0100,
> > Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jun 17, 2021 at 06:58:24PM +0800, Yanan Wang wrote:
> > > > @@ -606,6 +618,14 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
> > > >  		stage2_put_pte(ptep, data->mmu, addr, level, mm_ops);
> > > >  	}
> > > >  
> > > > +	/* Perform CMOs before installation of the guest stage-2 PTE */
> > > > +	if (mm_ops->clean_invalidate_dcache && stage2_pte_cacheable(pgt, new))
> > > > +		mm_ops->clean_invalidate_dcache(kvm_pte_follow(new, mm_ops),
> > > > +						granule);
> > > > +
> > > > +	if (mm_ops->invalidate_icache && stage2_pte_executable(new))
> > > > +		mm_ops->invalidate_icache(kvm_pte_follow(new, mm_ops), granule);
> > > 
> > > One thing I'm missing here is why we need the indirection via mm_ops. Are
> > > there cases where we would want to pass a different function pointer for
> > > invalidating the icache? If not, why not just call the function directly?
> > > 
> > > Same for the D side.
> > 
> > If we didn't do that, we'd end-up having to track whether the guest
> > context requires CMOs with additional flags, which is pretty ugly (see
> > v5 of this series for reference [1]).
> 
> Fair enough, although the function pointers here _are_ being used as
> flags, as they only ever have one of two possible values (NULL or
> the CMO function), so it's a shame to bring in the indirect branch
> as well.

What I hope eventually is to get rid of some of the FWB tracking we
have for the host in the protected case, and use the same abstraction.

> 
> > It also means that we would have to drag the CM functions into the EL2
> > object, something that we don't need with this approach.
> 
> I think it won't be long before we end up with CMO functions at EL2 and
> you'd hope we'd be able to use the same code as EL1 for something like
> that. But I also wouldn't want to put money on it...

It we reach that stage, I'll be happy to try and move these function
into some shared location.

> Anyway, no strong opinion on this, it just jumped out when I skimmed the
> patches.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.