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

Re: [PATCH V7 02/11] vpci: add hooks for PCI device assign/de-assign


  • To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 27 Jul 2022 12:03:04 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; 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=20WCdnFKcg8tMGaKS2EGRh5BbCB1Drrb+6HRMX83pUw=; b=Rr8IKWeUacH4Zt2Ers8amLVRL6BekJ0YxFmUB0gqSmbuAfysJfhPdtSdIFXrtnQShzrOIkvolRR4QhEpnqAKwJquC9CY+anh87c11z2uJmt+JngGuCQf93efgYidb7T+PBLmO6b44BXAig3dSzogvdHn68E36tKKnTQ6zIiCKjoioNGR3Md4yktoYExOe7HHDOXjd51GMjhHtWaHnOoIzey9hdG41iXdyh1S+a0IX9hr8V2Lr+3QgXZ+lkVJRM33/TTjz9CWy0VG2Jntho3OgiINbo2lYJuPpEcCXIeX0JGuL/vrKW50j/5YLEo/JFEqmOVyPi3QYSqm4yplVtxlIg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NVIbKDcdUie00KiJyt4b9zSTO8sSKlBHdorAecxxsdap0Jxr/KogyPuwsjArp7V2DUOu9n56ek87wQGA6GFkINOLmNz/76nCe/vTWaPU/KhpLJK5CXr8YFYafzCtrTYK0vTyxHeHqmZHVBO8hq3K2GwE92sph0WG9lUp2QbBsI7204oAxY9bvyqL6lfBBHoUe6TkHYXTttpcisRhKhov0a3RP1C9qYRubyQJknOR6TMmjROAoC5y5xmNtIQU20/imd4Pp3iZI7Tiv0g248rGDwYr4gJ1Td/lIY3uBrG/hdyNPBFBKSboQ9ZOeIsLVU8+yz8kHz2CQBy18YWZVqdTaw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 27 Jul 2022 10:03:28 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> @@ -1558,6 +1560,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
>      struct pci_dev *pdev;
> +    uint8_t old_devfn;

Why "old"? There's nothing "new" here. Perhaps "orig", considering ...

> @@ -1594,6 +1599,8 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>                            pci_to_dev(pdev), flag)) )
>          goto done;
>  
> +    old_devfn = devfn;
> +
>      for ( ; pdev->phantom_stride; rc = 0 )
>      {
>          devfn += pdev->phantom_stride;

... this updating of devfn is what you mean to deal with? Then again
I see no need for a separate variable in the first place. The input
(seg,bus,devfn) tuple is translated to a pdev, so you can simply ...

> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>                          pci_to_dev(pdev), flag);
>      }
>  
> +    rc = vpci_assign_device(pdev);
> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )

... use pdev->devfn here.

> +        domain_crash(d);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

This log message will want to appear _before_ the domain_crash()
related output, or you will want to add another log message there.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -92,6 +92,37 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    ASSERT(pcidevs_write_locked());
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return 0;
> +
> +    rc = vpci_add_handlers(pdev);
> +    if ( rc )
> +        vpci_deassign_device(pdev);
> +
> +    return rc;
> +}
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +void vpci_deassign_device(struct pci_dev *pdev)
> +{
> +    ASSERT(pcidevs_write_locked());
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return;

There's no need for this check since ...

> +    vpci_remove_device(pdev);

... this function already has it. At which point the question is why
a separate function needs to exist in the first place. To match with
vpci_assign_device(), a simple #define to alias is would be enough.
(This is one of the cases where personally I think a #define is
superior to an inline wrapper.)

Unless, of course, later patches extend this function. If so, the
commit message giving this as justification for the introduction of
(apparent) redundancy would be helpful.

Jan



 


Rackspace

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