Re: [XEN PATCH v10 1/5] xen/vpci: Clear all vpci status of device

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

On 2024/6/17 22:17, Jan Beulich wrote:
> On 17.06.2024 11:00, Jiqian Chen wrote:
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,11 +2,17 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  #include <xen/init.h>
>> +#include <xen/vpci.h>
>>  
>>  #ifndef COMPAT
>>  typedef long ret_t;
>>  #endif
>>  
>> +static const struct pci_device_state_reset_method
>> +                    pci_device_state_reset_methods[] = {
>> +    [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>> +};
> 
> What about the other three DEVICE_RESET_*? In particular ...
I don't know how to implement the other three types of reset.
This is a design form so that corresponding processing functions can be added 
later if necessary. Do I need to set them to NULL pointers in this array?
Does this form conform to your previous suggestion of using only one hypercall 
to handle all types of resets?

> 
>> @@ -67,6 +73,43 @@ 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 physdev_pci_device *dev;
>> +        struct pci_dev *pdev;
>> +        pci_sbdf_t sbdf;
>> +
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -EOPNOTSUPP;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
>> +            break;
>> +        dev = &dev_reset.dev;
>> +        sbdf = PCI_SBDF(dev->seg, dev->bus, 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();
>> +        ret = 
>> pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);
> 
> ... you're setting this up for calling NULL. In fact there's also no bounds
> check for the array index.
Oh, right. I will add checks next version.

> 
> Also, nit (further up): Opening figure braces for a new scope go onto their
OK, will change in next version.
> own line. Then again I notice that apparenly _all_ other instances in this
> file are doing it the wrong way, too.
Do I need to change them in this patch?
> 
> Finally, is the "dev" local variable really needed? It effectively hides that
> PCI_SBDF() is invoked on the hypercall arguments.
Will remove "dev" in next version.
> 
>> +        write_unlock(&pdev->domain->pci_lock);
>> +        if ( ret )
>> +            printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", 
>> &sbdf);
> 
> Maybe downgrade to dprintk()? The caller ought to handle the error anyway.
Will downgrade in next version.
> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>>  
>>      return rc;
>>  }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev)
> 
> As a target of an indirect call this needs to be annotated cf_check (both
> here and in the declaration, unlike __must_check, which is sufficient to
> have on just the declaration).
OK, will add cf_check in next version.
> 
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -156,6 +156,22 @@ struct pci_dev {
>>      struct vpci *vpci;
>>  };
>>  
>> +struct pci_device_state_reset_method {
>> +    int (*reset_fn)(struct pci_dev *pdev);
>> +};
>> +
>> +enum pci_device_state_reset_type {
>> +    DEVICE_RESET_FLR,
>> +    DEVICE_RESET_COLD,
>> +    DEVICE_RESET_WARM,
>> +    DEVICE_RESET_HOT,
>> +};
>> +
>> +struct pci_device_state_reset {
>> +    struct physdev_pci_device dev;
>> +    enum pci_device_state_reset_type reset_type;
>> +};
> 
> This is the struct to use as hypercall argument. How can it live outside of
> any public header? Also, when moving it there, beware that you should not
> use enum-s there. Only handles and fixed-width types are permitted.t
Yes, I put them there before, but enum is not permitted.
Then, do you have other suggested type to use to distinguish different types of 
resets, because enum can't work in the public header?

> 
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>>  
>>  /* Remove all handlers and free vpci related structures. */
>>  void vpci_deassign_device(struct pci_dev *pdev);
>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
> 
> What's the purpose of this __must_check, when the sole caller is calling
> this through a function pointer, which isn't similarly annotated?
This is what I added before introducing function pointers, but after modifying 
the implementation, it was not taken into account.
I will remove __must_check and change to cf_check, according to your above 
comment.

> 
> Jan

-- 
Best regards,
Jiqian Chen.