From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F4204C433B4 for ; Thu, 29 Apr 2021 09:04:19 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8E47461449 for ; Thu, 29 Apr 2021 09:04:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8E47461449 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.119797.226479 (Exim 4.92) (envelope-from ) id 1lc2ad-00023T-TO; Thu, 29 Apr 2021 09:04:07 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 119797.226479; Thu, 29 Apr 2021 09:04:07 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lc2ad-00023M-QF; Thu, 29 Apr 2021 09:04:07 +0000 Received: by outflank-mailman (input) for mailman id 119797; Thu, 29 Apr 2021 09:04:06 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lc2ac-00023H-Pq for xen-devel@lists.xenproject.org; Thu, 29 Apr 2021 09:04:06 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 9e3d16e3-ab0e-4c77-8ee9-8fe481de6eae; Thu, 29 Apr 2021 09:04:04 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 89683AE8D; Thu, 29 Apr 2021 09:04:03 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 9e3d16e3-ab0e-4c77-8ee9-8fe481de6eae X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1619687043; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XMf+4zQQ0ncV4K8/XY2rrqIbwGFQSKNYsoW5/w5lIDw=; b=rk1glSRJd+y/NPPJacZsgIphyVxHgKuqRelCyQ5q0HYqYo7yUsZXxkmXk7YElwU0y1gybX vC403CyndNTnkDefLDcIzfxzwqeUGTobc9NY4NiDt/9BcwxnmAF7KqvYPlhVIJigNVIq3T CHnQpe+Rqa/9BI6m1SYtOKSdk9a282A= Subject: Re: [PATCH v3 3/3] docs/doxygen: doxygen documentation for grant_table.h To: Luca Fancellu Cc: Bertrand Marquis , wei.chen@arm.com, Andrew Cooper , George Dunlap , Ian Jackson , Julien Grall , Stefano Stabellini , Wei Liu , xen-devel@lists.xenproject.org References: <20210426153741.26904-1-luca.fancellu@arm.com> <20210426153741.26904-4-luca.fancellu@arm.com> From: Jan Beulich Message-ID: Date: Thu, 29 Apr 2021 11:04:01 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 28.04.2021 16:59, Luca Fancellu wrote: >> On 27 Apr 2021, at 14:57, Jan Beulich wrote: >> On 26.04.2021 17:37, Luca Fancellu wrote: >>> @@ -66,6 +67,7 @@ >>> * compiler barrier will still be required. >>> * >>> * Introducing a valid entry into the grant table: >>> + * @code >>> * 1. Write ent->domid. >>> * 2. Write ent->frame: >>> * GTF_permit_access: Frame to which access is permitted. >>> @@ -73,20 +75,25 @@ >>> * frame, or zero if none. >>> * 3. Write memory barrier (WMB). >>> * 4. Write ent->flags, inc. valid type. >>> + * @endcode >>> * >>> * Invalidating an unused GTF_permit_access entry: >>> + * @code >>> * 1. flags = ent->flags. >>> * 2. Observe that !(flags & (GTF_reading|GTF_writing)). >>> * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). >>> * NB. No need for WMB as reuse of entry is control-dependent on success of >>> * step 3, and all architectures guarantee ordering of ctrl-dep writes. >>> + * @endcode >>> * >>> * Invalidating an in-use GTF_permit_access entry: >>> + * >>> * This cannot be done directly. Request assistance from the domain controller >>> * which can set a timeout on the use of a grant entry and take necessary >>> * action. (NB. This is not yet implemented!). >>> * >>> * Invalidating an unused GTF_accept_transfer entry: >>> + * @code >>> * 1. flags = ent->flags. >>> * 2. Observe that !(flags & GTF_transfer_committed). [*] >>> * 3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0). >> >> Since neither in the cover letter nor in the description here I could >> spot a link to the resulting generated doc, I wonder what the >> inconsistencies above are about: You add @code/@endcode (and no blank >> lines) to parts of what's being described, and _instead_ a blank line >> to another part. I think the goal should be that not only the >> generated doc ends up looking "nice", but that the source code also >> remains consistent. Otherwise, someone like me coming across this >> later on might easily conclude that there was a @code/@endcode pair >> missed. > > Yes I’ll explain better in the commit message, that part and other parts are > enclosed by @code/@endcode because they are formatted using spaces. > If the block is not enclosed the spaces are missing in the html page resulting > In a mess. > If you can suggest an alias for the @code/@endcode command, I can create > it so that the user looking the source code can understand better what's going on. > An example: @keepformat/@endkeepformat OR @keepindent/@endkeepindent Oh, are you suggesting @code / @endcode isn't something doxygen mandates? In this case either of your alternative suggestions would look better to me. Which one depend on whether the goal to to merely keep indentation or whether formatting should be kept altogether. >>> @@ -97,18 +104,23 @@ >>> * transferred frame is written. It is safe for the guest to spin waiting >>> * for this to occur (detect by observing GTF_transfer_completed in >>> * ent->flags). >>> + * @endcode >>> * >>> * Invalidating a committed GTF_accept_transfer entry: >>> * 1. Wait for (ent->flags & GTF_transfer_completed). >>> * >> >> Why did this not also get enclosed by @code/@endcode? > > In this case there are no spaces that contributes to the indentation. But if, for consistency, @code / @endcode were added here, all would still be well? >>> * Changing a GTF_permit_access from writable to read-only: >>> + * >>> * Use SMP-safe CMPXCHG to set GTF_readonly, while checking !GTF_writing. >>> * >>> * Changing a GTF_permit_access from read-only to writable: >>> + * >>> * Use SMP-safe bit-setting instruction. >> >> And these two? > > These two lines makes the resulting html page looks better, the source code however > seems not too impacted by the change though. I was rather asking about the absence of @code / @endcode here. >>> + * @addtogroup grant_table Grant Tables >> >> And no blank (comment) line ahead of this? > > Here there is no need for a blank line in the comment, but if in your opinion the source > code will look better I can add it I think so, yes. >>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t; >>> * [GST]: This field is written by the guest and read by Xen. >>> */ >>> >>> -/* >>> - * Version 1 of the grant table entry structure is maintained purely >>> - * for backwards compatibility. New guests should use version 2. >>> - */ >>> #if __XEN_INTERFACE_VERSION__ < 0x0003020a >>> #define grant_entry_v1 grant_entry >>> #define grant_entry_v1_t grant_entry_t >>> #endif >>> +/** >>> + * Version 1 of the grant table entry structure is maintained purely >>> + * for backwards compatibility. New guests should use version 2. >>> + */ >> >> In case I didn't say so already before - I think this would be a good >> opportunity to drop the comment pointing at v2. With v2 optionally >> unavailable altogether, this can't possibly be a generally valid >> course of action. > > Could you explain me better that? Do you want to get rid of this comment? Especially the second sentence is misleading. If new code used v2, it would not work on Xen with v2 support turned off. > /** > * Version 1 of the grant table entry structure is maintained purely > * for backwards compatibility. New guests should use version 2. > */ > > In this case I don’t think this commit is the right place to do that, I can just > put it back where it was so that the documentation simply doesn’t show that. Keeping misleading information out of the docs is imo rather desirable. Of course we should, independently of that, also address the misleading information in the source code. I can accept that doing the adjustment right in this patch may not be ideal. I don't suppose I could talk you into adding a prereq patch dropping at least that 2nd sentence? Jan