On Thu, 17 Jun 2021 14:21:16 +0100, Will Deacon <will@xxxxxxxxxx> 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@xxxxxxxxxx> 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.
