On 9/3/25 1:12 PM, Farhan Ali wrote:
>
> On 9/1/2025 4:25 AM, Cédric Le Goater wrote:
>> On 8/25/25 23:24, Farhan Ali wrote:
>>> Add an s390x specific callback for vfio error handling. For s390x pci
>>> devices,
>>> we have platform specific error information. We need to retrieve this error
>>> information for passthrough devices. This is done via a memory region which
>>> exposes that information.
>>>
>>> Once this error information is retrieved we can then inject an error into
>>> the guest, and let the guest drive the recovery.
>>>
>>> Signed-off-by: Farhan Ali <[email protected]>
>>> ---
>>> hw/s390x/s390-pci-bus.c | 5 +++
>>> hw/s390x/s390-pci-vfio.c | 76 ++++++++++++++++++++++++++++++++
>>> include/hw/s390x/s390-pci-bus.h | 1 +
>>> include/hw/s390x/s390-pci-vfio.h | 2 +
>>> 4 files changed, 84 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index f87d2748b6..af42eb9938 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -158,6 +158,8 @@ static void s390_pci_perform_unplug(S390PCIBusDevice
>>> *pbdev)
>>> {
>>> HotplugHandler *hotplug_ctrl;
>>> + qemu_mutex_destroy(&pbdev->err_handler_lock);
>>> +
>>> if (pbdev->pft == ZPCI_PFT_ISM) {
>>> notifier_remove(&pbdev->shutdown_notifier);
>>> }
>>> @@ -1140,6 +1142,7 @@ static void s390_pcihost_plug(HotplugHandler
>>> *hotplug_dev, DeviceState *dev,
>>> pbdev->iommu->pbdev = pbdev;
>>> pbdev->state = ZPCI_FS_DISABLED;
>>> set_pbdev_info(pbdev);
>>> + qemu_mutex_init(&pbdev->err_handler_lock);
>>> if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>>> /*
>>> @@ -1164,6 +1167,8 @@ static void s390_pcihost_plug(HotplugHandler
>>> *hotplug_dev, DeviceState *dev,
>>> 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);
>>> + /* Setup error handler for error recovery */
>>> + s390_pci_setup_err_handler(pbdev);
>>
>> This can fail. Please add an 'Error **' parameter and change the returned
>> value to bool.
>>
> I wanted to avoid hard failing here as we can have mismatch in kernel and
> QEMU support for the feature. For example we can have a newer QEMU version
> with the feature running on an older kernel. So wanted to treat any error in
> setting up the error handler would be more of an info/warn message.
+1, please do not cause a hard failure if the underlying host kernel is simply
missing support...
>>> +void s390_pci_setup_err_handler(S390PCIBusDevice *pbdev)
>>> +{
>>> + int ret;
>>> + VFIOPCIDevice *vfio_pci = container_of(pbdev->pdev, VFIOPCIDevice,
>>> pdev);
>>> + uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>>> + sizeof(uint64_t))] = {};
>>> + struct vfio_device_feature *feature = (struct vfio_device_feature
>>> *)buf;
>>> +
>>> + feature->argsz = sizeof(buf);
>>> + feature->flags = VFIO_DEVICE_FEATURE_PROBE |
>>> VFIO_DEVICE_FEATURE_ZPCI_ERROR;
>>> +
>>> + ret = vfio_pci->vbasedev.io_ops->device_feature(&vfio_pci->vbasedev,
>>> + feature);
>>
>> Please introduce vfio helpers to hide the internal indirection :
>>
>> ->vbasedev.io_ops->device_feature(...)
>>
>>> +
>>> + if (ret) {
>>
>> Shouldn't we test the return value to decide if the error is
>> an unimplemented feature or an unexpected error ?
>
> Yeah, I think it makes sense separate out error for unimplemented feature
> (ENOTTY) vs any other unexpected error. Will change this.
>
... But if you add differentiation here between the 2 types of errors then I
would be fine with hard-fail for unexpected cases and info/warn for missing
host kernel support.
