Re: [PATCH v2 4/9] s390x/pci: enable for load/store intepretation

From: Matthew Rosato <mjrosato@linux.ibm.com>
To: Thomas Huth <thuth@redhat.com>, qemu-s390x@nongnu.org
Cc: alex.williamson@redhat.com, schnelle@linux.ibm.com,
	cohuck@redhat.com, farman@linux.ibm.com, pmorel@linux.ibm.com,
	richard.henderson@linaro.org, david@redhat.com,
	pasic@linux.ibm.com, borntraeger@linux.ibm.com, mst@redhat.com,
	pbonzini@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org
Subject: Re: [PATCH v2 4/9] s390x/pci: enable for load/store intepretation
Date: Tue, 18 Jan 2022 13:08:21 -0500	[thread overview]
Message-ID: <aea53638-b38a-bc33-db46-a9ceefe95c87@linux.ibm.com> (raw)
In-Reply-To: <27a1db36-5664-6c90-ec39-aa6da5c23c31@redhat.com>

On 1/17/22 9:51 AM, Thomas Huth wrote:
> On 14/01/2022 21.38, Matthew Rosato wrote:
...
>>   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
>> DeviceState *dev,
>>                                 Error **errp)
>>   {
>>       S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>       PCIDevice *pdev = NULL;
>>       S390PCIBusDevice *pbdev = NULL;
>> +    int rc;
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>>           PCIBridge *pb = PCI_BRIDGE(dev);
>> @@ -1022,12 +1068,33 @@ static void s390_pcihost_plug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>           set_pbdev_info(pbdev);
>>           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> -            pbdev->fh |= FH_SHM_VFIO;
>> +            /*
>> +             * By default, interpretation is always requested; if the 
>> available
>> +             * facilities indicate it is not available, fallback to the
>> +             * intercept model.
>> +             */
>> +            if (pbdev->interp && 
>> !s390_has_feat(S390_FEAT_ZPCI_INTERP)) {
>> +                    DPRINTF("zPCI interpretation facilities 
>> missing.\n");
>> +                    pbdev->interp = false;
>> +                }
> 
> Wrong indentation in the above three lines.

Thanks

> 
>> +            if (pbdev->interp) {
>> +                rc = s390_pci_interp_plug(s, pbdev);
>> +                if (rc) {
>> +                    error_setg(errp, "zpci interp plug failed: %d", rc);
> 
> The error message is a little bit scarce for something that might be 
> presented to the user - maybe write at least "interpretation" instead of 
> "interp" ?
> 
Good point, I'll re-word to something like "Plug failed for zPCI device 
in interpretation mode: %d"

>> +                    return;
>> +                }
>> +            }
>>               pbdev->iommu->dma_limit = s390_pci_start_dma_count(s, 
>> pbdev);
>>               /* Fill in CLP information passed via the vfio region */
>>               s390_pci_get_clp_info(pbdev);
>> +            if (!pbdev->interp) {
>> +                /* Do vfio passthrough but intercept for I/O */
>> +                pbdev->fh |= FH_SHM_VFIO;
>> +            }
>>           } else {
>>               pbdev->fh |= FH_SHM_EMUL;
>> +            /* Always intercept emulated devices */
>> +            pbdev->interp = false;
>>           }
> 
>   Thomas
>