Re: [PATCH 3/5] live migration support for VM dirty log management

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

On Fri, Apr 18 2014 at 04:10:41 AM, Mario Smarduch <m.smarduch@samsung.com> 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.