Re: [PATCH v8 02/13] vpci: use per-domain PCI lock to protect vpci structure

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Roger,

Roger Pau Monné <roger.pau@xxxxxxxxxx> 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