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

Commit Message

Oleksandr Andrushchenko Feb. 1, 2022, 4:25 p.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

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é <[e-mail]>
Signed-off-by: Oleksandr Andrushchenko <[e-mail]>

---
New in v6
---
 xen/drivers/vpci/vpci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Roger Pau Monné Feb. 2, 2022, 8:44 a.m. UTC | #1

On Tue, Feb 01, 2022 at 06:25:07PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[e-mail]>
> 
> 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é <[e-mail]>
> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>

Acked-by: Roger Pau Monné <[e-mail]>

Would like to make sure whether Jan still have concerns about
splitting accesses though. Also since I'm the maintainer we need a
Reviewed-by from someone else.

Thanks, Roger.

Jan Beulich Feb. 2, 2022, 9:05 a.m. UTC | #2

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 <[e-mail]>

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é <[e-mail]>
>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>

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

> Acked-by: Roger Pau Monné <[e-mail]>

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 <[e-mail]>

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

Oleksandr Andrushchenko Feb. 2, 2022, 9:38 a.m. UTC | #3

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 <[e-mail]>
> 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é <[e-mail]>
>>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
> ... 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é <[e-mail]>
> 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 <[e-mail]>
>
> 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

Jan Beulich Feb. 2, 2022, 9:45 a.m. UTC | #4

On 02.02.2022 10:38, Oleksandr Andrushchenko wrote:
> 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 <[e-mail]>
>> 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é <[e-mail]>
>>>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
>> ... 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.

My personal view on it is that if you've broken this out of a larger
patch coming from Roger, then he should be named as the author.

Jan

Oleksandr Andrushchenko Feb. 2, 2022, 9:49 a.m. UTC | #5

On 02.02.22 11:45, Jan Beulich wrote:
> On 02.02.2022 10:38, Oleksandr Andrushchenko wrote:
>> 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 <[e-mail]>
>>> 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é <[e-mail]>
>>>>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
>>> ... 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.
> My personal view on it is that if you've broken this out of a larger
> patch coming from Roger, then he should be named as the author.
Agree, will change.
Roger, I am sorry I didn't do it from the start
> Jan
>
Thank you,
Oleksandr

Roger Pau Monné Feb. 2, 2022, 9:49 a.m. UTC | #6

On Wed, Feb 02, 2022 at 10:05:55AM +0100, 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 <[e-mail]>
> 
> 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é <[e-mail]>
> >> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
> 
> ... the first S-o-b, as these are expected to be in chronological
> order.
> 
> > Acked-by: Roger Pau Monné <[e-mail]>
> 
> 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 think both vPCI patches in the series could go in when ready. They
are improvements on their own.

> > 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 <[e-mail]>
> 
> 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.

I did realize about such asymmetry also, but I don't think it can be
solved.

Thanks, Roger.

Patch

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 657697fe3406..fb0947179b79 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -370,6 +370,7 @@  uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
             break;
         ASSERT(data_offset < size);
     }
+    spin_unlock(&pdev->vpci->lock);
 
     if ( data_offset < size )
     {
@@ -379,7 +380,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));
 }
@@ -475,13 +475,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);
 }
 
 /* Helper function to check an access size and alignment on vpci space. */