Hi Roger and Daniel P. Smith,
On 2023/11/28 22:08, Roger Pau Monné wrote:
> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the vpci on Xen
>> side won't get notification, so the cached state in vpci is
>> all out of date compare with the real device state.
>> To solve that problem, this patch add new hypercall to clear
>> all vpci device state. And when reset device happens on dom0
>> side, dom0 can call this hypercall to notify vpci.
>>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> Signed-off-by: Huang Rui <ray.hu...@amd.com>
>> ---
>> xen/arch/x86/hvm/hypercall.c | 1 +
>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++
>> xen/drivers/pci/physdev.c | 14 ++++++++++++++
>> xen/drivers/vpci/vpci.c | 9 +++++++++
>> xen/include/public/physdev.h | 2 ++
>> xen/include/xen/pci.h | 1 +
>> xen/include/xen/vpci.h | 6 ++++++
>> 7 files changed, 54 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
>> index eeb73e1aa5..6ad5b4d5f1 100644
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
>> arg)
>> case PHYSDEVOP_pci_mmcfg_reserved:
>> case PHYSDEVOP_pci_device_add:
>> case PHYSDEVOP_pci_device_remove:
>> + case PHYSDEVOP_pci_device_state_reset:
>> case PHYSDEVOP_dbgp_op:
>> if ( !is_hardware_domain(currd) )
>> return -ENOSYS;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 04d00c7c37..f871715585 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>> return ret;
>> }
>>
>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn)
>
> You could use pci_sbdf_t here instead of 3 parameters.
Will change in next version, thank you.
>
> I'm however unsure whether we really need this helper just to fetch
> the pdev and call vpci_reset_device_state(), I think you could place
> this logic directly in pci_physdev_op(). Unless there are plans to
> use such logic outside of pci_physdev_op().
If I place the logic of pci_reset_device_state directly in pci_physdev_op. I
think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I
will change in next version.
>
>> +{
>> + struct pci_dev *pdev;
>> + int ret = -ENODEV;
>
> Some XSM check should be introduced fro this operation, as none of the
> existing ones is suitable. See xsm_resource_unplug_pci() for example.
>
> xsm_resource_reset_state_pci() or some such I would assume.
I don't know what I should do in XSM function(assume it is
xsm_resource_reset_state_pci).
Hi Daniel P. Smith, could you please give me some suggestions?
Just to check the XSM_PRIV action?
>
> (adding the XSM maintainer for feedback).
>
>> +
>> + pcidevs_lock();
>> +
>> + pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
>> + if ( !pdev )
>> + goto error;
>> +
>> + ret = vpci_reset_device_state(pdev);
>> + if (ret)
>> + printk(XENLOG_ERR "PCI reset device %pp state failed\n",
>> &pdev->sbdf);
>> +
>> +error:
>> + pcidevs_unlock();
>> +
>> + return ret;
>> +}
>> +
>> /* Caller should hold the pcidevs_lock */
>> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>> uint8_t devfn)
>> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
>> index 42db3e6d13..cfdb545654 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -67,6 +67,20 @@ ret_t pci_physdev_op(int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> break;
>> }
>>
>> + case PHYSDEVOP_pci_device_state_reset: {
>> + struct physdev_pci_device dev;
>> +
>> + if ( !is_pci_passthrough_enabled() )
>> + return -EOPNOTSUPP;
>> +
>> + ret = -EFAULT;
>> + if ( copy_from_guest(&dev, arg, 1) != 0 )
>> + break;
>> +
>> + ret = pci_reset_device_state(dev.seg, dev.bus, dev.devfn);
>> + break;
>> + }
>> +
>> default:
>> ret = -ENOSYS;
>> break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 3bec9a4153..e9c3eb1cd6 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -103,6 +103,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>
>> return rc;
>> }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev)
>> +{
>> + ASSERT(pcidevs_locked());
>> +
>> + vpci_remove_device(pdev);
>> + return vpci_add_handlers(pdev);
>> +}
>> +
>> #endif /* __XEN__ */
>>
>> static int vpci_register_cmp(const struct vpci_register *r1,
>> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
>> index f0c0d4727c..4156948903 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -305,6 +305,8 @@ struct physdev_pci_device {
>> typedef struct physdev_pci_device physdev_pci_device_t;
>> DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
>>
>> +#define PHYSDEVOP_pci_device_state_reset 32
>
> This needs some comment in order to explain the expected behaviour,
> and might be better placed a bit up after PHYSDEVOP_release_msix.
I will add some comment and move it closer to PHYSDEVOP_release_msix in next
version. Thank you.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
