[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 17:58, Stewart Hildebrand wrote: > On 2/13/24 04:05, Jan Beulich wrote: >> 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. > > We will then also need a definition of pdev_list_is_read_locked() in the > #else case so we don't risk running into "error: implicit declaration of > function 'pdev_list_is_read_locked'". > > Such a definition might look like: > > #define pdev_list_is_read_locked(d) ({ (void)d; ASSERT_UNREACHABLE(); false; > }) > > so that we still evaluate d exactly once in the NDEBUG case. Except that ASSERT_UNREACHABLE() use is bogus in the NDEBUG case. The way our ASSERT() works in the NDEBUG case looks to make it sufficient for there to be bool pdev_list_is_read_locked(const struct domain *d); in the #else case (with no implementation anywhere). >>>>>> + * 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. > > Is "the option" here referring to making d_ const, or using d directly > (with suitable comment)? The latter. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |