Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/8/2 14:25, Jan Beulich wrote:
> 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.Chen@xxxxxxx>
>>>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>>> Reviewed-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>
>>> 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.
Like this?
struct pci_device_state_reset {
    physdev_pci_device_t dev;
#define PCI_DEVICE_STATE_RESET_COLD 0x0
#define PCI_DEVICE_STATE_RESET_WARM 0x1
#define PCI_DEVICE_STATE_RESET_HOT  0x2
#define PCI_DEVICE_STATE_RESET_FLR  0x3
#define PCI_DEVICE_STATE_RESET_MASK  0x3
    uint32_t flags;
};

> 
> Jan

-- 
Best regards,
Jiqian Chen.