Re: [PATCH 3/4] vpci: shrink critical section in vpci_{read/write}

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2022 09:44, Roger Pau Monné wrote:
> On Tue, Feb 01, 2022 at 06:25:07PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>

Oleksandr, can you please clarify authorship here? The rule of thumb is
that From: matches ...

>> Shrink critical section in vpci_{read/write} 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}.
>>
>> Please note, that we anyways split 64bit writes into two 32bit ones
>> without taking the lock for the whole duration of the access, so it is
>> possible to see a partially updated state as a result of a 64bit write:
>> the PCI(e) specification don't seem to specify whether the ECAM is allowed
>> to split memory transactions into multiple Configuration Requests and
>> whether those could then interleave with requests from a different CPU.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>

... the first S-o-b, as these are expected to be in chronological
order.

> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I'll take your unconstrained ack to indicate that you're also fine
with this going in right away; see my reply to 0/4 as to the earlier
two patches. Please let me know (soonish) if I shouldn't make this
implication, but I shall wait with committing for clarification of
the question further up anyway.

> Would like to make sure whether Jan still have concerns about
> splitting accesses though.

I continue to be a little concerned, but as long as the decision is
taken consciously (and this is recorded in the description), which
clearly is the case now, I have no objections. In the end well
behaved OSes will suitably serialize accesses to config space anyway.

> Also since I'm the maintainer we need a Reviewed-by from someone else.

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I'm not sure this is strictly needed though: I'd generally consider
a 2nd (later) S-o-b as valid stand-in for R-b, at least as long as
the 2nd author doesn't scope-restrict their tag.

One further remark though: The resulting asymmetry of the locking
(covering the "head" hw read but not the "tail" one) looks a little
odd, but I will admit that I don't see a good way to restore symmetry.

Jan