[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.
|