[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] vpci: introduce per-domain lock to protect vpci structure


  • To: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 14 Feb 2022 12:11:12 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=AxbcAcIV1mqtbUwec8vq0aeTNEJjfjry4Um6tFjlGaM=; b=CZHI/tz8+5Zvizk3NLmgr54VBpk2+7dbUVxzP9NBdbTNnE7bn7o20Uxre7xZyk4P2Hy0nCIZRvUv3pYxqGUuCeB/z/6uk2g5vEbUWNaXc88oScA1dz2buANw8kNCTIo1u+wjExRdhE4NrM7jKe8JKIry10LgD6kSN87HzuX3OR5XUi/zVCgr+W79vYWssvQkFt1fHqDgeerWBfCoGFR7LGZEGC3Gw/jNtmStEFDMBjEwV8/fbj5++zpfOVN37rAu6fsOYLB84bESinMBZTGF4kYz9DvelFukW4DKy7QdhKxfF0GWsku5zQnoM6ObKG6n3Xad77edLYjSoCO88nXDCQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZjPLYZZmBwsobBDPx7+BH89dppX3eeuOjlikmoYzSlE5lA2kzIiXCPC1FYxPoNJpeJumwGTuYdmQ9n4trSZybB+lHw+J8lmrIqOuBFrRYP0STcCnFsRn46MpOb274uqDGnqhSe7US7iZtXBO0F2MlfNDD2aBtODOcSvpNls6D6QvDN1bg2nWRA+0prILBubP/Bi8zIVV6LylCSGqZr7VkbivoNMoq/Xbykwx6Te6spzQ6c1oCYdLU5ztZc3iZiwgfIAL6ZbSMPqjlW3neLM5JirsCvbZBgKgvZnToMgp7Aj3F6pyxd+oaxAR6bMU4ugWgjDi+jQJKRJ5q99yotLPxw==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>
  • Delivery-date: Mon, 14 Feb 2022 11:11:31 +0000
  • Ironport-data: A9a23:p5RdLaO8GJQvuufvrR2skcFynXyQoLVcMsEvi/4bfWQNrUp0hTVRn zcXWmGEbqyIYGD9fdFzbo619hgC7ZCHyYcxSQto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6UUsxNbVU8En1500s/w7dRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYoz6Ao45/y tsKj8OxEVg5HrLRo6cMfiANRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/uWvYMDg29t7ixINe3XO towNCBzVg7vWSQTalI4BrIBx8790xETdBUH8QnI9MLb+VP78gt117T8NcvPTfaDT85Vg0Wwq 3rP+iLyBRRyHPubxDme+3SgnNjzjDj7U4IfEr6/3vNyiVjVzWsWYDU/T0e2pM6cm0G3Wt9BA 0EM8y9opq83nGSxR9ngW1ujoXiLvjYVQd8WGOo/gCmHx7DI+Q+fCi4BRyRYdd09nMYsQHoh0 Vrht8PkA3ljvaOYTVqZ96yItnWiNC4NN2gAaCQYCwwf7LHLh4U+jQnGSNp5J4ezgsfoAjH7w z2Mry8Wiq0aiIgA0KDT1VLNji+op5PJZhUo/QiRVWWghitif5KsbYGs7Vnd7N5DIZyfQ13Hu 2IL8+Cc8eQPAJelhCGLBuIXE9mUC+2taWOGxwQ1Rt94qmrrqyXLkZ1sDC9WL05VbMgGd3jTP 06L5hNx+4Jof2qHVPoiC26uMPgCwa/lHNXjc/nbaNtSf5R8HDO6EDFSiV24hD60zhV1+U0rE dLCKJv3Uy5GYUhy5GfuH48gPakXKjfSLI85bbTy1FyZ3LWXfxZ5op9VYQLVPojVAE5pyTg5E uqz1ePXkX2zs8WkO0E7FLL/ynhQcxAG6Wje8ZA/SwJ6ClMO9JsdI/HQ26g9XIdugr5YkOzFl lnkBBMElwqv2CCfcF7QApyGVF8IdcwhxU/XwAR2ZQr4s5TdSdrHAFgjm2sfIuB8qb0LIQ9cR PgZYcSQasmjuRyckwnxmaLV9dQ4HDzy3FrmF3P8PFAXIs4xLySUq4SMVla+q0Ez4t+f6JJWT 0uIjViAH/Luhm1KUa7rVR5Y5w3t7CZNwLooByMl4LB7IS3RzWSjEASo5tcfKMAQMxTTgDyc0 gedGxADoufR5YQy9bH0aWqs9u9FysNyQRhXGXf197GzOXWI92av29YYAu2JYSrcRCX//6D7P bdZyPT1MfsmmldWstUjT+Y3nPxmv9a/9aVHyglEHWnQawj5AL1XPXTbj9JEsbdAx+EFtFLuC F6P4NRTJZ6AJNjhTAwKPAMgY+najaMUlzDe4O4bOkL/4CMrrrOLXV8LZ0uHiTBHLaszO4Qgm L9ztMkT4g25qxwrLtfZ0XwEqzXSdiQNCvx1uIsbDYnnjhsQ5mtDOZGMWDXr5JyvaslXNhV4K DGjm6ef1a9XwVDPciRvGCGVj/Zdn5kHpDtD0EQGewaSgtPAi/I6gE9R/DAwQlgHxxlLybsua G1iNkkzLqSS5TZ4wsNEWjn0SQ1GARSY/G33ykcIyzKFHxX5CDSVIT1vI/uJ8WAY73lYL2pS8 7yvwWr4VSrnIZPq1SwoVE858/HuQLSdLOEZdBxLyyhdI6QHXA==
  • Ironport-hdrordr: A9a23:VtdhAKxk4Je236DgXxQqKrPxtuskLtp133Aq2lEZdPULSKKlfp GV88jziyWZtN9wYhEdcdDpAtjnfZr5z+8J3WB3B8bfYOCGghrTEGgG1+rfKlLbakjDH4JmpM Ndmu1FeaLN5DtB/LbHCWuDYq4dKbC8mcjC74qurAYOcegpUdAa0+4QMHfrLqQcfng+OXNWLu v62iIRzADQB0j/I/7LTEUtbqzmnZnmhZjmaRkJC1oO7xSPtyqh7PrfHwKD1hkTfjtTyfN6mF K13zDR1+GGibWW2xXc32jc49B/n8bg8MJKAIiphtIOIjvhpw60bMBKWqGEvhoyvOazgWxa3e XkklMFBYBe+nnRdma6rV/E3BTh6i8n7zvYxVqRkRLY0IXEbQN/L/AEqZNScxPf5UZllsp7yr h302WQsIcSJQ/cnQzmjuK4Fi1Cpw6Rmz4PgOQTh3tQXc81c7lKt7ES+0tTDdMpAD/60oY6C+ NjZfusqsq+SWnqLEwxg1MfguBFBh8Ib1K7qwk5y4OoOgFt7TBEJxBy/r1aop8CnKhNPaWsqd 60dZiAr4s+PPP+W5gNc9vpcfHHeVAlfii8RV56AW6XXJ3vaEi94KIe3t0OlZWXkdozvd0PpK g=
  • Ironport-sdr: GxV0ZSIrcLEFptTL9rcORFpEVNHERXBkvMyjemQNxaarfDJHDrPDBOq27E81Wa/V/7pG9ak9YJ C9Ex6F61BdVnjzFqz34BlJikzOeUSG3i4Y1EmuLP3uKg2zW+kr/VWRyeAGZ6ms873pF3qUqvL/ EMoD3nUd1RaLy0LfbS5f+REN8f5jAYWbvzMk6s0y62NCByF3oBUH1IdPSV/TKmMTelQcfeODCM bZO/3Rs5xedEQTcAt5HUoJMIN3nLBpNykT6vaPyzaZciah5c17YZAYcrVh2x/Z5VQPo721NPZK OCBBxyDyvPJ2bGd60iqsh8Ze
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Feb 14, 2022 at 10:53:43AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 14.02.22 12:34, Roger Pau Monné wrote:
> > On Mon, Feb 14, 2022 at 09:36:39AM +0000, Oleksandr Andrushchenko wrote:
> >>
> >> On 11.02.22 13:40, Roger Pau Monné wrote:
> >>> +
> >>>>>>         for ( i = 0; i < msix->max_entries; i++ )
> >>>>>>         {
> >>>>>>             const struct vpci_msix_entry *entry = &msix->entries[i];
> >>>>> Since this function is now called with the per-domain rwlock read
> >>>>> locked it's likely not appropriate to call process_pending_softirqs
> >>>>> while holding such lock (check below).
> >>>> You are right, as it is possible that:
> >>>>
> >>>> process_pending_softirqs -> vpci_process_pending -> read_lock
> >>>>
> >>>> Even more, vpci_process_pending may also
> >>>>
> >>>> read_unlock -> vpci_remove_device -> write_lock
> >>>>
> >>>> in its error path. So, any invocation of process_pending_softirqs
> >>>> must not hold d->vpci_rwlock at least.
> >>>>
> >>>> And also we need to check that pdev->vpci was not removed
> >>>> in between or *re-created*
> >>>>> We will likely need to re-iterate over the list of pdevs assigned to
> >>>>> the domain and assert that the pdev is still assigned to the same
> >>>>> domain.
> >>>> So, do you mean a pattern like the below should be used at all
> >>>> places where we need to call process_pending_softirqs?
> >>>>
> >>>> read_unlock
> >>>> process_pending_softirqs
> >>>> read_lock
> >>>> pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> >>>> if ( pdev && pdev->vpci && is_the_same_vpci(pdev->vpci) )
> >>>> <continue processing>
> >>> Something along those lines. You likely need to continue iterate using
> >>> for_each_pdev.
> >> How do we tell if pdev->vpci is the same? Jan has already brought
> >> this question before [1] and I was about to use some ID for that purpose:
> >> pdev->vpci->id = d->vpci_id++ and then we use pdev->vpci->id  for checks
> > Given this is a debug message I would be OK with just doing the
> > minimal checks to prevent Xen from crashing (ie: pdev->vpci exists)
> > and that the resume MSI entry is not past the current limit. Otherwise
> > just print a message and move on to the next device.
> Agree, I see no big issue (probably) if we are not able to print
> 
> How about this one:
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 809a6b4773e1..50373f04da82 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -171,10 +171,31 @@ static int __init apply_map(struct domain *d, const 
> struct pci_dev *pdev,
>                               struct rangeset *mem, uint16_t cmd)
>   {
>       struct map_data data = { .d = d, .map = true };
> +    pci_sbdf_t sbdf = pdev->sbdf;
>       int rc;
> 
> + ASSERT(rw_is_write_locked(&pdev->domain->vpci_rwlock));
> +
>       while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == 
> -ERESTART )
> +    {
> +
> +        /*
> +         * process_pending_softirqs may trigger vpci_process_pending which
> +         * may need to acquire pdev->domain->vpci_rwlock in read mode.
> +         */
> +        write_unlock(&pdev->domain->vpci_rwlock);
>           process_pending_softirqs();
> +        write_lock(&pdev->domain->vpci_rwlock);
> +
> +        /* Check if pdev still exists and vPCI was not removed or 
> re-created. */
> +        if (pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn) != 
> pdev)
> +            if ( vpci is NOT the same )
> +            {
> +                rc = 0;
> +                break;
> +            }
> +    }
> +
>       rangeset_destroy(mem);
>       if ( !rc )
>           modify_decoding(pdev, cmd, false);
> 
> This one also wants process_pending_softirqs to run so it *might*
> want pdev and vpci checks. But at the same time apply_map runs
> at ( system_state < SYS_STATE_active ), so defer_map won't be
> running yet, thus no vpci_process_pending is possible yet (in terms
> it has something to do yet). So, I think we just need:
> 
>          write_unlock(&pdev->domain->vpci_rwlock);
>          process_pending_softirqs();
>          write_lock(&pdev->domain->vpci_rwlock);
> 
> and this should be enough

Given the context apply_map is called from (dom0 specific init code),
there's no need to check for the pdev to still exits, or whether vpci
has been recreated, as it's not possible. Just add a comment to
explicitly note that the context of the function is special, and thus
there's no possibility of either the device or vpci going away.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.