[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 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. >> + * 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. >> + 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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |