Re: [PATCH v5 3/7] KVM: x86: Allow to use bitmap in ring-based dirty page tracking

From: Oliver Upton <oliver.upton@linux.dev>
To: Peter Xu <peterx@redhat.com>
Cc: kvm@vger.kernel.org, catalin.marinas@arm.com,
	andrew.jones@linux.dev, dmatlack@google.com, will@kernel.org,
	shan.gavin@gmail.com, bgardon@google.com, kvmarm@lists.linux.dev,
	pbonzini@redhat.com, zhenyzha@redhat.com, shuah@kernel.org,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v5 3/7] KVM: x86: Allow to use bitmap in ring-based dirty page tracking
Date: Tue, 18 Oct 2022 10:40:56 +0300	[thread overview]
Message-ID: <Y05YiFgffF6rS1Hv@google.com> (raw)
In-Reply-To: <Y05X4o1TxxkvES9E@google.com>

On Tue, Oct 18, 2022 at 10:38:10AM +0300, Oliver Upton wrote:
> On Fri, Oct 14, 2022 at 12:55:35PM -0400, Peter Xu wrote:
> > On Tue, Oct 11, 2022 at 01:12:43AM +0000, Oliver Upton wrote:
> > > The VMM must know something about the architecture it is running on, as
> > > it calls KVM_DEV_ARM_ITS_SAVE_TABLES after all...
> > 
> > IIUC this is still a kernel impl detail to flush data into guest pages
> > within this ioctl, or am I wrong?
> 
> Somewhat...
> 
> The guest is assigning memory from the IPA space to back the ITS tables,
> but KVM maintains its own internal representation. It just so happens
> that we've conditioned userspace to be aware that ITS emulation is
> incoherent w.r.t. the guest memory that backs the tables.
> 
> > For example, I'm assuming it's safe to change KVM_DEV_ARM_ITS_SAVE_TABLES
> > impl one day to not flush data to guest memories, then the kernel should
> > also disable the ALLOW_BITMAP cap in the same patch, so that any old qemu
> > binary that supports arm64 dirty ring will naturally skip all the bitmap
> > ops and becoming the same as what it does with x86 when running on that new
> > kernel.  With implicit approach suggested, we need to modify QEMU.
> > 
> > Changing impl of KVM_DEV_ARM_ITS_SAVE_TABLES is probably not a good
> > example.. but just want to show what I meant.  Fundamentally it sounds
> > cleaner if it's the kernel that tells the user "okay you collected the
> > ring, but that's not enough; you need to collect the bitmap too", rather
> > than assuming the user app will always know what kvm did in details.  No
> > strong opinion though, as I could also have misunderstood how arm works.
> 
> I think the SAVE_TABLES ioctl is likely here to stay given the odd quirk
> that it really is guest memory, so we'll probably need the bitmap on
> arm64 for a long time. Even if we were to kill it, userspace would need
> to take a change anyway to switch to a new ITS migration mechanism.
> 
> If we ever get to the point that we can relax this restriction i think a
> flag on the BITMAP_WITH_TABLE cap that says "I don't actually set any

BITMAP_WITH_RING

> bits in the bitmap" would do. We shouldn't hide the cap entirely, as
> that would be ABI breakage for VMMs that expect bitmap+ring.
> 
> Thoughts?
> 
> --
> Thanks,
> Oliver
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm