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.
