Re: [PATCH v4 4/5] vpci: add SR-IOV support for PVH Dom0

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 11/2/26 3:42 PM, Jan Beulich wrote:
> On 02.11.2026 14:22, Mykyta Poturai wrote:
>> On 10/28/26 3:49 PM, Jan Beulich wrote:
>>> On 15.10.2026 16:08, Mykyta Poturai wrote:
>>>> @@ -139,6 +142,47 @@ const unsigned long *pci_get_ro_map(u16 seg)
>>>>        return pseg ? pseg->ro_map : NULL;
>>>>    }
>>>>
>>>> +/*
>>>> + * Marking is serialised by the hardware domain's pci_lock, which the 
>>>> SR-IOV
>>>> + * control register write handler holds; unmarking only clears a bit.
>>>> + */
>>>
>>> What about pci_vf_setup_pending()? How's that (to be) serialized again 
>>> marking
>>> and unmarking?
>>>
>> It isn't. The test runs ahead of any lock because adding the VF takes them.
>>
>> We could put it under the hardware domain's pci_lock, which is what
>> control_write() holds for marking and unmarking, but it would have to be
>> dropped before vpci_sriov_add_vf() anyway: pci_add_device() takes 
>> pcidevs_lock,
>> and the order is pcidevs_lock then pci_lock. So the test and the add can't be
>> made atomic with the locks we have. Is the lock worth taking just for the bit
>> test?
> 
> No, such locking would make sense only when held from before the test until
> after the result was actually acted upon (in the case here: the bit was
> cleared by pci_vf_setup_end()).
> 
> pcidevs_lock vs d->pci_lock wouldn't really be an issue here, though: The
> former is recursive, so you could acquire ahead of acquiring d->pci_lock.
> The "inner" acquire in pci_add_device() would then be little more than a
> no-op. (Of course proper commentary would need adding if such an approach
> was taken.)
> 

Missed that it was recursive somehow, so yes, checking under the lock is clearly
better. Will do this in the next version.

>> The only thing I can see actually misfiring is vf_setup_map: xvzalloc_array()
>> zeroes it and the pointer store publishes it with nothing in between, so a
>> reader can theoretically see the pointer ahead of the zeroing and test an
>> uninitialised bit. Should we reallocate it in alloc_pseg(), or just add a 
>> memory barrier?
> 
> Hmm, is this really the only issue? What if two racing pci_vf_setup_pending()
> both yield true? Then we'd end up with two competing vpci_sriov_add_vf().
> 
> Jan