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

Matthew Rosato
2021-12-08T15:04:56Z

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@linux.ibm.com>
>> ---
>> 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<---
>