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

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.

> 
> 
>>>> + 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?

> 
> 
>>>> +    }
>>>> +
>>>> +    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;
>>>> +}