On Thu, 25 Jan 2018 17:37:22 +0100
Radim Krčmář <rkrcmar@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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.
