Mailing List Archive: [PATCH v2 01/14] kernel-doc: public/arch-arm.h

[PATCH v2 01/14] kernel-doc: public/arch-arm.h

Oct 20, 2020, 4:59 PM

Post #1 of 3 (36 views)

Convert in-code comments to kernel-doc format wherever possible.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
xen/include/public/arch-arm.h | 43 ++++++++++++++++++++++-------------
1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c365b1b39e..409697dede 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -27,8 +27,8 @@
#ifndef __XEN_PUBLIC_ARCH_ARM_H__
#define __XEN_PUBLIC_ARCH_ARM_H__

-/*
- * `incontents 50 arm_abi Hypercall Calling Convention
+/**
+ * DOC: Hypercall Calling Convention
*
* A hypercall is issued using the ARM HVC instruction.
*
@@ -72,8 +72,8 @@
* Any cache allocation hints are acceptable.
*/

-/*
- * `incontents 55 arm_hcall Supported Hypercalls
+/**
+ * DOC: Supported Hypercalls
*
* Xen on ARM makes extensive use of hardware facilities and therefore
* only a subset of the potential hypercalls are required.
@@ -175,10 +175,17 @@
typedef union { type *p; uint64_aligned_t q; } \
__guest_handle_64_ ## name

-/*
+/**
+ * DOC: XEN_GUEST_HANDLE - a guest pointer in a struct
+ *
* XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
* in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes
* aligned.
+ */
+
+/**
+ * DOC: XEN_GUEST_HANDLE_PARAM - a guest pointer as a hypercall arg
+ *
* XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an
* hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64.
*/
@@ -201,7 +208,9 @@ typedef uint64_t xen_pfn_t;
#define PRI_xen_pfn PRIx64
#define PRIu_xen_pfn PRIu64

-/*
+/**
+ * DOC: XEN_LEGACY_MAX_VCPUS
+ *
* Maximum number of virtual CPUs in legacy multi-processor guests.
* Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
*/
@@ -299,26 +308,28 @@ struct vcpu_guest_context {
typedef struct vcpu_guest_context vcpu_guest_context_t;
DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);

-/*
+
+/**
+ * struct xen_arch_domainconfig - arch-specific domain creation params
+ *
* struct xen_arch_domainconfig's ABI is covered by
* XEN_DOMCTL_INTERFACE_VERSION.
*/
+struct xen_arch_domainconfig {
+ /** @gic_version: IN/OUT parameter */
#define XEN_DOMCTL_CONFIG_GIC_NATIVE 0
#define XEN_DOMCTL_CONFIG_GIC_V2 1
#define XEN_DOMCTL_CONFIG_GIC_V3 2
-
+ uint8_t gic_version;
+ /** @tee_type: IN parameter */
#define XEN_DOMCTL_CONFIG_TEE_NONE 0
#define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
-
-struct xen_arch_domainconfig {
- /* IN/OUT */
- uint8_t gic_version;
- /* IN */
uint16_t tee_type;
- /* IN */
+ /** @nr_spis: IN parameter */
uint32_t nr_spis;
- /*
- * OUT
+ /**
+ * @clock_frequency: OUT parameter
+ *
* Based on the property clock-frequency in the DT timer node.
* The property may be present when the bootloader/firmware doesn't
* set correctly CNTFRQ which hold the timer frequency.
--
2.17.1

Re: [PATCH v2 01/14] kernel-doc: public/arch-arm.h [ In reply to ]

andrew.cooper3 at citrix

Oct 21, 2020, 10:47 AM

Post #2 of 3 (36 views)

Permalink

On 21/10/2020 00:59, Stefano Stabellini wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c365b1b39e..409697dede 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -201,7 +208,9 @@ typedef uint64_t xen_pfn_t;
> #define PRI_xen_pfn PRIx64
> #define PRIu_xen_pfn PRIu64
>
> -/*
> +/**
> + * DOC: XEN_LEGACY_MAX_VCPUS
> + *
> * Maximum number of virtual CPUs in legacy multi-processor guests.
> * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
> */

I suppose I don't really want to know why this exists in the ARM ABI?
It looks to be a misfeature.

Shouldn't it be labelled as obsolete ? (Is that even a thing you can do
in kernel-doc? It surely must be...)

> @@ -299,26 +308,28 @@ struct vcpu_guest_context {
> typedef struct vcpu_guest_context vcpu_guest_context_t;
> DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>
> -/*
> +
> +/**
> + * struct xen_arch_domainconfig - arch-specific domain creation params
> + *
> * struct xen_arch_domainconfig's ABI is covered by
> * XEN_DOMCTL_INTERFACE_VERSION.
> */
> +struct xen_arch_domainconfig {
> + /** @gic_version: IN/OUT parameter */
> #define XEN_DOMCTL_CONFIG_GIC_NATIVE 0
> #define XEN_DOMCTL_CONFIG_GIC_V2 1
> #define XEN_DOMCTL_CONFIG_GIC_V3 2
> -
> + uint8_t gic_version;

Please can we have a newline in here, and elsewhere separating blocks of
logically connected field/constant/comments.

It will make a world of difference to the readability of the header
files themselves.

~Andrew

Re: [PATCH v2 01/14] kernel-doc: public/arch-arm.h [ In reply to ]

sstabellini at kernel

Oct 21, 2020, 3:17 PM

Post #3 of 3 (36 views)

Permalink

On Wed, 21 Oct 2020, Andrew Cooper wrote:
> On 21/10/2020 00:59, Stefano Stabellini wrote:
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index c365b1b39e..409697dede 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -201,7 +208,9 @@ typedef uint64_t xen_pfn_t;
> > #define PRI_xen_pfn PRIx64
> > #define PRIu_xen_pfn PRIu64
> >
> > -/*
> > +/**
> > + * DOC: XEN_LEGACY_MAX_VCPUS
> > + *
> > * Maximum number of virtual CPUs in legacy multi-processor guests.
> > * Only one. All other VCPUS must use VCPUOP_register_vcpu_info.
> > */
>
> I suppose I don't really want to know why this exists in the ARM ABI?
> It looks to be a misfeature.
>
> Shouldn't it be labelled as obsolete ? (Is that even a thing you can do
> in kernel-doc? It surely must be...)

I tried not to make any content changes as part of this series, but as
we are looking into this, I could append patches to the end of the
series to make some additional changes. I.e. I'd prefer to keep the
mechanical patches mechanical.

In regards to XEN_LEGACY_MAX_VCPUS, it is part of struct shared_info so
it must be defined. It makes sense to define it to the smallest number
given that the newer interface (VCPUOP_register_vcpu_info) is preferred.

In regards to labelling things as obsolete, I couldn't find a way to do
it with kernel-doc, but keep in mind that the end goal is to use
doxygen. It might become possible then.

> > @@ -299,26 +308,28 @@ struct vcpu_guest_context {
> > typedef struct vcpu_guest_context vcpu_guest_context_t;
> > DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >
> > -/*
> > +
> > +/**
> > + * struct xen_arch_domainconfig - arch-specific domain creation params
> > + *
> > * struct xen_arch_domainconfig's ABI is covered by
> > * XEN_DOMCTL_INTERFACE_VERSION.
> > */
> > +struct xen_arch_domainconfig {
> > + /** @gic_version: IN/OUT parameter */
> > #define XEN_DOMCTL_CONFIG_GIC_NATIVE 0
> > #define XEN_DOMCTL_CONFIG_GIC_V2 1
> > #define XEN_DOMCTL_CONFIG_GIC_V3 2
> > -
> > + uint8_t gic_version;
>
> Please can we have a newline in here, and elsewhere separating blocks of
> logically connected field/constant/comments.
>
> It will make a world of difference to the readability of the header
> files themselves.

Sure, I can do that