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

On 260728 16:29, Farhan Ali wrote:
> 
> 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

However, s390_pcihost_unplug() doesn't change s->next_idx after calling 
g_hash_table_remove().

In practice, since we have 64K values to go through before the idx 
allocator wraps around and starts searching from 0, it seems unlikely to 
happen. (We'd have to plug PCI devices 64K times.)

The purpose of s->next_idx appears to be to help allocation to happen on 
the first try without checking zpci_table many times.

If we just keep incrementing s->next_idx and never set it back, it'll 
allocate on the first try 64K times.

If, however, we set it back after g_hash_table_remove(), the next 
allocation will be quick, but subsequent ones might have to scan 
zpci_table before finding a free value. In this implementation, the 
scanning we try to avoid might happen in practice long before we reach 
64K allocations. On top of that, we'll then reuse old "function handle" 
values - not that I know of a problem with that, but...

So, it's extra code to set s->next_idx back and I don't see how it's useful.

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