Re: [PATCH v4 14/15] s390x/pci: Implement migration for emulated devices

On 7/28/2026 1:59 PM, Konstantin Shkolnyy wrote:
> On 260728 12:46, Farhan Ali wrote:
>>
>> On 7/27/2026 4:23 PM, Konstantin Shkolnyy wrote:
>>>>> +static int s390_pci_device_pre_load(void *opaque)
>>>>> +{
>>>>> +    S390PCIBusDevice *pbdev = S390_PCI_DEVICE(opaque);
>>>>> +    S390PCIBusDevice *found_pbdev;
>>>>> +
>>>>> +    /*
>>>>> +     * Make sure pbdev is removed from the table before state 
>>>>> load. The change
>>>>> +     * of pbdev->idx means it needs to be moved to a different 
>>>>> position anyway,
>>>>> +     * and is illegal while in the table. But be careful to not 
>>>>> remove
>>>>> +     * instead another pbdev whose state might have been loaded 
>>>>> earlier and
>>>>
>>>> I think "instead" is not needed in the statement above.
>>>>
>>>> Maybe I am missing something, but could you help me understand why 
>>>> do we need to remove the pbdev if we found at a particular idx? If 
>>>> there is a collision with idx, ie on destination we have a 
>>>> different device with the same idx, are we removing a valid device?
>>>
>>> I see that this is not the best comment. How about this variant:
>>>
>>>     * State loading can change pbdev->idx. Therefore, make sure 
>>> pbdev is removed
>>>     * from the table before that happens. The table type used stores 
>>> a pointer
>>>     * to pbdev->idx and becomes corrupt if idx is changed from 
>>> outside. But be
>>>     * careful to not remove instead another pbdev whose state might 
>>> have been
>>>     * loaded earlier and that got assigned this idx value and had 
>>> therefore
>>>     * already replaced our pbdev in the table. post_load() will 
>>> reinsert our
>>>     * pbdev into the table.
>>>
>> So pbdev->idx is autogenerated when we create the device at 
>> destination, since we are migrating the idx from source this could 
>> change on destination and corrupt the hash table?
>
> Exactly.
>
>  Then do we need to migrate the idx?
>>
>
> idx is part of "function handle". So, it should remain unchanged after 
> migration.
>
>>
>>>>
>>>>> +     * that has then replaced our pbdev. (post_load() will put 
>>>>> our pbdev back.)
>>>>> +     */
>>>>> +    found_pbdev = g_hash_table_lookup(s390_get_phb()->zpci_table, 
>>>>> &pbdev->idx);
>>>>> +    assert(found_pbdev); 
>> AFAIU the pbdev->idx at pre_load() would be what we assign 
>> automatically at the destination. In that case wouldn't this assert 
>> always be true?
>
> Yes. This assert documents that a pbdev is always expected to be found 
> for this "autogenerated when we create the device at destination" 
> pbdev->idx - either the pbdev we are currently processing, or another 
> pbdev that was "loaded" earlier with the same idx value (which came 
> from the source) and inserted by post_load() into zpci_table.
>>
>>
>>>>> +    if (found_pbdev == pbdev) {
>>
>> similarly this should also be true?
>
> No always true, as described above.
>
Okay this makes a lot more sense to me now, thanks for the explanation!


>>
>>
>>>>> + g_hash_table_remove(s390_get_phb()->zpci_table, &pbdev->idx);
>>
>> Don't we have to free this idx now? so reverse of s390_pci_alloc_idx()?
>
> AFA I can see, simply removing pbdev->idx from zpci_table already 
> "frees" the idx value for reuse. s390_pci_alloc_idx() interprets 
> s->next_idx as the "next mostly likely free" idx value. It then checks 
> if zpci_table has this idx; if it does, it checks idx+1, etc. until it 
> doesn't.
>
> It seems, we could add "s->next_idx = pbdev->idx;" here to make it 
> pick this idx next time instead of larger values, but it's not 
> strictly necessary. Do you think we should do it? 

We seem to be doing it in s390_pci_interp_plug() [1] so maybe we should 
just to close this gap? In reality though this "leak" is not a concern 
for resource exhaustion.

[1] 
https://elixir.bootlin.com/qemu/v11.1.0-rc1/source/hw/s390x/s390-pci-bus.c#L1072

Thanks

Farhan

>
>>
>>
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int s390_pci_device_post_load(void *opaque, int version_id)
>>>>> +{
>>>>> +    S390PCIBusDevice *pbdev = S390_PCI_DEVICE(opaque);
>>>>> +
>>>>>       /*
>>>>> -     * TODO: add state handling here, so migration works at least 
>>>>> with
>>>>> -     * emulated pci devices on s390x
>>>>> +     * Now that pbdev->idx has been loaded, use it to place pbdev 
>>>>> back into
>>>>> +     * the table. This may replace a different 
>>>>> not-yet-state-loaded pbdev,
>>>>> +     * but pre_load() handles this case.
>>>>>        */
>>>>> -    .unmigratable = 1,
>>>>> +    g_hash_table_replace(s390_get_phb()->zpci_table, &pbdev->idx, 
>>>>> pbdev);
>>>>> +
>>>>> +    /*
>>>>> +     * Regenerate IOMMU state, including IOTLB contents and QEMU 
>>>>> memory regions.
>>>>> +     */
>>>>> +    if (pbdev->iommu_enabled) {
>>>>> +        assert(pbdev->iommu);
>>>>> +        if (s390_pci_is_translation_enabled(pbdev->g_iota)) {
>>>>> +            s390_pci_iommu_enable(pbdev);
>>>>> +            s390_pci_ioat_replay(pbdev);
>>>>> +        } else {
>>>>> +            s390_pci_iommu_direct_map_enable(pbdev);
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * Guest sets fmb_addr by mpcifc.ZPCI_MOD_FC_SET_MEASURE 
>>>>> instruction,
>>>>> +     * whose handler consequently starts fmb_timer. We may need 
>>>>> to restart it.
>>>>> +     */
>>>>> +    if (pbdev->fmb_addr) {
>>>>> +        assert(!pbdev->fmb_timer);
>>>>> +        assert(pbdev->pci_group);
>>>>> +        pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>>>>> +                                        fmb_update, pbdev);
>>>>> +        timer_mod(pbdev->fmb_timer,
>>>>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
>>>>> + pbdev->pci_group->zpci_group.mui);
>>>>> +    }
>>>>> +    return 0;
>>>>> +} 
>