Re: [PATCH v2 10/10] KVM: s390: Enable 1M pages by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: s390: missing hpage check in kvm_s390_set_mem_control() for CMMA
- [Low] KVM: s390: missing kernel-parameters.txt update for kvm.hpage default change
--

commit ecd16cdb4adaf78710af05b565a1375d75d2ce7b
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Enable 1M pages by default

This commit changes the default value of the hpage module parameter in
arch/s390/kvm/s390/s390.c from 0 to 1 so that 1M hugepage backing for
guest mappings is enabled by default.

> diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c
> index 280310870dac3..71d79b4437ac0 100644
> --- a/arch/s390/kvm/s390/s390.c
> +++ b/arch/s390/kvm/s390/s390.c
> @@ -212,7 +212,7 @@ module_param(nested, int, S_IRUGO);
>  MODULE_PARM_DESC(nested, "Nested virtualization support");
>  
>  /* allow 1m huge page guest backing */
> -static int hpage;
> +static int hpage = 1;

[Severity: Medium]
With hpage enabled by default, KVM_CAP_S390_HPAGE_1M is now available by
default to userspace VMMs. If userspace enables KVM_CAP_S390_HPAGE_1M (or
KVM_CAP_S390_HPAGE_2G) in kvm_vm_ioctl_enable_cap() before enabling CMMA via
KVM_S390_VM_MEM_ENABLE_CMMA, does kvm_s390_set_mem_control() allow mutually
exclusive hugepage and CMMA support to be active at the same time?

In arch/s390/kvm/s390/s390.c, kvm_vm_ioctl_enable_cap() checks
kvm->arch.use_cmma when enabling KVM_CAP_S390_HPAGE_1M:

	case KVM_CAP_S390_HPAGE_1M:
		mutex_lock(&kvm->lock);
		if (kvm->created_vcpus)
			r = -EBUSY;
		else if (!hpage || kvm->arch.use_cmma || kvm_is_ucontrol(kvm))
			r = -EINVAL;
		else {
			r = 0;
			set_bit(GMAP_FLAG_ALLOW_HPAGE_1M, &kvm->arch.gmap->flags);

However, kvm_s390_set_mem_control() only checks kvm->created_vcpus and does
not check whether GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G is set
on kvm->arch.gmap->flags:

	case KVM_S390_VM_MEM_ENABLE_CMMA:
		ret = -ENXIO;
		if (!sclp.has_cmma)
			break;

		VM_EVENT(kvm, 3, "%s", "ENABLE: CMMA support");
		mutex_lock(&kvm->lock);
		if (kvm->created_vcpus)
			ret = -EBUSY;
		else {
			kvm->arch.use_cmma = 1;
			/* Not compatible with cmma. */
			kvm->arch.use_pfmfi = 0;
			ret = 0;
		}
		mutex_unlock(&kvm->lock);
		break;

Prior to commit e38c884df921 ("KVM: s390: Switch to new gmap"),
KVM_S390_VM_MEM_ENABLE_CMMA checked kvm->mm->context.allow_gmap_hpage_1m and
returned -EINVAL.

When 1M or 2G leaf DAT entries are installed via gmap_link(), no 4K page
tables or PGSTEs exist for those mappings, causing dat_perform_essa() and
dat_set_cmma_bits() in arch/s390/kvm/gmap/dat.c to fail with -EFBIG when
walking to TABLE_TYPE_PAGE_TABLE without DAT_WALK_SPLIT:

	if (dat_entry_walk(NULL, gfn, datt, 0, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep)) {
		*state = (union essa_state) { .exception = 1 };
		return -1;
	}

Should kvm_s390_set_mem_control() reject KVM_S390_VM_MEM_ENABLE_CMMA with
-EINVAL when GMAP_FLAG_ALLOW_HPAGE_1M or GMAP_FLAG_ALLOW_HPAGE_2G is set?

>  module_param(hpage, int, 0444);
>  MODULE_PARM_DESC(hpage, "1m huge page backing support");

[Severity: Low]
Since this changes the default behavior of the kvm.hpage kernel command-line
and module parameter, should Documentation/admin-guide/kernel-parameters.txt
(as well as the kvm.hpage notes in
Documentation/virt/kvm/x86/running-nested-guests.rst and
Documentation/virt/kvm/api.rst) be updated to document this parameter and its
new default?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023155531.206113-1-imbrenda@linux.ibm.com?part=10