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. -- WBR, Volodymyr
