Re: [XEN PATCH v11 1/8] xen/vpci: Clear all vpci status of device

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

On 2024/7/1 15:18, Jan Beulich wrote:
> On 30.06.2024 14:33, 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, add a new hypercall to clear all vpci
>> device state. When the state of device is reset on dom0 side,
>> dom0 can call this hypercall to notify vpci.
> 
> While the description properly talks about all of this being about device
> reset, the title suggests otherwise (leaving open what the context is, thus
> - to me at least - suggesting it's during vPCI init for a particular
> device).
Change title to "xen/pci: Add hypercall to support reset of pcidev" ?

> 
>> @@ -67,6 +68,63 @@ 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();
>> +        /* Implement FLR, other reset types may be implemented in future */
> 
> The comment isn't in sync with the code anymore.
Change to "/* vpci_reset_device_state is called by default for all reset types, 
other specific operations can be added later as needed */" ?

> 
>> +        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:
>> +        {
> 
> This brace isn't needed while at the same time it is confusing.
> 
>> +            ret = vpci_reset_device_state(pdev, dev_reset.reset_type);
>> +            if ( ret )
>> +                dprintk(XENLOG_ERR,
>> +                        "%pp: failed to reset vPCI device state\n", &sbdf);
> 
> I question the need for a log message here.
OK, will delete it in next version.

> 
>> --- 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,19 @@ 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_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;
>> +};
> 
> Do we really need the _PCI_DEVICE_STATE_RESET_* bit positions as separate
> #define-s? I can't spot any use anywhere.
I thought it was a coding style.
I will delete them in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.