Re: [PATCH v1 1/3] Add map/unmap ioctl and clean mappings post-guest

[Matthew Rosato]

On 3/7/26 10:04 PM, Douglas Freimuth wrote:
> Patch 1: This patch adds map/unmap ioctls which map the adapter set

This comment applies to all patches in the series:

Drop the Patch #: intro, it would otherwise end up in git history.

Also remove phrases like 'this patch' and switch to imperative phrasing
ref: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Also +1 to what Sean said, "KVM: s390: Add map..."

> indicator pages so the pages can be accessed when interrupts are
> disabled. The mappings are cleaned up when the guest is removed.
> 
> Fencing of Fast Inject in Secure Execution environments is enabled in
This is a bit misleading.  You haven't implemented kvm_ach_set_irq_inatomic at this point, so this patch can't be fencing it yet.

What you are doing is fencing the map/unmap ioctls so that you avoid the longterm pin in SE.  I'd focus on that part here.

And then in patch 3, when you enable the feature, you can point out that it is fenced in SE because the mappings will never be available.

> this patch by not mapping adapter indicator pages. In Secure Execution
> environments the path of execution available before this patch is followed.
> Statistical counters to count map/unmap functions for adapter indicator
> pages are added in this patch. The counters can be used to analyze
> map/unmap functions in non-Secure Execution environments and similarly
> can be used to analyze Secure Execution environments where the counters
> should not be incremented as the adapter indicator pages are not mapped.
> 
> Signed-off-by: Douglas Freimuth <freimuth@xxxxxxxxxxxxx>


> @@ -2477,14 +2572,28 @@ static int modify_io_adapter(struct kvm_device *dev,
>  		if (ret > 0)
>  			ret = 0;
>  		break;
> -	/*
> -	 * The following operations are no longer needed and therefore no-ops.
> -	 * The gpa to hva translation is done when an IRQ route is set up. The
> -	 * set_irq code uses get_user_pages_remote() to do the actual write.
> -	 */
>  	case KVM_S390_IO_ADAPTER_MAP:
>  	case KVM_S390_IO_ADAPTER_UNMAP:
> -		ret = 0;
> +		mutex_lock(&dev->kvm->lock);
> +		if (kvm_s390_pv_is_protected(dev->kvm)) {
> +			mutex_unlock(&dev->kvm->lock);
> +			break;

Was mentioned by the kernel bot also -- because you removed the initialization of ret above you will now return something uninitialized if you take this if statement.

Either initialize ret above or return the intended value (0?) here.

Also I think this is worth a comment block above this check stating why you are doing it (e.g. avoid long-term pin for secure exeuction guest)