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.
