On 2024/7/31 23:55, Roger Pau Monné wrote:
> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the Xen hypervisor
>> doesn't get notification, so the cached state in vpci is all
>> out of date compare with the real device state.
>>
>> To solve that problem, add a new hypercall to support the reset
>> of pcidev and clear the vpci state of device. So that once the
>> state of device is reset on dom0 side, dom0 can call this
>> hypercall to notify hypervisor.
>>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> Signed-off-by: Huang Rui <ray.hu...@amd.com>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> Reviewed-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
>> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
>
> Thanks, just a couple of nits.
>
> This is missing a changelog between versions, and I haven't been
> following all the versions, so some of my questions might have been
> answered in previous revisions.
Sorry, I will add changelogs here in next version.
>
>> ---
>> xen/arch/x86/hvm/hypercall.c | 1 +
>> xen/drivers/pci/physdev.c | 52 ++++++++++++++++++++++++++++++++++++
>> xen/drivers/vpci/vpci.c | 10 +++++++
>> xen/include/public/physdev.h | 16 +++++++++++
>> xen/include/xen/vpci.h | 8 ++++++
>> 5 files changed, 87 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
>> index 7fb3136f0c7c..0fab670a4871 100644
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -83,6 +83,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/pci/physdev.c b/xen/drivers/pci/physdev.c
>> index 42db3e6d133c..c0f47945d955 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,6 +2,7 @@
>> #include <xen/guest_access.h>
>> #include <xen/hypercall.h>
>> #include <xen/init.h>
>> +#include <xen/vpci.h>
>>
>> #ifndef COMPAT
>> typedef long ret_t;
>> @@ -67,6 +68,57 @@ ret_t pci_physdev_op(int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> break;
>> }
>>
>> + case PHYSDEVOP_pci_device_state_reset:
>> + {
>> + struct pci_device_state_reset dev_reset;
>> + struct pci_dev *pdev;
>> + pci_sbdf_t sbdf;
>> +
>> + ret = -EOPNOTSUPP;
>> + if ( !is_pci_passthrough_enabled() )
>> + break;
>> +
>> + ret = -EFAULT;
>> + if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
>> + break;
>> +
>> + sbdf = PCI_SBDF(dev_reset.dev.seg,
>> + dev_reset.dev.bus,
>> + dev_reset.dev.devfn);
>> +
>> + ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
>> + if ( ret )
>> + break;
>> +
>> + pcidevs_lock();
>> + pdev = pci_get_pdev(NULL, sbdf);
>> + if ( !pdev )
>> + {
>> + pcidevs_unlock();
>> + ret = -ENODEV;
>> + break;
>> + }
>> +
>> + write_lock(&pdev->domain->pci_lock);
>> + pcidevs_unlock();
>> + switch ( dev_reset.reset_type )
>> + {
>> + case PCI_DEVICE_STATE_RESET_COLD:
>> + case PCI_DEVICE_STATE_RESET_WARM:
>> + case PCI_DEVICE_STATE_RESET_HOT:
>> + case PCI_DEVICE_STATE_RESET_FLR:
>> + ret = vpci_reset_device_state(pdev, dev_reset.reset_type);
>> + break;
>> +
>> + default:
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> + write_unlock(&pdev->domain->pci_lock);
>> +
>> + break;
>> + }
>> +
>> default:
>> ret = -ENOSYS;
>> break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 1e6aa5d799b9..7e914d1eff9f 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>>
>> return rc;
>> }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev,
>> + uint32_t reset_type)
>
> There's probably no use in passing reset_type to
> vpci_reset_device_state() if it's ignored?
>
>> +{
>> + ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>> +
>> + vpci_deassign_device(pdev);
>> + return vpci_assign_device(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 f0c0d4727c0b..3cfde3fd2389 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -296,6 +296,13 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
>> */
>> #define PHYSDEVOP_prepare_msix 30
>> #define PHYSDEVOP_release_msix 31
>> +/*
>> + * Notify the hypervisor that a PCI device has been reset, so that any
>> + * internally cached state is regenerated. Should be called after any
>> + * device reset performed by the hardware domain.
>> + */
>> +#define PHYSDEVOP_pci_device_state_reset 32
>> +
>> struct physdev_pci_device {
>> /* IN */
>> uint16_t seg;
>> @@ -305,6 +312,15 @@ struct physdev_pci_device {
>> typedef struct physdev_pci_device physdev_pci_device_t;
>> DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
>>
>> +struct pci_device_state_reset {
>> + physdev_pci_device_t dev;
>> +#define PCI_DEVICE_STATE_RESET_COLD 0
>> +#define PCI_DEVICE_STATE_RESET_WARM 1
>> +#define PCI_DEVICE_STATE_RESET_HOT 2
>> +#define PCI_DEVICE_STATE_RESET_FLR 3
>> + uint32_t reset_type;
>
> This might want to be a flags field, with the low 2 bits (or maybe 3
> bits to cope if more rest modes are added in the future) being used to
> signal the reset type. We can always do that later if flags need to
> be added.
Do you mean this?
+struct pci_device_state_reset {
+ physdev_pci_device_t dev;
+#define _PCI_DEVICE_STATE_RESET_COLD 0
+#define PCI_DEVICE_STATE_RESET_COLD (1U<<_PCI_DEVICE_STATE_RESET_COLD)
+#define _PCI_DEVICE_STATE_RESET_WARM 1
+#define PCI_DEVICE_STATE_RESET_WARM (1U<<_PCI_DEVICE_STATE_RESET_WARM)
+#define _PCI_DEVICE_STATE_RESET_HOT 2
+#define PCI_DEVICE_STATE_RESET_HOT (1U<<_PCI_DEVICE_STATE_RESET_HOT)
+#define _PCI_DEVICE_STATE_RESET_FLR 3
+#define PCI_DEVICE_STATE_RESET_FLR (1U<<_PCI_DEVICE_STATE_RESET_FLR)
+ uint32_t reset_type;
+};
>
> Seeing as reset_type has no impact on the hypercall, I would like to
> ask for some reasoning for it's presence to be added to the commit
> message, otherwise it feels like pointless code churn.
OK, will add some commit messages to illustrate that this is for the
forward-looking implementation of different reset types of processing
situations in the future.
>
>> +};
>> +
>> #define PHYSDEVOP_DBGP_RESET_PREPARE 1
>> #define PHYSDEVOP_DBGP_RESET_DONE 2
>>
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index da8d0f41e6f4..6be812dbc04a 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -38,6 +38,8 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>>
>> /* Remove all handlers and free vpci related structures. */
>> void vpci_deassign_device(struct pci_dev *pdev);
>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev,
>> + uint32_t reset_type);
>>
>> /* Add/remove a register handler. */
>> int __must_check vpci_add_register_mask(struct vpci *vpci,
>> @@ -282,6 +284,12 @@ static inline int vpci_assign_device(struct pci_dev
>> *pdev)
>>
>> static inline void vpci_deassign_device(struct pci_dev *pdev) { }
>>
>> +static inline int __must_check vpci_reset_device_state(struct pci_dev *pdev,
>> + uint32_t reset_type)
>> +{
>> + return 0;
>> +}
>> +
>
> Maybe it turns out to be more complicated than the current approach,
> but vpci_reset_device_state() could be an static inline function in
> vpci.h defined regardless of whether CONFIG_HAS_VPCI is selected or
> not, as the underlying functions vpci_{de}assign_device() are always
> defined.
OK, will change to this in next version.
+static inline int __must_check vpci_reset_device_state(struct pci_dev *pdev)
+{
+ ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
+
+ vpci_deassign_device(pdev);
+ return vpci_assign_device(pdev);
+}
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
