On Thu, 2022-10-13 at 21:12 +0000, Sean Christopherson wrote: > From: Michal Luczaj <mhal@xxxxxxx> > > Make the length of a gfn=>pfn cache an immutable property of the cache > to cleanup the APIs and avoid potential bugs, e.g calling check() with a > larger size than refresh() could put KVM into an infinite loop. Hm, that's a strange hypothetical bug to be worried about, given the pattern is usually to have the check() and refresh() within a few lines of each other with just atomicity/locking stuff in between them. I won't fight for it, but I quite liked the idea that each user of a GPC would know how much space *it* is going to access, and provide that length as a required parameter. I do note you've added a WARN_ON to one such user, and that's great — but overall, this patch makes that checking *optional* instead of mandatory. > All current (and anticipated future) users access the cache with a > predetermined size, which isn't a coincidence as using a dedicated cache > really only make sense when the access pattern is "fixed". In fixing up the runstate area, I've made that not true. Not only does the runstate area change size at runtime if the guest changes between 32-bit and 64-bit mode, but it also now uses *two* GPCs to cope with a region that crosses a page boundary, and the size of the first therefore changes according to how much fits on the tail of the page. > Add a WARN in kvm_setup_guest_pvclock() to assert that the offset+size > matches the length of the cache, both to make it more obvious that the > length really is immutable in that case, and to detect future bugs. ... > @@ -3031,13 +3030,13 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v, > struct pvclock_vcpu_time_info *guest_hv_clock; > unsigned long flags; > > + WARN_ON_ONCE(gpc->len != offset + sizeof(*guest_hv_clock)); > + That ought to be 'gpc->len < offset + sizeof(*guest_hv_clock)' I think? In the case where we are writing a clock *within* a mapped Xen vcpu_info structure, it doesn't have to be at the *end* of that structure. I think the xen_shinfo_test should have caught that?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
