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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2024 08:25, Chen, Jiqian wrote:
> 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?

No.

> Does this form conform to your previous suggestion of using only one 
> hypercall to handle all types of resets?

Yes, at least in principle. Question here is: To be on the safe side,
wouldn't we better reset state for all forms of reset, leaving possible
relaxation of that for later? At which point the function wouldn't need
calling indirectly, and instead would be passed the reset type as an
argument.

>> 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?

No.

>>> --- 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.

Which may not be necessary if you go the route suggested above.

>>> --- 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?

Do like we do everywhere else: Use a set of #define-s.

>>> --- 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

Why remove? Is it relevant for the return value to be checked? Or if it
isn't, why would there be a return value?

Jan

> and change to cf_check, according to your above comment.