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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan