Re: [PATCH 2/13] KVM: MIPS: Pass type of fault down to kvm_mips_map_page()

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]

Hi,

On Wed, Jan 18, 2017 at 04:18:46PM +0800, jiang.biao2@zte.com.cn wrote:
> Hi,
> 
> ＞ I presume you mean from the saved host cause register in the VCPU
> ＞ context (since intervening exceptions/interrupts will clobber the actual
> ＞ CP0 Cause register).
> ＞ 
> ＞ It directly needs to know whether it can get away with a read-only
> ＞ mapping, and although it directly depends on a GVA segment, it doesn't
> ＞ necessarily relate to a memory access made by the guest.
> ＞ 
> ＞ kvm_mips_map_page() is called via:
> ＞ 
> ＞ - kvm_mips_handle_kseg0_tlb_fault()
> ＞   for faults in guest KSeg0
> ＞ 
> ＞  - kvm_mips_handle_mapped_seg_tlb_fault()
> ＞    for faults in guest TLB mapped segments
> ＞ 
> ＞ From these functions:
> ＞ 
> ＞  - kvm_trap_emul_handle_tlb_mod() (write_fault = true)
> ＞   in response to a write to a read-only page (exccode = MOD)
> ＞ 
> ＞ - kvm_trap_emul_handle_tlb_miss() (write_fault = true or false)
> ＞   in response to a read or write when TLB mapping absent or invalid
> ＞   (exccode = TLBL/TLBS)
> ＞
> ＞ 
> ＞ So there is a many:many mapping from exccode to write_fault for these
> ＞ exccodes:
> ＞ 
> ＞  - CPU (CoProcessor Unusable)
> ＞    could be reading instruction or servicing a CACHE instruction
> ＞    (write_fault = false) or replacing an instruction (write_fault =
> ＞    true).
> 
> ＞  - MOD, TLBS, ADES
> ＞   could be the write itself (write_fault = true), or a read of the
> ＞   instruction triggering the exception or the prior branch instruction
> ＞   (write_fault = false).
> ＞ 
> Thanks for the detail, it is more complicted than I thought.
> 
> But there may be still bad smell from the long parameters, espacially from 
> 
> 
> bool type ones.  

Whats wrong with bool parameters?

It needs a GPA mapping created, either for a read or a write depending
on the caller. bool would seem ideally suited for just such a situation,
and in fact its exactly what the KVM GPA fault code path does to pass
whether the page needs to be writable:

kvm_mips_map_page() -> gfn_to_pfn_prot() -> __gfn_to_pfn_memslot() ->
hva_to_pfn() -> hva_to_pfn_slow().

so all this really does is extend that pattern up the other way as
necessary to be able to provide that information to gfn_to_pfn_prot().

Cheers
James

> 
> 
> Maybe there is better way to handle that, but I can not figure it out right now 
> 
> 
> because of the complexity.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]