On Mon, Jul 24, 2023 at 12:07:48AM +0000, Volodymyr Babchuk wrote: > > Hi Roger, > > Roger Pau Monné <[email protected]> writes: > > > On Thu, Jul 20, 2023 at 03:27:29PM +0200, Jan Beulich wrote: > >> On 20.07.2023 13:20, Roger Pau Monné wrote: > >> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote: > >> >> @@ -447,8 +476,16 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, > >> >> unsigned int size, > >> >> > >> >> /* > >> >> * Find the PCI dev matching the address, which for hwdom also > >> >> requires > >> >> - * consulting DomXEN. Passthrough everything that's not trapped. > >> >> + * consulting DomXEN. Passthrough everything that's not trapped. > >> >> + * If this is hwdom, we need to hold locks for both domain in case > >> >> if > >> >> + * modify_bars is called() > >> > > >> > Typo: the () wants to be at the end of modify_bars(). > >> > > >> >> */ > >> >> + read_lock(&d->pci_lock); > >> >> + > >> >> + /* dom_xen->pci_lock always should be taken second to prevent > >> >> deadlock */ > >> >> + if ( is_hardware_domain(d) ) > >> >> + read_lock(&dom_xen->pci_lock); > >> > > >> > For modify_bars() we also want the locks to be in write mode (at least > >> > the hw one), so that the position of the BARs can't be changed while > >> > modify_bars() is iterating over them. > >> > >> Isn't changing of the BARs happening under the vpci lock? > > > > It is. > > > >> Or else I guess > >> I haven't understood the description correctly: My reading so far was > >> that it is only the presence (allocation status / pointer validity) that > >> is protected by this new lock. > > > > Hm, I see, yes. I guess it was a previous patch version that also > > took care of the modify_bars() issue by taking the lock in exclusive > > mode here. > > > > We can always do that later, so forget about that comment (for now). > > Are you sure? I'd rather rework the code to use write lock in the > modify_bars(). This is why we began all this journey in the first place.
Well, I was just saying that it doesn't need to be done in this same patch, it can be done as a followup if that's preferred, but one way or another we need to deal with it. I'm fine if you want to adjust the commit message and do the change in this same patch. Thanks, Roger.
