[Qemu-devel] RE: [kvm-devel] [PATCH][UPDATE] kvm-userspace: sync icache for morearchitectures

[Zhang, Xiantao]

Christian Ehrhardt wrote:
> Zhang, Xiantao wrote:
>> Christian Ehrhardt wrote:
> <[...]
>>> @@ -2600,8 +2601,8 @@ void cpu_physical_memory_rw(target_phys_addr_t
>>>                      addr, uint8_t *buf, phys_ram_dirty[addr1 >>
>>>                          TARGET_PAGE_BITS] |= (0xff &
>>>                  ~CODE_DIRTY_FLAG); }
>>> -#ifdef __ia64__
>>> -           kvm_sync_icache((unsigned long)ptr, l);
>>> +#ifdef USE_KVM
>>> +           flush_icache_range((unsigned long)ptr, ((unsigned
long)ptr)+l);
>> 
>> Are you sure ia64 implement this function for applications ? I didn't
>> find it at my side, so I write it.
> What do you mean with "implement it for applications" ?
> Take a look at dyngen.h - it starts with the comment "dyngen helpers"
> and contains a lot of static functions to use them where you need.
> I don't see anything obvious that would prevent these functions from
> being built and the file has no own include statements which may
> change that. Therefor the include "dyngen.h" in the USE_KVM case
> should be enough to have this function available for
> cpu_physical_memory_rw in exec.c where we need it.

Good! I didn't notice this definition. Originally, I think you mean it
is a library functions. :) 

> 
> Hollis Blanchard wrote:
>> A comment to explain why the icache needs flushing only in the KVM
>> case would be useful. Other than that I'm fine with it.
>> 
>> Signed-off-by: Hollis Blanchard <address@hidden>
> AFAIK Plain qemu does not directly execute guest code on the
> processor, so the icache is not an issue for it.
> Qemu itself has the flush_icache_range function only as helper for the
> dynamic code generation.
> But we may now write executable guest code with our intercepted mmio
> handling that is directly executed when switching back to the guest
> context, therefore we need that invalidation in the kvm case.
> 
> For the case that I'm overlooking something in plain qemu, so that it
> might need it too I add address@hidden for comments from there,
> but currently I think to have it in #ifdef USE_KVM is the right way.
> 
> 
> P.S. Hollis did you mean you would like to see a comment in the code
> where that call takes place?