From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH 3/5] live migration support for VM dirty log management Date: Fri, 18 Apr 2014 09:23:07 +0100 Message-ID: <86sipb59n8.fsf@arm.com> References: <534F2F9B.80004@samsung.com> <87a9bkxu5p.fsf@approximate.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain Cc: "kvmarm\@lists.cs.columbia.edu" , "christoffer.dall\@linaro.org" , "kvm\@vger.kernel.org" , =?utf-8?B?7J207KCV7ISd?= , =?utf-8?B?7KCV7ISx7KeE?= To: Mario Smarduch Return-path: Received: from fw-tnat.austin.arm.com ([217.140.110.23]:51266 "EHLO collaborate-mta1.arm.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751845AbaDRIXY (ORCPT ); Fri, 18 Apr 2014 04:23:24 -0400 In-Reply-To: (Mario Smarduch's message of "Fri, 18 Apr 2014 04:10:41 +0100") Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Apr 18 2014 at 04:10:41 AM, Mario Smarduch wrote: > MZ> So let's play the difference game with x86: > >> int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log > > > kvm_vm_ioctl_get_dirty_log() is identical now to x86 version moved it > to kvm_main.c, to make it generic, it's declared weak. Do I go into > x86 and remove that function? > > Or leave it to x86 folks to do it? No, this must be part of your patch series. >> + } >> + >> + if (is_dirty) >> + kvm_tlb_flush_vm(kvm); > > MZ> This can be easily abstracted to be a kvm_flush_remote_tlbs on > MZ> x86, and a HW broadcast on ARM. > > Kvm_tlb_flush_vm() is replaced with kvm_flush_remote_tlbs() I made that > function weak and declared a ARM version, in arm mmu.c > > The current version sends IPIs to vCPU running the guest, ARMv7+ > does not need that. Is that ok? We don't need any of that. You should have akvm_flush_remote_tlb that is essentially a call to kvm_tlb_flush_vmid. > > >> + /* walk set bits in the mask and write protect corresponding pages */ >> + while (mask) { >> + ipa = (slot->base_gfn + gfn_offset + __ffs(mask)) << PAGE_SHIFT; >> + pgd = pgdp + pgd_index(ipa); >> + if (!pgd_present(*pgd)) >> + goto update_mask; > > MZ> I think something is wrong in your logic. If there is no PGD, it > MZ> means a whole 1GB isn't present. Yet you're just clearing one bit > MZ> from the mask and doing it again. As you're only looking at > MZ> BITS_PER_LONG MZ> contiguous pages at a time, it is likely that > MZ> the same thing will happen for the other pages, and you're just > MZ> wasting precious CPU cycles here. > > Yes this is grossly inefficient, I updated it to walk ptes only, after > first determining if it straddles a pmd. > Should mostly be pte walks with maybe one PMD walk but unlikely. Yes, that's what I thought. > >> + new_pte = pfn_pte(pte_pfn(*pte), PAGE_S2); >> + *pte = new_pte; > > MZ> I'd like to see these two lines in a separate function (something > MZ> like "stage2_mark_pte_ro")... > > Yes ok. Thanks. M. -- Jazz is not dead. It just smells funny.