On 02.08.2024 04:55, Chen, Jiqian wrote:
> 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;
> +};
That's four bits, not two. I'm pretty sure Roger meant to keep the enum-
like #define-s, but additionally define a 2-bit mask constant (0x3). I
don't think it needs to be three bits right away - we can decide what to
do there when any of the higher bits are to be assigned a meaning.
Jan
