From: Sean Christopherson <seanjc@google.com>
To: Gavin Shan <gshan@redhat.com>
Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
bgardon@google.com, andrew.jones@linux.dev, dmatlack@google.com,
will@kernel.org, shan.gavin@gmail.com, catalin.marinas@arm.com,
kvmarm@lists.linux.dev, pbonzini@redhat.com, zhenyzha@redhat.com,
shuah@kernel.org, kvmarm@lists.cs.columbia.edu,
ajones@ventanamicro.com
Subject: Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap
Date: Fri, 11 Nov 2022 23:00:11 +0000 [thread overview]
Message-ID: <Y27T+1Y8w0U6j63k@google.com> (raw)
In-Reply-To: <d11043b5-ff65-0461-146e-6353cf66f737@redhat.com>
On Sat, Nov 12, 2022, Gavin Shan wrote:
> Hi Marc,
>
> On 11/11/22 11:19 PM, Marc Zyngier wrote:
> > On Thu, 10 Nov 2022 23:47:41 +0000,
> > Gavin Shan <gshan@redhat.com> wrote:
> > But that I don't get. Or rather, I don't get the commit message that
> > matches this hunk. Do we want to catch the case where all of the
> > following are true:
> >
> > - we don't have a vcpu,
> > - we're allowed to log non-vcpu dirtying
> > - we *only* have the ring?
As written, no, because the resulting WARN will be user-triggerable. As mentioned
earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
logging is enabled with a bitmap, then this code can WARN.
> > If so, can we please capture that in the commit message?
> >
>
> Nice catch! This particular case needs to be warned explicitly. Without
> the patch, kernel crash is triggered. With this patch applied, the error
> or warning is dropped silently. We either check memslot->dirty_bitmap
> in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
> I personally the later one. Let me post a formal patch on top of your
> 'next' branch where the commit log will be improved accordingly.
As above, a full WARN is not a viable option unless ARM commits to rejecting
KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario. IMO, either reject the ITS save
or silently ignore the goof. Adding a pr_warn_ratelimited() to alert the user
that they shot themselves in the foot after the fact seems rather pointless if
KVM could have prevented the self-inflicted wound in the first place.
[*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: Sean Christopherson <seanjc@google.com>
To: Gavin Shan <gshan@redhat.com>
Cc: Marc Zyngier <maz@kernel.org>,
kvmarm@lists.linux.dev, kvmarm@lists.cs.columbia.edu,
kvm@vger.kernel.org, shuah@kernel.org, catalin.marinas@arm.com,
andrew.jones@linux.dev, ajones@ventanamicro.com,
bgardon@google.com, dmatlack@google.com, will@kernel.org,
suzuki.poulose@arm.com, alexandru.elisei@arm.com,
pbonzini@redhat.com, peterx@redhat.com, oliver.upton@linux.dev,
zhenyzha@redhat.com, shan.gavin@gmail.com
Subject: Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap
Date: Fri, 11 Nov 2022 23:00:11 +0000 [thread overview]
Message-ID: <Y27T+1Y8w0U6j63k@google.com> (raw)
Message-ID: <20221111230011.xa8cYB-3mhMS_kQNfKocnQr-J0b_nWiy28mnJZgi3TE@z> (raw)
In-Reply-To: <d11043b5-ff65-0461-146e-6353cf66f737@redhat.com>
On Sat, Nov 12, 2022, Gavin Shan wrote:
> Hi Marc,
>
> On 11/11/22 11:19 PM, Marc Zyngier wrote:
> > On Thu, 10 Nov 2022 23:47:41 +0000,
> > Gavin Shan <gshan@redhat.com> wrote:
> > But that I don't get. Or rather, I don't get the commit message that
> > matches this hunk. Do we want to catch the case where all of the
> > following are true:
> >
> > - we don't have a vcpu,
> > - we're allowed to log non-vcpu dirtying
> > - we *only* have the ring?
As written, no, because the resulting WARN will be user-triggerable. As mentioned
earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
logging is enabled with a bitmap, then this code can WARN.
> > If so, can we please capture that in the commit message?
> >
>
> Nice catch! This particular case needs to be warned explicitly. Without
> the patch, kernel crash is triggered. With this patch applied, the error
> or warning is dropped silently. We either check memslot->dirty_bitmap
> in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
> I personally the later one. Let me post a formal patch on top of your
> 'next' branch where the commit log will be improved accordingly.
As above, a full WARN is not a viable option unless ARM commits to rejecting
KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario. IMO, either reject the ITS save
or silently ignore the goof. Adding a pr_warn_ratelimited() to alert the user
that they shot themselves in the foot after the fact seems rather pointless if
KVM could have prevented the self-inflicted wound in the first place.
[*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
next prev parent reply other threads:[~2022-11-11 23:00 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-10 10:49 [PATCH v10 0/7] KVM: arm64: Enable ring-based dirty memory tracking Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 10:49 ` [PATCH v10 1/7] KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 10:49 ` [PATCH v10 2/7] KVM: Move declaration of kvm_cpu_dirty_log_size() to kvm_dirty_ring.h Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 10:49 ` [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 16:46 ` Sean Christopherson
2022-11-10 16:46 ` Sean Christopherson
2022-11-10 23:47 ` Gavin Shan
2022-11-10 23:47 ` Gavin Shan
2022-11-11 15:19 ` Marc Zyngier
2022-11-11 15:19 ` Marc Zyngier
2022-11-11 22:19 ` Gavin Shan
2022-11-11 22:19 ` Gavin Shan
2022-11-11 23:00 ` Sean Christopherson [this message]
2022-11-11 23:00 ` Sean Christopherson
2022-11-11 23:43 ` Gavin Shan
2022-11-11 23:43 ` Gavin Shan
2022-11-12 0:18 ` Sean Christopherson
2022-11-12 0:18 ` Sean Christopherson
2022-11-12 9:50 ` Gavin Shan
2022-11-12 9:50 ` Gavin Shan
2022-11-11 23:02 ` Marc Zyngier
2022-11-11 23:02 ` Marc Zyngier
2022-11-10 10:49 ` [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2023-01-15 11:20 ` Zenghui Yu
2023-01-15 11:56 ` Gavin Shan
2023-01-15 23:55 ` Gavin Shan
2023-01-16 4:09 ` Gavin Shan
2023-01-16 4:54 ` Zenghui Yu
2023-01-16 4:51 ` Zenghui Yu
2022-11-10 10:49 ` [PATCH v10 5/7] KVM: selftests: Use host page size to map ring buffer in dirty_log_test Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 10:49 ` [PATCH v10 6/7] KVM: selftests: Clear dirty ring states between two modes " Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 10:49 ` [PATCH v10 7/7] KVM: selftests: Automate choosing dirty ring size " Gavin Shan
2022-11-10 10:49 ` Gavin Shan
2022-11-10 13:21 ` [PATCH v10 0/7] KVM: arm64: Enable ring-based dirty memory tracking Marc Zyngier
2022-11-10 13:21 ` Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y27T+1Y8w0U6j63k@google.com \
--to=seanjc@google.com \
--cc=ajones@ventanamicro.com \
--cc=andrew.jones@linux.dev \
--cc=bgardon@google.com \
--cc=catalin.marinas@arm.com \
--cc=dmatlack@google.com \
--cc=gshan@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=kvmarm@lists.linux.dev \
--cc=maz@kernel.org \
--cc=pbonzini@redhat.com \
--cc=shan.gavin@gmail.com \
--cc=shuah@kernel.org \
--cc=will@kernel.org \
--cc=zhenyzha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.