[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v13 01/14] vpci: use per-domain PCI lock to protect vpci structure
On 13.02.2024 10:01, Roger Pau Monné wrote: > On Tue, Feb 13, 2024 at 09:44:58AM +0100, Jan Beulich wrote: >> On 13.02.2024 09:35, Roger Pau Monné wrote: >>> On Fri, Feb 02, 2024 at 04:33:05PM -0500, Stewart Hildebrand wrote: >>>> --- a/xen/include/xen/sched.h >>>> +++ b/xen/include/xen/sched.h >>>> @@ -462,7 +462,8 @@ struct domain >>>> #ifdef CONFIG_HAS_PCI >>>> struct list_head pdev_list; >>>> /* >>>> - * pci_lock protects access to pdev_list. >>>> + * pci_lock protects access to pdev_list. pci_lock also protects >>>> pdev->vpci >>>> + * structure from being removed. >>>> * >>>> * Any user *reading* from pdev_list, or from devices stored in >>>> pdev_list, >>>> * should hold either pcidevs_lock() or pci_lock in read mode. >>>> Optionally, >>>> @@ -628,6 +629,18 @@ struct domain >>>> unsigned int cdf; >>>> }; >>>> >>>> +/* >>>> + * Check for use in ASSERTs to ensure that: >>>> + * 1. we can *read* d->pdev_list >>>> + * 2. pdevs (belonging to this domain) do not go away >>>> + * 3. pdevs (belonging to this domain) do not get assigned to other >>>> domains >>> >>> I think you can just state that this check ensures there will be no >>> changes to the entries in d->pdev_list, but not the contents of each >>> entry. No changes to d->pdev_list already ensures not devices can be >>> deassigned or removed from the system, and obviously makes the list >>> safe to iterate against. >>> >>> I would also drop the explicitly mention this is intended for ASSERT >>> usage: there's nothing specific in the code that prevents it from >>> being used in other places (albeit I think that's unlikely). >> >> But pcidevs_locked(), resolving to spin_is_locked(), isn't reliable. The >> assertion usage is best-effort only, without a guarantee that all wrong >> uses would be caught. > > Do we want to protect this with !NDEBUG guards then? Yes, that would look to be desirable. >>>> + * This check is not suitable for protecting other state or critical >>>> regions. >>>> + */ >>>> +#define pdev_list_is_read_locked(d) ({ \ >>> >>> I would be tempted to drop at least the '_read_' part from the name, >>> the name is getting a bit too long for my taste. >> >> While I agree with the long-ish aspect, I'm afraid the "read" part is >> crucial. As a result I see no room for shortening. > > OK, if you think that's crucial then I'm not going to argue. > >>>> + struct domain *d_ = (d); \ >>> >>> Why do you need this local domain variable? Can't you use the d >>> parameter directly? >> >> It would be evaluated then somewhere between 0 and 2 times. > > It's ASSERT code only, so I don't see that as an issue. Fair point. > Otherwise d_ needs to be made const. Indeed, but for assert-only code I agree the option is slightly better, ideally suitably commented upon. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |