> On 29 Apr 2021, at 10:04, Jan Beulich <[email protected]> wrote:
>
> On 28.04.2021 16:59, Luca Fancellu wrote:
>>> On 27 Apr 2021, at 14:57, Jan Beulich <[email protected]> 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
