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

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

On 2024/8/19 17:04, Jan Beulich wrote:
> On 16.08.2024 13:08, Jiqian Chen wrote:
>> @@ -67,6 +68,57 @@ ret_t pci_physdev_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pci_device_reset:
>> +    {
>> +        struct pci_device_reset dev_reset;
>> +        struct pci_dev *pdev;
>> +        pci_sbdf_t sbdf;
>> +
>> +        ret = -EOPNOTSUPP;
>> +        if ( !is_pci_passthrough_enabled() )
>> +            break;
> 
> It occurs to me (only now, sorry): Does this case really need to be an
> error? I.e. do we really need to bother callers by having them find out
> whether pass-through is supported in the underlying Xen?
I am not sure, but for x86, passthrough is always true, it doesn't matter.
For arm, this hypercall is also used for passthrough devices for now, so it is 
better to keep the same behavior as other PHYSDEVOP_pci_device_* operation?

> 
>> +        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.flags & PCI_DEVICE_RESET_MASK )
>> +        {
>> +        case PCI_DEVICE_RESET_COLD:
>> +        case PCI_DEVICE_RESET_WARM:
>> +        case PCI_DEVICE_RESET_HOT:
>> +        case PCI_DEVICE_RESET_FLR:
>> +            ret = vpci_reset_device(pdev);
>> +            break;
>> +
>> +        default:
>> +            ret = -EOPNOTSUPP;
> 
> EINVAL
> 
> But: What about the other flag bits? You don't check them (anymore; I
> thought there was a check there before).
Before this version, here used value(like enum), so no check for other 
bits(should be zero).
I will add 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_reset 32
> 
> Nit: Please pad the 32 to align with the 30 and 31 in context.
Will change in next version.
> 
> Jan

-- 
Best regards,
Jiqian Chen.