Hi Sean,
On 11/12/22 7:00 AM, Sean Christopherson wrote:
On Sat, Nov 12, 2022, Gavin Shan wrote:
On 11/11/22 11:19 PM, Marc Zyngier wrote:
On Thu, 10 Nov 2022 23:47:41 +0000,
Gavin Shan <gshan@xxxxxxxxxx> 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.
I assume you're saying to reject the command when dirty ring is enabled __without__
a bitmap. vgic/its is the upper layer of dirty dirty. To me, it's a bad idea for the
upper layer needs to worry too much about the lower layer.
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+@xxxxxxxxxx
Without a message printed by WARN, kernel crash or pr_warn_ratelimited(), it
will be hard for userspace to know what's going on, because the dirty bits
have been dropped silently. I think we still survive since we have WARN
message for other known cases where no running vcpu context exists.
So if I'm correct, what we need to do is to improve the commit message to
address Marc's concerns here? :)
Thanks,
Gavin
