Re: [PATCH v3 3/3] docs/doxygen: doxygen documentation for grant_table.h

[Luca Fancellu <luca.fancellu@arm.com>]

> On 29 Apr 2021, at 10:04, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.04.2021 16:59, Luca Fancellu wrote:
>>> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@suse.com> 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.

Hi Jan,

Sure, I can go with @keepindent/@endkeepindent

> 
>>>> @@ -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?

Yes it would be ok, in the html page you will see a box with this text inside.
If you see the url I sent in previous mail, you can see the rendering of the
@code/@endcode in the html page to have an idea on how it looks.

> 
>>>> * 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.

I didn’t use it because there is no space indentation to be kept, it looks better either in the
source code and in the html page in this way

> 
>>>> + * @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.

I will add it in the next patch

> 
>>>> @@ -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.

Can you suggest what to write here? I’m not very familiar with this xen
Interface

> 
>> /**
>> * 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?
> 

I will push a patch to change this comment and I will rebase this serie on
Top of that.

Cheers,
Luca

> Jan