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

Re: [PATCH 2/9] vpci: Add hooks for PCI device assign/de-assign


  • To: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 6 Sep 2021 15:23:11 +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; bh=xCq+T/w9za/+vXVmsi/kL7xE4T9zITd+xcOCO9ublJ0=; b=JnVGxIU75PhBdciQs+8Efy4TB0uw/Xu+FGQMdZiFVUfdKuWJQ8XCLsNfY3KsuIIKmzkVK2DsBmi/mMZYNFk+Lrb/vjroxCWwmGIYi8wl6C+lfL/YMlreKZzl4pU6zZrrLoatI67UDuTTkUZbuko1/jypEReD0/c4nhOBvXJEgemQjujJ/D+wrfJFO9/6UTy6sEmHNzcXQnKM9xQ72ZmtH+w2rGFvhFE4b9BMbo2OBdLXZ+XX0aSKJKUhWwo223kUO8zBQdlIY0QWVXHEXpqmlDCMcn0pnVJchKNExspfiSgRppyZqpKd/626laliH1ctMby5pKZZhUYXoUBivUhRBA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HhMlIlDF6bEYB3ozbM+KscYSqqEhkfQN/rd5rXwgBDIpfqg3venWAn+QQsoyonh+drME+6VsMgeLDMmM8s02/6WJs5aeRdhIb/96HbLQg8fnOZmqzoFFwZkGvNH/+JOG54pwmyup4i8hrLYEZlcZ0iisqaMp1B+/iI/SZv3/QesBpuOTcLZMZMLoo6RszDizzfVV4XBnhPfRgIH+lMIjC5aKX5rjd2sAgJd/gCBrPS7ZnkRhbnKyhUn9ZJaSXpIjhVDUt97Tq2cBnIEAtflIdr1lxigIndooVRRpI2816/LyDqgWNTkSglmk9gmarOlrKO9jnVpCHXAdEtPwxfN+6Q==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: julien@xxxxxxx, sstabellini@xxxxxxxxxx, oleksandr_tyshchenko@xxxxxxxx, volodymyr_babchuk@xxxxxxxx, Artem_Mygaiev@xxxxxxxx, roger.pau@xxxxxxxxxx, bertrand.marquis@xxxxxxx, rahul.singh@xxxxxxx, Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 06 Sep 2021 13:23:21 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -864,6 +864,10 @@ static int deassign_device(struct domain *d, uint16_t 
> seg, uint8_t bus,
>      if ( ret )
>          goto out;
>  
> +    ret = vpci_deassign_device(d, pdev);
> +    if ( ret )
> +        goto out;
> +
>      if ( pdev->domain == hardware_domain  )
>          pdev->quarantine = false;
>  
> @@ -1425,6 +1429,11 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>          rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
> flag);
>      }
>  
> +    if ( rc )
> +        goto done;
> +
> +    rc = vpci_assign_device(d, pdev);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

I have to admit that I'm worried by the further lack of unwinding in
case of error: We're not really good at this, I agree, but it would
be quite nice if the problem didn't get worse. At the very least if
the device was de-assigned from Dom0 and assignment to a DomU failed,
imo you will want to restore Dom0's settings.

Also in the latter case don't you need to additionally call
vpci_deassign_device() for the prior owner of the device?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct domain *d, struct pci_dev *dev)
> +{
> +    /* It only makes sense to assign for hwdom or guest domain. */
> +    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )

Please don't open-code is_system_domain(). I also think you want to
flip the two sides of the ||, to avoid evaluating whatever has_vcpi()
expands to for system domains. (Both again below.)

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>  /* Add vPCI handlers to device. */
>  int __must_check vpci_add_handlers(struct pci_dev *dev);
>  
> +/* Notify vPCI that device is assigned to guest. */
> +int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);

Is the expectation that "dev" may get altered? If not, it may want to
become pointer-to-const. (For "d" there might be the need to acquire
locks, so I guess it might not be a god idea to constify that one.)

I also think that one comment ought to be enough for the two functions.

Jan




 


Rackspace

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