On Wed, Sep 13, 2023 at 11:53:56PM +0000, Volodymyr Babchuk wrote:
>
> Hi,
>
> Jan Beulich <[email protected]> writes:
>
> > On 13.09.2023 01:41, Volodymyr Babchuk wrote:
> >> Jan Beulich <[email protected]> writes:
> >>> On 30.08.2023 01:19, Volodymyr Babchuk wrote:
> >>>> @@ -1481,6 +1488,13 @@ static int assign_device(struct domain *d, u16
> >>>> seg, u8 bus, u8 devfn, u32 flag)
> >>>> if ( pdev->broken && d != hardware_domain && d != dom_io )
> >>>> goto done;
> >>>>
> >>>> + if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> >>>> + {
> >>>> + write_lock(&pdev->domain->pci_lock);
> >>>> + vpci_deassign_device(pdev);
> >>>> + write_unlock(&pdev->domain->pci_lock);
> >>>> + }
> >>>
> >>> Why is the DomIO special case ...
> >>
> >> vpci_deassign_device() does nothing if vPCI was initialized for a
> >> domain. So it not wrong to call this function even if pdev belongs to
> >> dom_io.
> >
> > Well, okay, but then you acquire a lock just to do nothing (apart
> > from the apparent asymmetry).
>
> Yes, I agree. I'll add the same check as below. Thanks for the review.
Hm, no, I would rather rely on the has_vpci check inside of
vpci_{,de}assign_device() than open coding dom_io checks elsewhere.
This is not a hot path, the extra lock taking is likely negligible
compared to the cost of assign operations.
Thanks, Roger.
