Re: [PATCH v4 09/15] KVM: s390: move some gmap shadowing functions away from mm/gmap.c

["Christoph Schlameuss" <schlameuss@linux.ibm.com>]

LGTM, but maybe add some more comments about the required locks?

On Thu Jan 23, 2025 at 3:46 PM CET, Claudio Imbrenda wrote:
> Move some gmap shadowing functions from mm/gmap.c to kvm/kvm-s390.c and
> the newly created kvm/gmap-vsie.c
>
> This is a step toward removing gmap from mm.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/include/asm/gmap.h |   9 +-
>  arch/s390/kvm/Makefile       |   2 +-
>  arch/s390/kvm/gmap-vsie.c    | 139 ++++++++++++++++++++
>  arch/s390/kvm/gmap.h         |  20 +++
>  arch/s390/kvm/kvm-s390.c     |  62 ++++++++-
>  arch/s390/kvm/kvm-s390.h     |   2 +
>  arch/s390/kvm/vsie.c         |   2 +
>  arch/s390/mm/gmap.c          | 238 +++++------------------------------
>  8 files changed, 256 insertions(+), 218 deletions(-)
>  create mode 100644 arch/s390/kvm/gmap-vsie.c

[...]

> +/**
> + * gmap_find_shadow - find a specific asce in the list of shadow tables
> + * @parent: pointer to the parent gmap
> + * @asce: ASCE for which the shadow table is created
> + * @edat_level: edat level to be used for the shadow translation
> + *
> + * Returns the pointer to a gmap if a shadow table with the given asce is
> + * already available, ERR_PTR(-EAGAIN) if another one is just being created,
> + * otherwise NULL

nit: Called with parent->shadow_lock

> + */
> +static struct gmap *gmap_find_shadow(struct gmap *parent, unsigned long asce, int edat_level)
> +{
> +	struct gmap *sg;
> +
> +	list_for_each_entry(sg, &parent->children, list) {
> +		if (!gmap_shadow_valid(sg, asce, edat_level))
> +			continue;
> +		if (!sg->initialized)
> +			return ERR_PTR(-EAGAIN);
> +		refcount_inc(&sg->ref_count);
> +		return sg;
> +	}
> +	return NULL;
> +}

[...]

nit: add comment:

Called with vcpu->kvm->srcu and vcpu->arch.gmap->mm in read

> +int __kvm_s390_mprotect_many(struct gmap *gmap, gpa_t gpa, u8 npages, unsigned int prot,
> +			     unsigned long bits)
> +{
> +	unsigned int fault_flag = (prot & PROT_WRITE) ? FAULT_FLAG_WRITE : 0;
> +	gpa_t end = gpa + npages * PAGE_SIZE;
> +	int rc;
> +
> +	for (; gpa < end; gpa = ALIGN(gpa + 1, rc)) {
> +		rc = gmap_protect_one(gmap, gpa, prot, bits);
> +		if (rc == -EAGAIN) {
> +			__kvm_s390_fixup_fault_sync(gmap, gpa, fault_flag);
> +			rc = gmap_protect_one(gmap, gpa, prot, bits);
> +		}
> +		if (rc < 0)
> +			return rc;
> +	}
> +
> +	return 0;
> +}

[...]