| Message ID | 1516280198-25386-2-git-send-email-imbrenda@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/18/2018 01:56 PM, Claudio Imbrenda wrote: > These are some utilty functions that will be used later on for storage > attributes migration. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 32 ++++++++++++++++++++++++++++++++ > arch/s390/kvm/kvm-s390.h | 6 ++++++ > 2 files changed, 38 insertions(+) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 6f17031..5a69334 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1512,6 +1512,38 @@ static long kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) > #define KVM_S390_CMMA_SIZE_MAX ((u32)KVM_S390_SKEYS_MAX) > > /* > + * Similar to gfn_to_memslot, but returns a memslot also when the address falls gfn_to_memslot returns a memslot, this returns an int? > + * in a hole. In that case a memslot near the hole is returned. Can you please clarify that statement? Will it be the slot that is closest in terms of bytes or what? Maybe also provide a use case and an example > + */ > +static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn) > +{ > + struct kvm_memslots *slots = kvm_memslots(kvm); > + int start = 0, end = slots->used_slots; > + int slot = atomic_read(&slots->lru_slot); > + struct kvm_memory_slot *memslots = slots->memslots; > + > + if (gfn >= memslots[slot].base_gfn && > + gfn < memslots[slot].base_gfn + memslots[slot].npages) > + return slot; > + > + while (start < end) { > + slot = start + (end - start) / 2; > + > + if (gfn >= memslots[slot].base_gfn) > + end = slot; > + else > + start = slot + 1; > + } > + > + if (gfn >= memslots[start].base_gfn && > + gfn < memslots[start].base_gfn + memslots[start].npages) { > + atomic_set(&slots->lru_slot, start); > + } Another question for Paolo/Radim. In case we need such function, would it be better in common code (kvm_main.c) > + > + return start; > +} > + > +/* > * This function searches for the next page with dirty CMMA attributes, and > * saves the attributes in the buffer up to either the end of the buffer or > * until a block of at least KVM_S390_MAX_BIT_DISTANCE clean bits is found; > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h > index 04a3e91..b86c1ad 100644 > --- a/arch/s390/kvm/kvm-s390.h > +++ b/arch/s390/kvm/kvm-s390.h > @@ -183,6 +183,12 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm) > return kvm->arch.user_cpu_state_ctrl != 0; > } > > +static inline unsigned long *cmma_bitmap(struct kvm_memory_slot *slot) > +{ > + return slot->dirty_bitmap + > + kvm_dirty_bitmap_bytes(slot) / sizeof(*slot->dirty_bitmap); > +} > + Hmmm, in virt/kvm/kvm_main.c we do have in kvm_get_dirty_log_protect n = kvm_dirty_bitmap_bytes(memslot); dirty_bitmap_buffer = dirty_bitmap + n / sizeof(long); Does it make sense to have that helper common (and call it maybe unsigned long *kvm_get_shadow_dirty(struct kvm_memory_slot *slot)
On 01/22/2018 10:08 AM, Christian Borntraeger wrote: >> +static inline unsigned long *cmma_bitmap(struct kvm_memory_slot *slot) >> +{ >> + return slot->dirty_bitmap + >> + kvm_dirty_bitmap_bytes(slot) / sizeof(*slot->dirty_bitmap); >> +} >> + > > Hmmm, in virt/kvm/kvm_main.c we do have in kvm_get_dirty_log_protect > > > n = kvm_dirty_bitmap_bytes(memslot); > > dirty_bitmap_buffer = dirty_bitmap + n / sizeof(long); FWIW, I find this variant easier to read. > > > Does it make sense to have that helper common (and call it maybe > > unsigned long *kvm_get_shadow_dirty(struct kvm_memory_slot *slot) >
On Mon, 22 Jan 2018 10:08:47 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: [...] > > /* > > + * Similar to gfn_to_memslot, but returns a memslot also when the > > address falls > > gfn_to_memslot returns a memslot, this returns an int? true, I have to update the comment > > + * in a hole. In that case a memslot near the hole is returned. > > Can you please clarify that statement? Will it be the slot that is > closest in terms of bytes or what? Maybe also provide a use case and > an example I think that just any of the two memslots (before and after the hole) can be returned, not necessarily the closest one. The logic that uses this function takes this into account. Maybe I can change the description to "the index of any of the memoslots bordering the hole is returned" > > + */ > > +static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn) > > +{ > > + struct kvm_memslots *slots = kvm_memslots(kvm); > > + int start = 0, end = slots->used_slots; > > + int slot = atomic_read(&slots->lru_slot); > > + struct kvm_memory_slot *memslots = slots->memslots; > > + > > + if (gfn >= memslots[slot].base_gfn && > > + gfn < memslots[slot].base_gfn + memslots[slot].npages) > > + return slot; > > + > > + while (start < end) { > > + slot = start + (end - start) / 2; > > + > > + if (gfn >= memslots[slot].base_gfn) > > + end = slot; > > + else > > + start = slot + 1; > > + } > > + > > + if (gfn >= memslots[start].base_gfn && > > + gfn < memslots[start].base_gfn + > > memslots[start].npages) { > > + atomic_set(&slots->lru_slot, start); > > + } > > Another question for Paolo/Radim. In case we need such function, would > it be better in common code (kvm_main.c) > > + > > + return start; > > > > +} > > + > > +/* > > * This function searches for the next page with dirty CMMA > > attributes, and > > * saves the attributes in the buffer up to either the end of the > > buffer or > > * until a block of at least KVM_S390_MAX_BIT_DISTANCE clean bits > > is found; diff --git a/arch/s390/kvm/kvm-s390.h > > b/arch/s390/kvm/kvm-s390.h index 04a3e91..b86c1ad 100644 > > --- a/arch/s390/kvm/kvm-s390.h > > +++ b/arch/s390/kvm/kvm-s390.h > > @@ -183,6 +183,12 @@ static inline int > > kvm_s390_user_cpu_state_ctrl(struct kvm *kvm) return > > kvm->arch.user_cpu_state_ctrl != 0; } > > > > +static inline unsigned long *cmma_bitmap(struct kvm_memory_slot > > *slot) +{ > > + return slot->dirty_bitmap + > > + kvm_dirty_bitmap_bytes(slot) / > > sizeof(*slot->dirty_bitmap); +} > > + > > Hmmm, in virt/kvm/kvm_main.c we do have in kvm_get_dirty_log_protect > > > n = kvm_dirty_bitmap_bytes(memslot); > > dirty_bitmap_buffer = dirty_bitmap + n / sizeof(long); > > > Does it make sense to have that helper common (and call it maybe > > unsigned long *kvm_get_shadow_dirty(struct kvm_memory_slot *slot) I hadn't noticed it; it surely makes sense to share the code.
2018-01-22 10:08+0100, Christian Borntraeger: > On 01/18/2018 01:56 PM, Claudio Imbrenda wrote: > > These are some utilty functions that will be used later on for storage > > attributes migration. > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > > --- > > arch/s390/kvm/kvm-s390.c | 32 ++++++++++++++++++++++++++++++++ > > arch/s390/kvm/kvm-s390.h | 6 ++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index 6f17031..5a69334 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -1512,6 +1512,38 @@ static long kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) > > #define KVM_S390_CMMA_SIZE_MAX ((u32)KVM_S390_SKEYS_MAX) > > > > /* > > + * Similar to gfn_to_memslot, but returns a memslot also when the address falls > > gfn_to_memslot returns a memslot, this returns an int? > > > + * in a hole. In that case a memslot near the hole is returned. > > Can you please clarify that statement? Will it be the slot that is closest > in terms of bytes or what? Maybe also provide a use case and an example > > + */ > > +static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn) > > +{ > > + struct kvm_memslots *slots = kvm_memslots(kvm); > > + int start = 0, end = slots->used_slots; > > + int slot = atomic_read(&slots->lru_slot); > > + struct kvm_memory_slot *memslots = slots->memslots; > > + > > + if (gfn >= memslots[slot].base_gfn && > > + gfn < memslots[slot].base_gfn + memslots[slot].npages) > > + return slot; > > + > > + while (start < end) { > > + slot = start + (end - start) / 2; > > + > > + if (gfn >= memslots[slot].base_gfn) > > + end = slot; > > + else > > + start = slot + 1; > > + } > > + > > + if (gfn >= memslots[start].base_gfn && > > + gfn < memslots[start].base_gfn + memslots[start].npages) { > > + atomic_set(&slots->lru_slot, start); > > + } > > Another question for Paolo/Radim. In case we need such function, would > it be better in common code (kvm_main.c) Please keep it in s390 and don't do it in the best case. The function doesn't look reusable. If a gfn isn't in a slot, then we shouldn't be using slots to work with it. I don't understand why CMMA need adresses in the gaps, so I can't provide a good idea here -- maybe we can add slots where needed?
On Thu, 25 Jan 2018 17:37:22 +0100 Radim Krčmář <rkrcmar@redhat.com> wrote: > 2018-01-22 10:08+0100, Christian Borntraeger: > > On 01/18/2018 01:56 PM, Claudio Imbrenda wrote: > > > These are some utilty functions that will be used later on for > > > storage attributes migration. > > > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > > > --- > > > arch/s390/kvm/kvm-s390.c | 32 ++++++++++++++++++++++++++++++++ > > > arch/s390/kvm/kvm-s390.h | 6 ++++++ > > > 2 files changed, 38 insertions(+) > > > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > > index 6f17031..5a69334 100644 > > > --- a/arch/s390/kvm/kvm-s390.c > > > +++ b/arch/s390/kvm/kvm-s390.c > > > @@ -1512,6 +1512,38 @@ static long kvm_s390_set_skeys(struct kvm > > > *kvm, struct kvm_s390_skeys *args) #define KVM_S390_CMMA_SIZE_MAX > > > ((u32)KVM_S390_SKEYS_MAX) > > > > > > /* > > > + * Similar to gfn_to_memslot, but returns a memslot also when > > > the address falls > > > > gfn_to_memslot returns a memslot, this returns an int? > > > > > + * in a hole. In that case a memslot near the hole is returned. > > > > Can you please clarify that statement? Will it be the slot that is > > closest in terms of bytes or what? Maybe also provide a use case > > and an example > > > + */ > > > +static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn) > > > +{ > > > + struct kvm_memslots *slots = kvm_memslots(kvm); > > > + int start = 0, end = slots->used_slots; > > > + int slot = atomic_read(&slots->lru_slot); > > > + struct kvm_memory_slot *memslots = slots->memslots; > > > + > > > + if (gfn >= memslots[slot].base_gfn && > > > + gfn < memslots[slot].base_gfn + > > > memslots[slot].npages) > > > + return slot; > > > + > > > + while (start < end) { > > > + slot = start + (end - start) / 2; > > > + > > > + if (gfn >= memslots[slot].base_gfn) > > > + end = slot; > > > + else > > > + start = slot + 1; > > > + } > > > + > > > + if (gfn >= memslots[start].base_gfn && > > > + gfn < memslots[start].base_gfn + > > > memslots[start].npages) { > > > + atomic_set(&slots->lru_slot, start); > > > + } > > > > Another question for Paolo/Radim. In case we need such function, > > would it be better in common code (kvm_main.c) > > Please keep it in s390 and don't do it in the best case. > The function doesn't look reusable. If a gfn isn't in a slot, then we > shouldn't be using slots to work with it. > > I don't understand why CMMA need adresses in the gaps, so I can't > provide a good idea here -- maybe we can add slots where needed? We actually don't need addresses in the gap. What we want instead is the first address after the gap, and that is not possible if we simply return a NULL when we hit a hole, like gfn_to_memslot would do. That's because the current code returns (almost) only the dirty values, so if userspace starts at a "clean" address, the first clean values are skipped, and the start address updated accordingly. If the start address is in a hole, we want to skip the hole and start at the next valid address, but to do that we need a slot next to the hole. an alternative would be to limit the interface to work inside memslots, thus returning an error when the starting address is in a hole, like we are already doing in the "peek" case. This will require changes in userspace. Not sure we want to do that, although until the recent patch from Christian, multi-slot guests were broken anyway.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 6f17031..5a69334 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1512,6 +1512,38 @@ static long kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args) #define KVM_S390_CMMA_SIZE_MAX ((u32)KVM_S390_SKEYS_MAX) /* + * Similar to gfn_to_memslot, but returns a memslot also when the address falls + * in a hole. In that case a memslot near the hole is returned. + */ +static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn) +{ + struct kvm_memslots *slots = kvm_memslots(kvm); + int start = 0, end = slots->used_slots; + int slot = atomic_read(&slots->lru_slot); + struct kvm_memory_slot *memslots = slots->memslots; + + if (gfn >= memslots[slot].base_gfn && + gfn < memslots[slot].base_gfn + memslots[slot].npages) + return slot; + + while (start < end) { + slot = start + (end - start) / 2; + + if (gfn >= memslots[slot].base_gfn) + end = slot; + else + start = slot + 1; + } + + if (gfn >= memslots[start].base_gfn && + gfn < memslots[start].base_gfn + memslots[start].npages) { + atomic_set(&slots->lru_slot, start); + } + + return start; +} + +/* * This function searches for the next page with dirty CMMA attributes, and * saves the attributes in the buffer up to either the end of the buffer or * until a block of at least KVM_S390_MAX_BIT_DISTANCE clean bits is found; diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h index 04a3e91..b86c1ad 100644 --- a/arch/s390/kvm/kvm-s390.h +++ b/arch/s390/kvm/kvm-s390.h @@ -183,6 +183,12 @@ static inline int kvm_s390_user_cpu_state_ctrl(struct kvm *kvm) return kvm->arch.user_cpu_state_ctrl != 0; } +static inline unsigned long *cmma_bitmap(struct kvm_memory_slot *slot) +{ + return slot->dirty_bitmap + + kvm_dirty_bitmap_bytes(slot) / sizeof(*slot->dirty_bitmap); +} + /* implemented in interrupt.c */ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu); void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu);
These are some utilty functions that will be used later on for storage attributes migration. Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> --- arch/s390/kvm/kvm-s390.c | 32 ++++++++++++++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 6 ++++++ 2 files changed, 38 insertions(+)