Re: [PATCH v2 7/8] KVM: s390: vsie: Fix guest page tables protection

[Janosch Frank <frankja@linux.ibm.com>]

On 3/20/26 17:15, Claudio Imbrenda wrote:
> When shadowing, the guest page tables are write-protected, in order to
> trap changes and properly unshadow the shadow mapping for the nested
> guest. Already shadowed levels are skipped, so that only the needed
> levels are write protected.
> 
> Currently the levels that get write protected are exactly one level too
> deep: the last level (nested guest memory) gets protected in the wrong
> way, and will be protected again correctly a few lines afterwards; most
> importantly, the highest non-shadowed level does *not* get write
> protected.
> 
> Moreover, if the nested guest is running in a real address space, there
> are no DAT tables to shadow.
> 
> Write protect the correct levels, so that all the levels that need to
> be protected are protected, and avoid double protecting the last level;
> skip attempting to shadow the DAT tables when the nested guest is
> running in a real address space.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> ---
>   arch/s390/kvm/gaccess.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 1054f9bd107f..17563f889c6b 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1517,13 +1517,15 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
>   	 * Skip levels that are already protected. For each level, protect
>   	 * only the page containing the entry, not the whole table.
>   	 */
> -	for (i = gl ; i >= w->level; i--) {
> -		rc = gmap_protect_rmap(mc, sg, entries[i - 1].gfn, gpa_to_gfn(saddr),
> -				       entries[i - 1].pfn, i, entries[i - 1].writable);

Yeah, tt for page tables is -1 and adding -1 ends up being LEVEL_MEM...

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> -		if (rc)
> -			return rc;
> -		if (!sg->parent)
> -			return -EAGAIN;
> +	if (w->level > LEVEL_MEM) {
> +		for (i = gl ; i >= w->level; i--) {

You could have fixed the rogue white space after "gl"

> +			rc = gmap_protect_rmap(mc, sg, entries[i].gfn, gpa_to_gfn(saddr),
> +					       entries[i].pfn, i + 1, entries[i].writable);
> +			if (rc)
> +				return rc;
> +			if (!sg->parent)
> +				return -EAGAIN;
> +		}
>   	}
>   
>   	rc = dat_entry_walk(NULL, entries[LEVEL_MEM].gfn, sg->parent->asce, DAT_WALK_LEAF,