Re: [PATCH v2 07/18] KVM/MIPS32: MMU/TLB operations for the Guest.

[Sanjay Lal <sanjayl@kymasys.com>]

On Feb 6, 2013, at 7:08 AM, Gleb Natapov wrote:

>> 
>> +static void kvm_mips_map_page(struct kvm *kvm, gfn_t gfn)
>> +{
>> +    pfn_t pfn;
>> +
>> +    if (kvm->arch.guest_pmap[gfn] != KVM_INVALID_PAGE)
>> +            return;
>> +
>> +    pfn =kvm_mips_gfn_to_pfn(kvm, gfn);
> This call should be in srcu read section since it access memory slots which
> are srcu protected. You should test with RCU debug enabled.

kvm_mips_gfn_to_pfn just maps to gfn_to_pfn. I don't see an instance where 
gfn_to_pfn is in a scru read section?

> 
>> 
>> +
>> +uint32_t kvm_get_inst(uint32_t *opc, struct kvm_vcpu *vcpu)
>> +{
>> +    uint32_t inst;
>> +    struct mips_coproc *cop0 __unused = vcpu->arch.cop0;
>> +    int index;
>> +    ulong paddr, flags;
>> +
>> +    if (KVM_GUEST_KSEGX((ulong) opc) < KVM_GUEST_KSEG0 ||
>> +        KVM_GUEST_KSEGX((ulong) opc) == KVM_GUEST_KSEG23) {
>> +            local_irq_save(flags);
>> +            index = kvm_mips_host_tlb_lookup(vcpu, (ulong) opc);
>> +            if (index >= 0) {
>> +                    inst = *(opc);
> Here and in some more places below you access __user memory. Shouldn't you
> use get_user() to access it? What prevents the kernel crash by access fault 
> here
> if userspace remaps the memory to be non-readable? Hmm, may be it uses
> guest translation here so it cannot happen, but still, sparse will not
> be happy and kvm_mips_translate_guest_kseg0_to_hpa() case below uses
> host translation anyway.
> 
Actually, I don't need the __user declaration in most cases, since KVM/MIPS 
handles mapping the page (if needed) and does not rely on the usual kernel 
mechanisms.