Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs

From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
To: Julien Grall <julien.grall@arm.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/7] xen/public: arch-arm: Restrict the visibility of struct vcpu_guest_core_regs
Date: Fri, 26 Jul 2019 13:17:14 +0000	[thread overview]
Message-ID: <87h879hr52.fsf@epam.com> (raw)
In-Reply-To: <b93a7a7e-970f-b550-bd0e-b31ac8e62975@arm.com>

Julien Grall writes:

> On 26/07/2019 13:14, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>
> Hi Volodymyr,
>
>> Julien Grall writes:
>>
>>> Currently, the structure vcpu_guest_core_regs is part of the public API.
>>> This implies that any change in the structure should be backward
>>> compatible.
>>>
>>> However, the structure is only needed by the tools and Xen. It is also
>>> not expected to be ever used outside of that context. So we could save us
>>> some headache by only declaring the structure for Xen and tools.
>>>
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>      This is a follow-up of the discussion [1].
>>>
>>>      [1] <3c245c5b-51c6-1d0e-ad6c-42414573166f@arm.com>
>>>
>>>      Changes in v3:
>>>          - Avoid introduce a new #ifdef in the header by moving the
>>>          definitions later on.
>>> ---
>>>   xen/include/public/arch-arm.h | 24 ++++++++++++------------
>>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index 3e8cdc151d..7ce139a0f5 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -197,6 +197,18 @@
>>>       } while ( 0 )
>>>   #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>>>   +typedef uint64_t xen_pfn_t;
>>> +#define PRI_xen_pfn PRIx64
>>> +#define PRIu_xen_pfn PRIu64
>>> +
>>> +/* Maximum number of virtual CPUs in legacy multi-processor guests. */
>>> +/* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
>> Just a suggestion: you already touching this part. Maybe you'll fix this
>> comment as well?
>
> I am not sure what's wrong with the current comment. Can you expand
> your thoughts please?
Sure. It does not conform to CODING_STYLE:

   Comments containing a single sentence may end with a full
   stop; comments containing several sentences must have a full stop
   after each sentence.

The second comment misses full stop at the end. Also, maybe we should
consider this as s multi-line comment:

   Multi-line comment blocks should start and end with comment markers on
   separate lines and each line should begin with a leading '*'.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel