On 27.07.22 13:03, Jan Beulich wrote:
Hello Jan
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 ...
ok
@@ -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?
As I understand that was the intention of the author. At least I don't
see other reasons.
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.
Thanks, good point, will drop old_devfn and use pdev->devfn. I am
wondering whether the printk after "done:" label (and other possible
printk-s down the code) should really use pdev->devfn instead of devfn
in PCI_SBDF construct?
+ 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.
I will probably add another log message before domain_crash() leaving
this one as is.
printk(XENLOG_ERR "%pd: %pp was left partially assigned\n", d,
&PCI_SBDF(seg, bus, devfn));
--- 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.)
Agree, but ...
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.
... exactly, the later patches extend this function. I will update
commit description.
Jan
--
Regards,
Oleksandr Tyshchenko
