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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Jan!

On 02.02.22 11:05, Jan Beulich wrote:
> 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.
Well, I was not sure here: the idea and the original code belongs
to Roger and it was a part of a dedicated other patch. So, technically,
this patch didn't exist before and Roger hasn't created it (the patch).
So, this is why I'm in doubt here: should I change the authorship
to Roger's? I had no means to offend anyone here nor I pretend
for the authorship in any form.

I would like to apologize if anyone feels offended because of the authorship

Please help me understand what is the right approach here.
>
>> 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.
I would postpone patches [0; 1] and just go with [3; 4] if your will
If not, then the whole series can be postponed until I have the
bigger one ready.
>
>> 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
>
Thank you,
Oleksandr