[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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Date: Fri, 17 May 2024 11:01:46 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=lXUspc/KmdqKgmU+4BNvCueklnSAmJrXNnaG2f1FFAA=; b=Gc1l/YMxc13wpZHl/835GVnnoVt80GN+wHha0F8+nkd6ty8u3ClbQ3nxkNyepPpugk9vfc9FqMzZTlwS6Rh82aM77DtWD+iOaBOst/9uLDtX1felTD+cznnI4iR7pJPjPKipwvj0cB+kUu+kM8fcr2nnCuQQIALe2zmI8Ao9P1Cn3For8klUdFuKZFb2/kw5aGouQ6JtBJHOcUtUJy8ev0Om7dPwGXEXSldI79wEiuJwFFRCsSvHxBPCZItwKp7i88C+KpDKtFymhd4RWhiPS21Z9+dduJ0UFlTUWZ0fqOX9fc38RufTZpnPiVlJK+xtn2tpTag0+ag6phjFP/Zsig==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=WiC8dxKe+RhjJZSdM7ctyKM+TMP5RbT2UF3kXDBfeFT8E1Sp2f1CfEfPg0SWig9rset6gA9Ae0U4ymGpn6C++McjiBBHKUvwrH2WvBtvndf9CUgPSW3D1kSoAZ5m8kJEVjDwmSxM+NoyG+1g87Dk+9vZ1BMfBp3ggIZV9krA8v1eJV4me/UAgh+0Wr5cwZTK5sBRksSUMrYvZPjHtVCGg6Kvyv8MSqbRzzdIq4N9C8ULsjPCt82fj0mvC5MHVbVJzw/KH7Cmr2fDgf3eMsnKBVz6EumCEaDHKOkyqPA3h2BBRjbjVWrc44z7A8mF0zksA3diHrHOz11Fav9Upoz+tA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Anthony PERARD <anthony@xxxxxxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, "Hildebrand, Stewart" <Stewart.Hildebrand@xxxxxxx>, "Huang, Ray" <Ray.Huang@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Delivery-date: Fri, 17 May 2024 11:02:04 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHap3bYcGtlWtrcpEyRLTK5fjq5SLGZ1UMAgAFwSAD//9GPgIAAlWQA//+DzoCAAIa3gP//hO2AABFvioA=
  • Thread-topic: [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.

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.