Re: [PATCH v3 3/3] KVM: selftests: Add selftests for dirty quota throttling

[Sean Christopherson <seanjc@google.com>]

On Thu, Apr 28, 2022, Shivam Kumar wrote:
> 
> On 18/04/22 9:47 pm, Sean Christopherson wrote:
> > On Mon, Apr 18, 2022, Shivam Kumar wrote:
> > > > > +void vcpu_handle_dirty_quota_exit(struct kvm_run *run,
> > > > > +            uint64_t test_dirty_quota_increment)
> > > > > +{
> > > > > +    uint64_t quota = run->dirty_quota_exit.quota;
> > > > > +    uint64_t count = run->dirty_quota_exit.count;
> > > > > +
> > > > > +    /*
> > > > > +     * Due to PML, number of pages dirtied by the vcpu can exceed its dirty
> > > > > +     * quota by PML buffer size.
> > > > > +     */
> > > > > +    TEST_ASSERT(count <= quota + PML_BUFFER_SIZE, "Invalid number of pages
> > > > > +        dirtied: count=%"PRIu64", quota=%"PRIu64"\n", count, quota);
> > > Sean, I don't think this would be valid anymore because as you mentioned, the
> > > vcpu can dirty multiple pages in one vmexit. I could use your help here.
> > TL;DR: Should be fine, but s390 likely needs an exception.
> > 
> > Practically speaking the 512 entry fuzziness is all but guaranteed to prevent
> > false failures.
> > 
> > But, unconditionally allowing for overflow of 512 entries also means the test is
> > unlikely to ever detect violations.  So to provide meaningful coverage, this needs
> > to allow overflow if and only if PML is enabled.
> > 
> > And that brings us back to false failures due to _legitimate_ scenarios where a vCPU
> > can dirty multiple pages.  Emphasis on legitimate, because except for an s390 edge
> > case, I don't think this test's guest code does anything that would dirty multiple
> > pages in a single instruction, e.g. there's no emulation, shouldn't be any descriptor
> > table side effects, etc...  So unless I'm missing something, KVM should be able to
> > precisely handle the core run loop.
> > 
> > s390 does appear to have a caveat:
> > 
> > 	/*
> > 	 * On s390x, all pages of a 1M segment are initially marked as dirty
> > 	 * when a page of the segment is written to for the very first time.
> > 	 * To compensate this specialty in this test, we need to touch all
> > 	 * pages during the first iteration.
> > 	 */
> > 	for (i = 0; i < guest_num_pages; i++) {
> > 		addr = guest_test_virt_mem + i * guest_page_size;
> > 		*(uint64_t *)addr = READ_ONCE(iteration);
> > 	}
> > 
> > IIUC, subsequent iterations will be ok, but the first iteration needs to allow
> > for overflow of 256 (AFAIK the test only uses 4kb pages on s390).
> Hi Sean, need an advice from your side before sending v4. In my opinion, I
> should organise my patchset in a way that the first n-1 patches have changes
> for x86 and the last patch has the changes for s390 and arm64. This can help
> me move forward for the x86 arch and get help and reviews from s390 and
> arm64 maintainers in parallel. Please let me know if this makes sense.

Works for me.  It probably makes sense to split s390 and arm64 too, that way you
don't need a v5 if one wants the feature and the other does not.