On 12.01.2022 16:42, Roger Pau Monné wrote:
> On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote:
>> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote:
>>> @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
>>> unsigned int size)
>>>
>>> data = merge_result(data, tmp_data, size - data_offset,
>>> data_offset);
>>> }
>>> - spin_unlock(&pdev->vpci->lock);
>>>
>>> return data & (0xffffffff >> (32 - 8 * size));
>>> }
>>
>> Here and ...
>>
>>> @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
>>> unsigned int size,
>>> break;
>>> ASSERT(data_offset < size);
>>> }
>>> + spin_unlock(&pdev->vpci_lock);
>>>
>>> if ( data_offset < size )
>>> /* Tailing gap, write the remaining. */
>>> vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
>>> data >> (data_offset * 8));
>>> -
>>> - spin_unlock(&pdev->vpci->lock);
>>> }
>>
>> ... even more so here I'm not sure of the correctness of the moving
>> you do: While pdev->vpci indeed doesn't get accessed, I wonder
>> whether there wasn't an intention to avoid racing calls to
>> vpci_{read,write}_hw() this way. In any event I think such movement
>> would need justification in the description.
>
> I agree about the need for justification in the commit message, or
> even better this being split into a pre-patch, as it's not related to
> the lock switching done here.
>
> I do think this is fine however, as racing calls to
> vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers
> around pci_conf_{read,write} functions, and the required locking (in
> case of using the IO ports) is already taken care in
> pci_conf_{read,write}.
IOW you consider it acceptable for a guest (really: Dom0) read racing
a write to read back only part of what was written (so far)? I would
think individual multi-byte reads and writes should appear atomic to
the guest.
Jan
