Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally

[Marc Zyngier <marc.zyngier@arm.com>]

On Sat, 10 Mar 2018 03:23:18 +0000,
peng hao wrote:

> >> For emulation devices just like vga, keeping coherent dcache between
> >> guest and host timely is needed.
> >> Now the display of vnc-viewer will not update continuously and the
> >> patch can fix up.
> >> 
> >> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> >> ---
> >>  virt/kvm/arm/mmu.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ec62d1c..4a28395e 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>              kvm_set_pfn_dirty(pfn);
> >>          }
> >>  
> >> -        if (fault_status != FSC_PERM)
> >> -            clean_dcache_guest_page(pfn, PMD_SIZE);
> >> +        clean_dcache_guest_page(pfn, PMD_SIZE);
> >>  
> >>          if (exec_fault) {
> >>              new_pmd = kvm_s2pmd_mkexec(new_pmd);
> >> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>              mark_page_dirty(kvm, gfn);
> >>          }
> >>  
> >> -        if (fault_status != FSC_PERM)
> >> -            clean_dcache_guest_page(pfn, PAGE_SIZE);
> >> +        clean_dcache_guest_page(pfn, PAGE_SIZE);
> >>  
> >>          if (exec_fault) {
> >>              new_pte = kvm_s2pte_mkexec(new_pte);
> >> 
> 
> >I'm sorry, but I have to NAK this.
> 
> >You're papering over the fundamental issue that you're accessing a
> >cacheable alias of a non cacheable memory. The architecture is very
> >clear about why this doesn't work, and KVM implements the architecture.
> 
> 
> 
> I find that I just encounter the problem after the commit
> '15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .

This should really be a9c0e12ebee5 ("KVM: arm/arm64: Only clean the
dcache on translation fault").

> The commit contains "icache invalidation optimizations, improving VM
> startup time",it changes from unconditionally calling
> coherent_cache_guest_page(including dcache handle) to conditionally
> calling clean_dcache_guest_page.
> 
> I trace the display of vnc abnormally and find it generate data
> abort in vga address region with FSC_PERM,and it will not call
> clean_dcache_guest_page . So I think should recover to uncontionally
> calling clean_dcache_guest_page.

[I really hate ranting on a Saturday morning as the sun is finally out
and I could be relaxing in the garden, but hey, I guess that's the
fate of a kernel hacker who made the silly mistake of reading email on
a Saturday morning instead of being out in the garden...]

Even if you enabled dirty tracking on the VGA memory (and thus making
it RO to generate a permission fault), you would end-up cleaning to
the PoC *before* any write has been committed to the page. How useful
is that? You're also pretty lucky that this does a clean+invalidate
(rather than a clean which would be good enough), meaning that QEMU
will in turn miss in the cache (PIPT caches) and read some *stale*
data from memory.

Repeat that often enough, and yes, you can get some sort of
display. Is that the right thing to do? Hell no. You might just as
well have a userspace process thrashing the cache at the PoC, running
concurrently with your VM, it would have a similar effect. It may work
on your particular platform, in some specific conditions, but it just
doesn't work in general.

What you are describing is a long standing issue. VGA emulation never
worked on ARM, because it is fundamentally incompatible with the ARM
memory architecture. Read the various discussions on the list over the
past 4 years or so, read the various presentations I and others have
given about this problem and how to address it[1].

The *real* fix is a one-liner in the Linux VGA driver: map the VGA
memory as cacheable, and stop lying to the guest. Or if you want to
lie to the guest, perform cache maintenance from userspace in QEMU
based on the dirty tracking bitmap that it uses for the VGA memory.

Anything else is out of the scope of the architecture, and I'm not
going to add more crap to satisfy requirements that cannot be
implemented on real HW, let alone in a virtualized environment.

Thanks,

	M.

[1] http://events17.linuxfoundation.org/sites/events/files/slides/slides_10.pdf

-- 
Jazz is not dead, it just smell funny.