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

Re: [PATCH v8 04/13] vpci: add hooks for PCI device assign/de-assign


  • To: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 26 Jul 2023 10:42:48 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=Xu5DTKTOnkrNQYMrvMddhJv/W0069m8B2szg4ae1OLo=; b=c5rZ10XgjdgakCgGUt6XRaEHyv1+oYQYh8PcYQfO6FitUF3v9BR4XsY012BUb3jkgmKQafS0Fawm6xyISBi/b81/RU0cmeY1u3jsmmIj2vV/AcMOSt0TmfuhstAVuCOPlCR1Kd85kaYycpgSi77ztnoz2qyVLNcBcgdSLniRQGSHobAxgbyzTEr8zskqEoVIBWrYmkVOC6nvjXYSp3dmS/JcJS9jh9PcW7+zL6S/YZH89zmVBKoiSdtwI9u9FgWgTmsOuzgaNMz1l0mKwh+zuLOwOv5mFTuUdqxg0JYu/uZnullKPU4N9IHGj9BKssobtrdAfG3CC/PY//PEhBsJCA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G+ErOwoU0amxwD9Eqt3+F/hkMP/Phzq9Q68FbdPY1ovIB9LU9RuUzetRrIpcOtITySKNbGCSCDqQ12DlE7qgo70yhohyS30hQ4I0pq0ISwplXRMtr4bacNeYeNyjPzWwkq+KMLYKDlkh146CWs3KM73/PeOnq44SZIx1CG7Fe585oz3Di+bYEL3eer1iD+dNNhk/0+II3n2r5cS+7HR7SF0b9LyETaiRIbut/iMr83LClHqUS/sbCGG+tprYNjrdmPshRUC6GuHZkXwbBJkX34rA3d9/hFlKAaVQsQhZbU77bj7szsSUSpWTk9qsI8mov2zsqHq8tOSzr1bPshSiEA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Delivery-date: Wed, 26 Jul 2023 08:43:10 +0000
  • Ironport-data: A9a23:VjU3IK783t4WQsaZxnBqeAxRtP/GchMFZxGqfqrLsTDasY5as4F+v mVNDWqFPazfZTGjf9FyPd7j/BtTv8fTzYdrGgdkrX0xHi5G8cbLO4+Ufxz6V8+wwm8vb2o8t plDNYOQRCwQZiWBzvt4GuG59RGQ7YnRGvynTraCYnsrLeNdYH9JoQp5nOIkiZJfj9G8Agec0 fv/uMSaM1K+s9JOGjt8B5mr9lU35ZwehBtC5gZlPa8R4QeE/5UoJMl3yZ+ZfiOQrrZ8RoZWd 86bpJml82XQ+QsaC9/Nut4XpWVTH9Y+lSDX4pZnc/DKbipq/0Te4Y5iXBYoUm9Fii3hojxE4 I4lWapc6+seFvakdOw1C3G0GszlVEFM0OevzXOX6aR/w6BaGpdFLjoH4EweZOUlFuhL7W5m1 KRDFAJdcEq4rPOI57iJTdJCt/t8BZy+VG8fkikIITDxK98DGMqGaYOaoNhS0XE3m9xEGuvYa 4wBcz1zYR/cYhpJfFAKFJY5m+TujX76G9FagAvN+exrvC6OkUooj+CF3Nn9I7RmQe1PmUmVv CTe9nnRCRAGLt2PjzGC9xpAg8eWx3ujCdNMTefQGvhCgkaM1GorMEwsZGC4p9ScsVahBtZiA hlBksYphe1onKCxdfHDWBm/rG+BrwQrcdNaGO0n6ymA0qPRpQ2eAwAsXjNHLdArqsIybTgrz UOS2cPkAyR1t7+YQm7b8a2bxRuVPSUWNmYEaTUzZA0J+cT4oIozgxTMSf5uCKewyNbyHFnNL yuiqSE/g/AZi54N3qDip1Tf2Wvz/t7OUxI/4RjRUiS99ARlaYW5Zouur1/G8fJHK4XfRV6E1 JQZp/WjACk1JcnlvESwrC8lRtlFO97t3OXgvGNS
  • Ironport-hdrordr: A9a23:GsSU36+CbKg7mXsR6RJuk+HWdr1zdoMgy1knxilNoENuE/Bwxv rBoB1E73DJYW4qKQ8dcdDpAtjmfZquz+8F3WBxB8bsYOCCgguVxe5ZnPDfK7OLIVyFygcw79 YET0E6MqyOMbEYt7e13ODbKadc/DDvysnB7o2yowYPPGNXguNbnntE422gYytLrXx9dOIE/e 2nl7N6TlSbCBAqR/X+IkNAc/nIptXNmp6jSRkaByQ/4A3LoSK05KX8Gx242A5bdz9U278t/U XMjgS8v8yYwrGG4y6Z81WWw4VdmdPnxNcGLMuQivINIjGpphe0aJ9nU7iiuilwhO208l4lnP TFvh9lFcVu7HH6eH2zvHLWqkjd+Qdrz0Wn5U6TgHPlr8C8bik9EdB9iYVQdQacw1Y8vflnuZ g7nV6xht5yN1ftjS7979/HW1VBjUyvu0cvluYVkjh2TZYeUrlMtoYSlXklVavoXRiKrLzPIt MeSv0018wmKG9yqEqp5lWH9ebcGUjb2C32GXTq9PbliQS+10oJv3fwjPZv7UvosqhNCKWsrt 60QJhAhfVASNQbYrl6A/pEScyrCnbVSRaJK26KJ0/7fZt3cU4lhqSHqInd3tvaM6Ag3d83gt DMQVlYvWk9dwbnDtCPxoRC9lTITH+mVTrgx8lC79wh04eMCIbDIGmGUhQjgsGgq/IQDonSXO uyIotfB7vmIXH1EYhE0gXiU91ZKGUYUscSptEnMmj+7/7jO8nvrKjWYfzTLL3iHXItXX7+GG IKWHzpKMBJ/imQKzbFadjqKgXQk2DEjOVN+fLhjp0uIaA2R/lxjjQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Jul 26, 2023 at 01:38:30AM +0000, Volodymyr Babchuk wrote:
> 
> Hi Roger,
> 
> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
> 
> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >> @@ -1509,6 +1517,19 @@ static int assign_device(struct domain *d, u16 seg, 
> >> u8 bus, u8 devfn, u32 flag)
> >>          rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
> >>                          pci_to_dev(pdev), flag);
> >>      }
> >> +    if ( rc )
> >> +        goto done;
> >> +
> >> +    devfn = pdev->devfn;
> >> +    write_lock(&pdev->domain->pci_lock);
> >> +    rc = vpci_assign_device(pdev);
> >> +    write_unlock(&pdev->domain->pci_lock);
> >> +    if ( rc && deassign_device(d, seg, bus, devfn) )
> >> +    {
> >> +        printk(XENLOG_ERR "%pd: %pp was left partially assigned\n",
> >> +               d, &PCI_SBDF(seg, bus, devfn));
> >
> > &pdev->sbdf?  Then you can get of the devfn usage above.
> 
> Yes, thanks.
> 
> >> +        domain_crash(d);
> >
> > This seems like a bit different from the other error paths in the
> > function, isn't it fine to return an error and let the caller handle
> > the deassign?
> 
> I believe, intention was to not leave device in an unknown state: we
> failed both assign_device() and deassign_device() call, so what to do
> now? But yes, I think you are right and it is better to let caller to
> decide what to do next.

I don't think it would be a security risk to leave the device in that
state.  For domUs the guest won't get access to the device registers
anyway as we use an allow list approach.  Also deassign_device() is
not called when we fail to assign one of the phantom functions.

We don't seem to undo any of the work in assign_device() on error so
it should be fine to not do the call to deassign_device() on error to
initialize vPCI.

> >
> > Also, if we really need to call deassign_device() we must do so for
> > all possible phantom devices, see the above loop around
> > iommu_call(..., assing_device, ...);
> 
> But deassign_device() has the loop for all phantom devices that already
> does all the work. Unless I miss something, of course.

No, you are right, a single call is fine.

Thanks, Roger.



 


Rackspace

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