[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v8 1/5] xen/vpci: Clear all vpci status of device
On 2024/5/17 18:31, Jan Beulich wrote: > On 17.05.2024 12:00, Chen, Jiqian wrote: >> On 2024/5/17 17:50, Jan Beulich wrote: >>> On 17.05.2024 11:28, Chen, Jiqian wrote: >>>> On 2024/5/17 16:20, Jan Beulich wrote: >>>>> On 17.05.2024 10:08, Chen, Jiqian wrote: >>>>>> On 2024/5/16 21:08, Jan Beulich wrote: >>>>>>> On 16.05.2024 11:52, Jiqian Chen wrote: >>>>>>>> struct physdev_pci_device { >>>>>>>> /* IN */ >>>>>>>> uint16_t seg; >>>>>>> >>>>>>> Is re-using this struct for this new sub-op sufficient? IOW are all >>>>>>> possible resets equal, and hence it doesn't need specifying what kind of >>>>>>> reset was done? For example, other than FLR most reset variants reset >>>>>>> all >>>>>>> functions in one go aiui. Imo that would better require only a single >>>>>>> hypercall, just to avoid possible confusion. It also reads as if FLR >>>>>>> would >>>>>>> not reset as many registers as other reset variants would. >>>>>> If I understood correctly that you mean in this hypercall it needs to >>>>>> support resetting both one function and all functions of a slot(dev)? >>>>>> But it can be done for caller to use a cycle to call this reset >>>>>> hypercall for each slot function. >>>>> >>>>> It could, yes, but since (aiui) there needs to be an indication of the >>>>> kind of reset anyway, we can as well avoid relying on the caller doing >>>>> so (and at the same time simplify what the caller needs to do). >>>> Since the corresponding kernel patch has been merged into linux_next branch >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20240515&id=b272722511d5e8ae580f01830687b8a6b2717f01, >>>> if it's not very mandatory and necessary, just let the caller handle it >>>> temporarily. >>> >>> As also mentioned for the other patch having a corresponding kernel one: >>> The kernel patch would imo better not be merged until the new sub-op is >>> actually finalized. >> OK, what should I do next step? >> Upstream a patch to revert the merged patch on kernel side? >> >>> >>>> Or it can add a new hypercall to reset all functions in one go in future >>>> potential requirement, like PHYSDEVOP_pci_device_state_reset_all_func. >>> >>> I disagree. We shouldn't introduce incomplete sub-ops. At the very least, >>> if you want to stick to the present form, I'd expect you to supply reasons >>> why distinguishing different reset forms is not necessary (now or later). >> OK, if want to distinguish different reset, is it acceptable to add a >> parameter, like "u8 flag", and reset every function if corresponding bit is >> 1? > > I'm afraid a boolean won't do, at least not long term. I think it wants to > be an enumeration (i.e. a set of enumeration-like #define-s). And just to > stress it again: The extra argument is _not_ primarily for the looping over > all functions. It is to convey the kind of reset that was done. The single > vs all function(s) aspect is just a useful side effect this will have. Do you mean, like: enum RESET_DEVICE_STATE { RESET_DEVICE_SINGLE_FUNC, RESET_DEVICE_ALL_FUNC, Others }; If caller pass in RESET_DEVICE_SINGLE_FUNC, I call what I add in this patch, as for other types call other functions if added in future? > > Jan -- Best regards, Jiqian Chen.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |