| Message ID | 20240204061257.1408243-1-Emily.Deng@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: Add vf reset notification for pf | expand |
On Sun, Feb 04, 2024 at 02:12:57PM +0800, Emily Deng wrote: > @@ -926,6 +926,7 @@ struct pci_driver { > int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */ > int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */ > u32 (*sriov_get_vf_total_msix)(struct pci_dev *pf); > + void (*sriov_vf_reset_notification)(struct pci_dev *pf, struct pci_dev *vf); You've created a new callback, but there is no user. Could you resubmit this with an in-kernel use case?
On Sun, Feb 04, 2024 at 02:12:57PM +0800, Emily Deng wrote: > When a vf has been reset, the pf wants to get notification to remove the vf > out of schedule. It is very questionable if this is right thing to do. The idea of SR-IOV is that VFs represent a physical device and they should be treated separately from the PF. In addition to that Keith said, this patch needs better justification. Thanks > > Solution: > Add the callback function in pci_driver sriov_vf_reset_notification. When > vf reset happens, then call this callback function. > > Signed-off-by: Emily Deng <Emily.Deng@amd.com> > --- > drivers/pci/pci.c | 8 ++++++++ > include/linux/pci.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 60230da957e0..aca937b05531 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4780,6 +4780,14 @@ EXPORT_SYMBOL_GPL(pcie_flr); > */ > int pcie_reset_flr(struct pci_dev *dev, bool probe) > { > + struct pci_dev *pf_dev; > + > + if (dev->is_virtfn) { > + pf_dev = dev->physfn; > + if (pf_dev->driver->sriov_vf_reset_notification) > + pf_dev->driver->sriov_vf_reset_notification(pf_dev, dev); > + } > + > if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > return -ENOTTY; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c69a2cc1f412..4fa31d9b0aa7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -926,6 +926,7 @@ struct pci_driver { > int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */ > int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */ > u32 (*sriov_get_vf_total_msix)(struct pci_dev *pf); > + void (*sriov_vf_reset_notification)(struct pci_dev *pf, struct pci_dev *vf); > const struct pci_error_handlers *err_handler; > const struct attribute_group **groups; > const struct attribute_group **dev_groups; > -- > 2.36.1 > >
[AMD Official Use Only - General] The use case is the vfio-pci driver, which is used to support a user mode PF driver. Will also sent out the vfio-pci driver patch. And add more comments in the patch. Thanks. Emily Deng Best Wishes >-----Original Message----- >From: Leon Romanovsky <leon@kernel.org> >Sent: Sunday, February 4, 2024 7:21 PM >To: Deng, Emily <Emily.Deng@amd.com> >Cc: amd-gfx@lists.freedesktop.org; bhelgaas@google.com; >alex.williamson@redhat.com; linux-pci@vger.kernel.org; linux- >kernel@vger.kernel.org; kvm@vger.kernel.org >Subject: Re: [PATCH] PCI: Add vf reset notification for pf > >On Sun, Feb 04, 2024 at 02:12:57PM +0800, Emily Deng wrote: >> When a vf has been reset, the pf wants to get notification to remove >> the vf out of schedule. > >It is very questionable if this is right thing to do. The idea of SR-IOV is that >VFs represent a physical device and they should be treated separately from >the PF. > >In addition to that Keith said, this patch needs better justification. > >Thanks > >> >> Solution: >> Add the callback function in pci_driver sriov_vf_reset_notification. >> When vf reset happens, then call this callback function. >> >> Signed-off-by: Emily Deng <Emily.Deng@amd.com> >> --- >> drivers/pci/pci.c | 8 ++++++++ >> include/linux/pci.h | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index >> 60230da957e0..aca937b05531 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4780,6 +4780,14 @@ EXPORT_SYMBOL_GPL(pcie_flr); >> */ >> int pcie_reset_flr(struct pci_dev *dev, bool probe) { >> + struct pci_dev *pf_dev; >> + >> + if (dev->is_virtfn) { >> + pf_dev = dev->physfn; >> + if (pf_dev->driver->sriov_vf_reset_notification) >> + pf_dev->driver->sriov_vf_reset_notification(pf_dev, >dev); >> + } >> + >> if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) >> return -ENOTTY; >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h index >> c69a2cc1f412..4fa31d9b0aa7 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -926,6 +926,7 @@ struct pci_driver { >> int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */ >> int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int >msix_vec_count); /* On PF */ >> u32 (*sriov_get_vf_total_msix)(struct pci_dev *pf); >> + void (*sriov_vf_reset_notification)(struct pci_dev *pf, struct >> +pci_dev *vf); >> const struct pci_error_handlers *err_handler; >> const struct attribute_group **groups; >> const struct attribute_group **dev_groups; >> -- >> 2.36.1 >> >>
Am 04.02.24 um 07:12 schrieb Emily Deng: > When a vf has been reset, the pf wants to get notification to remove the vf > out of schedule. > > Solution: > Add the callback function in pci_driver sriov_vf_reset_notification. When > vf reset happens, then call this callback function. Well that doesn't make much sense. As other already noted as well a VF should be an encapsulated representation of a physical devices functionality. AMD implemented that a bit different with a hypervisor to control which PF functionality a VF exposes, but that doesn't mean that we can leak this AMD specific handling into the common Linux PCI subsystem. Additional to that a technical blocker is that when a VF is passed into a VM you don't have access to the PF any more to make this reset notification. Regards, Christian. > > Signed-off-by: Emily Deng <Emily.Deng@amd.com> > --- > drivers/pci/pci.c | 8 ++++++++ > include/linux/pci.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 60230da957e0..aca937b05531 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4780,6 +4780,14 @@ EXPORT_SYMBOL_GPL(pcie_flr); > */ > int pcie_reset_flr(struct pci_dev *dev, bool probe) > { > + struct pci_dev *pf_dev; > + > + if (dev->is_virtfn) { > + pf_dev = dev->physfn; > + if (pf_dev->driver->sriov_vf_reset_notification) > + pf_dev->driver->sriov_vf_reset_notification(pf_dev, dev); > + } > + > if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > return -ENOTTY; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c69a2cc1f412..4fa31d9b0aa7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -926,6 +926,7 @@ struct pci_driver { > int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */ > int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */ > u32 (*sriov_get_vf_total_msix)(struct pci_dev *pf); > + void (*sriov_vf_reset_notification)(struct pci_dev *pf, struct pci_dev *vf); > const struct pci_error_handlers *err_handler; > const struct attribute_group **groups; > const struct attribute_group **dev_groups;
On Sun, 4 Feb 2024 14:12:57 +0800 Emily Deng <Emily.Deng@amd.com> wrote: > When a vf has been reset, the pf wants to get notification to remove > the vf out of schedule. > > Solution: > Add the callback function in pci_driver sriov_vf_reset_notification. > When vf reset happens, then call this callback function. > > Signed-off-by: Emily Deng <Emily.Deng@amd.com> > --- > drivers/pci/pci.c | 8 ++++++++ > include/linux/pci.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 60230da957e0..aca937b05531 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4780,6 +4780,14 @@ EXPORT_SYMBOL_GPL(pcie_flr); > */ > int pcie_reset_flr(struct pci_dev *dev, bool probe) > { > + struct pci_dev *pf_dev; > + > + if (dev->is_virtfn) { > + pf_dev = dev->physfn; > + if (pf_dev->driver->sriov_vf_reset_notification) > + > pf_dev->driver->sriov_vf_reset_notification(pf_dev, dev); > + } > + > if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > return -ENOTTY; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c69a2cc1f412..4fa31d9b0aa7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -926,6 +926,7 @@ struct pci_driver { > int (*sriov_configure)(struct pci_dev *dev, int num_vfs); > /* On PF */ int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int > msix_vec_count); /* On PF */ u32 (*sriov_get_vf_total_msix)(struct > pci_dev *pf); > + void (*sriov_vf_reset_notification)(struct pci_dev *pf, > struct pci_dev *vf); const struct pci_error_handlers *err_handler; > const struct attribute_group **groups; > const struct attribute_group **dev_groups; Hi: I would suggest you can provide a cover letter including a complete picture that tells the background, detailed problem statement, the solutions and plus the users. As this seems very like a generic change, it needs a better justification to convince folks why this is the best solution. Without a complete picture, the solution just looks like a workaround. Thanks, Zhi.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 60230da957e0..aca937b05531 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4780,6 +4780,14 @@ EXPORT_SYMBOL_GPL(pcie_flr); */ int pcie_reset_flr(struct pci_dev *dev, bool probe) { + struct pci_dev *pf_dev; + + if (dev->is_virtfn) { + pf_dev = dev->physfn; + if (pf_dev->driver->sriov_vf_reset_notification) + pf_dev->driver->sriov_vf_reset_notification(pf_dev, dev); + } + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) return -ENOTTY; diff --git a/include/linux/pci.h b/include/linux/pci.h index c69a2cc1f412..4fa31d9b0aa7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -926,6 +926,7 @@ struct pci_driver { int (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */ int (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */ u32 (*sriov_get_vf_total_msix)(struct pci_dev *pf); + void (*sriov_vf_reset_notification)(struct pci_dev *pf, struct pci_dev *vf); const struct pci_error_handlers *err_handler; const struct attribute_group **groups; const struct attribute_group **dev_groups;
When a vf has been reset, the pf wants to get notification to remove the vf out of schedule. Solution: Add the callback function in pci_driver sriov_vf_reset_notification. When vf reset happens, then call this callback function. Signed-off-by: Emily Deng <Emily.Deng@amd.com> --- drivers/pci/pci.c | 8 ++++++++ include/linux/pci.h | 1 + 2 files changed, 9 insertions(+)