| Message ID | 20200601115957.1581250-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Dirty ring interface | expand |
On Mon, Jun 01, 2020 at 07:59:45AM -0400, Peter Xu wrote: > Cache the address space ID just like the slot ID. It will be used in > order to fill in the dirty ring entries. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/kvm_host.h | 1 + > virt/kvm/kvm_main.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 01276e3d01b9..5e7bbaf7a36b 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -346,6 +346,7 @@ struct kvm_memory_slot { > unsigned long userspace_addr; > u32 flags; > short id; > + u16 as_id; > }; > > static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot) > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 74bdb7bf3295..ebdd98a30e82 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1243,6 +1243,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > if (!mem->memory_size) > return kvm_delete_memslot(kvm, mem, &old, as_id); This technically needs to set as_id in the deleted memslot. I highly doubt it will ever matter from a functionality perspective, but it'd be confusing to encounter a memslot whose as_id did not match that of its owner. > + new.as_id = as_id; > new.id = id; > new.base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; > new.npages = mem->memory_size >> PAGE_SHIFT; > -- > 2.26.2 >
On Thu, Jul 02, 2020 at 04:08:49PM -0700, Sean Christopherson wrote: > On Mon, Jun 01, 2020 at 07:59:45AM -0400, Peter Xu wrote: > > Cache the address space ID just like the slot ID. It will be used in > > order to fill in the dirty ring entries. > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/linux/kvm_host.h | 1 + > > virt/kvm/kvm_main.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 01276e3d01b9..5e7bbaf7a36b 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -346,6 +346,7 @@ struct kvm_memory_slot { > > unsigned long userspace_addr; > > u32 flags; > > short id; > > + u16 as_id; > > }; > > > > static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 74bdb7bf3295..ebdd98a30e82 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1243,6 +1243,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > > if (!mem->memory_size) > > return kvm_delete_memslot(kvm, mem, &old, as_id); > > This technically needs to set as_id in the deleted memslot. I highly doubt > it will ever matter from a functionality perspective, but it'd be confusing > to encounter a memslot whose as_id did not match that of its owner. Yeah it shouldn't matter because as_id is directly passed in to look up the pointer of kvm_memslots in kvm_delete_memslot, and memslot->as_id shouldn't be further referenced. I can add a comment above if this can clarify things a bit: + u16 as_id; /* cache of as_id; only valid if npages != 0 */ Thanks,
On Fri, Jul 03, 2020 at 02:41:22PM -0400, Peter Xu wrote: > On Thu, Jul 02, 2020 at 04:08:49PM -0700, Sean Christopherson wrote: > > On Mon, Jun 01, 2020 at 07:59:45AM -0400, Peter Xu wrote: > > > Cache the address space ID just like the slot ID. It will be used in > > > order to fill in the dirty ring entries. > > > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > include/linux/kvm_host.h | 1 + > > > virt/kvm/kvm_main.c | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > index 01276e3d01b9..5e7bbaf7a36b 100644 > > > --- a/include/linux/kvm_host.h > > > +++ b/include/linux/kvm_host.h > > > @@ -346,6 +346,7 @@ struct kvm_memory_slot { > > > unsigned long userspace_addr; > > > u32 flags; > > > short id; > > > + u16 as_id; > > > }; > > > > > > static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot) > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index 74bdb7bf3295..ebdd98a30e82 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -1243,6 +1243,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > if (!mem->memory_size) > > > return kvm_delete_memslot(kvm, mem, &old, as_id); > > > > This technically needs to set as_id in the deleted memslot. I highly doubt > > it will ever matter from a functionality perspective, but it'd be confusing > > to encounter a memslot whose as_id did not match that of its owner. > > Yeah it shouldn't matter because as_id is directly passed in to look up the > pointer of kvm_memslots in kvm_delete_memslot, and memslot->as_id shouldn't be > further referenced. > > I can add a comment above if this can clarify things a bit: > > + u16 as_id; /* cache of as_id; only valid if npages != 0 */ Why not just set it? It's a single line of code, and there's more than one "shouldn't" in the above.
On Mon, Jul 06, 2020 at 11:17:32PM -0700, Sean Christopherson wrote: > On Fri, Jul 03, 2020 at 02:41:22PM -0400, Peter Xu wrote: > > On Thu, Jul 02, 2020 at 04:08:49PM -0700, Sean Christopherson wrote: > > > On Mon, Jun 01, 2020 at 07:59:45AM -0400, Peter Xu wrote: > > > > Cache the address space ID just like the slot ID. It will be used in > > > > order to fill in the dirty ring entries. > > > > > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > include/linux/kvm_host.h | 1 + > > > > virt/kvm/kvm_main.c | 1 + > > > > 2 files changed, 2 insertions(+) > > > > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > > index 01276e3d01b9..5e7bbaf7a36b 100644 > > > > --- a/include/linux/kvm_host.h > > > > +++ b/include/linux/kvm_host.h > > > > @@ -346,6 +346,7 @@ struct kvm_memory_slot { > > > > unsigned long userspace_addr; > > > > u32 flags; > > > > short id; > > > > + u16 as_id; > > > > }; > > > > > > > > static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > > index 74bdb7bf3295..ebdd98a30e82 100644 > > > > --- a/virt/kvm/kvm_main.c > > > > +++ b/virt/kvm/kvm_main.c > > > > @@ -1243,6 +1243,7 @@ int __kvm_set_memory_region(struct kvm *kvm, > > > > if (!mem->memory_size) > > > > return kvm_delete_memslot(kvm, mem, &old, as_id); > > > > > > This technically needs to set as_id in the deleted memslot. I highly doubt > > > it will ever matter from a functionality perspective, but it'd be confusing > > > to encounter a memslot whose as_id did not match that of its owner. > > > > Yeah it shouldn't matter because as_id is directly passed in to look up the > > pointer of kvm_memslots in kvm_delete_memslot, and memslot->as_id shouldn't be > > further referenced. > > > > I can add a comment above if this can clarify things a bit: > > > > + u16 as_id; /* cache of as_id; only valid if npages != 0 */ > > Why not just set it? Because the value is useless even if set? :) You mean in kvm_delete_memslot(), am I right? > It's a single line of code, and there's more than one > "shouldn't" in the above. If you want, I can both set it and add the comment. Thanks,
On Tue, Jul 07, 2020 at 03:50:09PM -0400, Peter Xu wrote: > On Mon, Jul 06, 2020 at 11:17:32PM -0700, Sean Christopherson wrote: > > On Fri, Jul 03, 2020 at 02:41:22PM -0400, Peter Xu wrote: > > > On Thu, Jul 02, 2020 at 04:08:49PM -0700, Sean Christopherson wrote: > > > > This technically needs to set as_id in the deleted memslot. I highly doubt > > > > it will ever matter from a functionality perspective, but it'd be confusing > > > > to encounter a memslot whose as_id did not match that of its owner. > > > > > > Yeah it shouldn't matter because as_id is directly passed in to look up the > > > pointer of kvm_memslots in kvm_delete_memslot, and memslot->as_id shouldn't be > > > further referenced. > > > > > > I can add a comment above if this can clarify things a bit: > > > > > > + u16 as_id; /* cache of as_id; only valid if npages != 0 */ > > > > Why not just set it? > > Because the value is useless even if set? :) It's useless when things go according to plan, but I can see it being useful if there's a bug that leads to consumption of a deleted memslot. Maybe not "useful" so much as "not misleading". > You mean in kvm_delete_memslot(), am I right? Yes. > > It's a single line of code, and there's more than one > > "shouldn't" in the above. > > If you want, I can both set it and add the comment. Thanks, Why bother with the comment? It'd be wrong in the sense that the as_id is always valid/accurate, even if npages == 0.
On Tue, Jul 07, 2020 at 12:56:58PM -0700, Sean Christopherson wrote: > > > It's a single line of code, and there's more than one > > > "shouldn't" in the above. > > > > If you want, I can both set it and add the comment. Thanks, > > Why bother with the comment? It'd be wrong in the sense that the as_id is > always valid/accurate, even if npages == 0. Sorry I'm confused.. when npages==0, why as_id field is meaningful? Even if the id field is meaningless after the slot is successfully removed, or am I wrong? My understanding is that after your dynamic slot work, we'll only have at most one extra memslot that was just removed, and that slot should be meaningless as a whole. Feel free to correct me.
On Tue, Jul 07, 2020 at 04:15:08PM -0400, Peter Xu wrote: > On Tue, Jul 07, 2020 at 12:56:58PM -0700, Sean Christopherson wrote: > > > > It's a single line of code, and there's more than one > > > > "shouldn't" in the above. > > > > > > If you want, I can both set it and add the comment. Thanks, > > > > Why bother with the comment? It'd be wrong in the sense that the as_id is > > always valid/accurate, even if npages == 0. > > Sorry I'm confused.. when npages==0, why as_id field is meaningful? Even if > the id field is meaningless after the slot is successfully removed, or am I > wrong? > > My understanding is that after your dynamic slot work, we'll only have at most > one extra memslot that was just removed, and that slot should be meaningless as > a whole. Feel free to correct me. Your understanding is correct. What I'm saying is that if something goes awry and the memslots need to be debugged, having accurate info for that one defunct memslot could be helpful, if only to not confuse a future debugger that doesn't fully understand memslots or address spaces. Sure, it could be manually added back in for debug, but it's literally a single line of code to carry and it avoids the need for a special comment.
On Tue, Jul 07, 2020 at 01:26:23PM -0700, Sean Christopherson wrote: > On Tue, Jul 07, 2020 at 04:15:08PM -0400, Peter Xu wrote: > > On Tue, Jul 07, 2020 at 12:56:58PM -0700, Sean Christopherson wrote: > > > > > It's a single line of code, and there's more than one > > > > > "shouldn't" in the above. > > > > > > > > If you want, I can both set it and add the comment. Thanks, > > > > > > Why bother with the comment? It'd be wrong in the sense that the as_id is > > > always valid/accurate, even if npages == 0. > > > > Sorry I'm confused.. when npages==0, why as_id field is meaningful? Even if > > the id field is meaningless after the slot is successfully removed, or am I > > wrong? > > > > My understanding is that after your dynamic slot work, we'll only have at most > > one extra memslot that was just removed, and that slot should be meaningless as > > a whole. Feel free to correct me. > > Your understanding is correct. What I'm saying is that if something goes > awry and the memslots need to be debugged, having accurate info for that one > defunct memslot could be helpful, if only to not confuse a future debugger > that doesn't fully understand memslots or address spaces. Sure, it could be > manually added back in for debug, but it's literally a single line of code > to carry and it avoids the need for a special comment. Sure, will do. But again, I hope you allow me to add at least some comment. To me, it's still weird to set these in a destroying memslot...
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 01276e3d01b9..5e7bbaf7a36b 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -346,6 +346,7 @@ struct kvm_memory_slot { unsigned long userspace_addr; u32 flags; short id; + u16 as_id; }; static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 74bdb7bf3295..ebdd98a30e82 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1243,6 +1243,7 @@ int __kvm_set_memory_region(struct kvm *kvm, if (!mem->memory_size) return kvm_delete_memslot(kvm, mem, &old, as_id); + new.as_id = as_id; new.id = id; new.base_gfn = mem->guest_phys_addr >> PAGE_SHIFT; new.npages = mem->memory_size >> PAGE_SHIFT;
Cache the address space ID just like the slot ID. It will be used in order to fill in the dirty ring entries. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 1 + 2 files changed, 2 insertions(+)