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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 07, 2022 at 05:37:49PM +0100, Jan Beulich wrote:
> 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.

AFAICT you should avoid taking the per-device vpci lock when you take
the per-domain lock in write mode. Otherwise you still need the
per-device vpci lock in order to keep consistency between concurrent
accesses to the device registers.

Thanks, Roger.