Re: [PATCH 20/32] KVM: s390: pci: provide routines for enabling/disabling interpretation

[Matthew Rosato]

On 12/8/21 4:44 AM, Niklas Schnelle wrote:
> On Tue, 2021-12-07 at 15:57 -0500, Matthew Rosato wrote:
> > These routines will be wired into the vfio_pci_zdev ioctl handlers to
> > respond to requests to enable / disable a device for zPCI Load/Store
> > interpretation.
> >
> > The first time such a request is received, enable the necessary facilities
> > for the guest.
> >
> > Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > ---
> >   arch/s390/include/asm/kvm_pci.h |  4 ++
> >   arch/s390/kvm/pci.c             | 91 +++++++++++++++++++++++++++++++++
> >   arch/s390/pci/pci.c             |  3 ++
> >   3 files changed, 98 insertions(+)
> >
> > diff --git a/arch/s390/include/asm/kvm_pci.h b/arch/s390/include/asm/kvm_pci.h
> > index 3e491a39704c..5d6283acb54c 100644
> > --- a/arch/s390/include/asm/kvm_pci.h
> ---8<---
> > 		return rc;
> > +	}
> > +
> > +	/*
> > +	 * Store information about the identity of the kvm guest allowed to
> > +	 * access this device via interpretation to be used by host CLP
> > +	 */
> > +	zdev->gd = gd;
> > +
> > +	rc = zpci_enable_device(zdev);
> > +	if (rc)
> > +		goto err;
> > +
> > +	/* Re-register the IOMMU that was already created */
> > +	rc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
> > +				(u64)zdev->dma_table);
>
> The zdev->dma_table is a virtual address but we need an absolute
> address in the MPCIFC so the above should use
> virt_to_phys(zdev->dma_table) to be compatible with future V != R
> kernel memory. As of now since virtual and absolute kernel addresses
> are the same this is not a bug and we've had this (wrong) pattern in
> the rest of the code but let's get it righht here from the start.
>
> See also my commit "s390/pci: use physical addresses in DMA tables"
> that is currently in the s390 feature branch.

You're right of course -- I saw those changes happening as I prepared 
this series but I didn't want to delay getting comments any longer, what 
with the holidays approaching.  Of course, I didn't realize they were 
already out on the feature branch.

I suspect there is some more of this also in the code related to 
handling RPCIT.  AEN setup too.

> > +	if (rc)
> > +		goto err;
> > +
> > +	return rc;
> > +
> > +err:
> > +	zdev->gd = 0;
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_s390_pci_interp_enable);
> > +
> > +int kvm_s390_pci_interp_disable(struct zpci_dev *zdev)
> > +{
> > +	int rc;
> > +
> > +	if (zdev->gd == 0)
> > +		return -EINVAL;
> > +
> > +	/* Remove the host CLP guest designation */
> > +	zdev->gd = 0;
> > +
> > +	if (zdev_enabled(zdev)) {
> > +		rc = zpci_disable_device(zdev);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	rc = zpci_enable_device(zdev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* Re-register the IOMMU that was already created */
> > +	rc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
> > +				(u64)zdev->dma_table);
>
> Same as above
>
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_s390_pci_interp_disable);
> > +
> ---8<---