Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2022 17:21, Oleksandr Andrushchenko wrote:
> 
> 
> On 07.02.22 18:15, Jan Beulich wrote:
>> On 07.02.2022 17:07, Oleksandr Andrushchenko wrote:
>>> On 07.02.22 17:26, Jan Beulich wrote:
>>>> 1b. Make vpci_write use write lock for writes to command register and BARs
>>>> only; keep using the read lock for all other writes.
>>> I am not quite sure how to do that. Do you mean something like:
>>> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>>>                   uint32_t data)
>>> [snip]
>>>       list_for_each_entry ( r, &pdev->vpci->handlers, node )
>>> {
>>> [snip]
>>>       if ( r->needs_write_lock)
>>>           write_lock(d->vpci_lock)
>>>       else
>>>           read_lock(d->vpci_lock)
>>> ....
>>>
>>> And provide rw as an argument to:
>>>
>>> int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
>>>                         vpci_write_t *write_handler, unsigned int offset,
>>>                         unsigned int size, void *data, --->>> bool 
>>> write_path <<<-----)
>>>
>>> Is this what you mean?
>> This sounds overly complicated. You can derive locally in vpci_write(),
>> from just its "reg" and "size" parameters, whether the lock needs taking
>> in write mode.
> Yes, I started writing a reply with that. So, the summary (ROM
> position depends on header type):
> if ( (reg == PCI_COMMAND) || (reg == ROM) )
> {
>      read PCI_COMMAND and see if memory or IO decoding are enabled.
>      if ( enabled )
>          write_lock(d->vpci_lock)
>      else
>          read_lock(d->vpci_lock)
> }

Hmm, yes, you can actually get away without using "size", since both
command register and ROM BAR are 32-bit aligned registers, and 64-bit
accesses get split in vpci_ecam_write().

For the command register the memory- / IO-decoding-enabled check may
end up a little more complicated, as the value to be written also
matters. Maybe read the command register only for the ROM BAR write,
using the write lock uniformly for all command register writes?

> Do you also think we can drop pdev->vpci (or currently pdev->vpci->lock)
> at all then?

I haven't looked at this in any detail, sorry. It sounds possible,
yes.

Jan