Re: struct mctelem_cookie missing definition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2025 22:47, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Jan Beulich wrote:
>> On 13.02.2025 03:00, Stefano Stabellini wrote:
>>> On Wed, 12 Feb 2025, Stefano Stabellini wrote:
>>>> On Thu, 13 Feb 2025, Andrew Cooper wrote:
>>>>> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I am looking through the few remaining MISRA violations that we have
>>>>>> left.  One of them is R11.2:
>>>>>>
>>>>>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/PROJECT.ecd;/by_service/MC3A2.R11.2.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}}
>>>>>>
>>>>>> Specifically, mctelem_cookie_t is a pointer to incomplete type and
>>>>>> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
>>>>>> incomplete type and any other type".
>>>>>>
>>>>>> mctelem_cookie_t is defined as:
>>>>>>
>>>>>> typedef struct mctelem_cookie *mctelem_cookie_t;
>>>>>>
>>>>>> I am looking through the code and I genuinely cannot find the definition
>>>>>> of struct mctelem_cookie.
>>>>>>
>>>>>> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
>>>>>> sense to do:
>>>>>>
>>>>>> typedef struct mctelem_ent *mctelem_cookie_t;
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> What am I missing?
>>>>>
>>>>> Nothing.  Or perhaps the twisted thinking of the original author.
>>>>>
>>>>> It is genuinely a pointer type (== known size) which you can't deference
>>>>> (because there is no definition), and can only operate on by casting to
>>>>> an integer.  Except the code also requires it to be a uint64_t which is
>>>>> why there's some fun disabling of relevant hypercalls for compat guests.
>>>>>
>>>>> If someone could find the time to file it in /dev/null and replace it
>>>>> with literally anything else, I'd be very thankful.
>>>>
>>>> Are you OK with typedefing mctelem_cookie_t to uint64_t instead?
>>>
>>> I confirm that the following resolves the MISRA violations
>>>
>>> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h 
>>> b/xen/arch/x86/cpu/mcheck/mctelem.h
>>> index f4c5ff848d..2ccd490e5d 100644
>>> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
>>> @@ -52,7 +52,7 @@
>>>   * the element from the processing list.
>>>   */
>>>  
>>> -typedef struct mctelem_cookie *mctelem_cookie_t;
>>> +typedef uint64_t *mctelem_cookie_t;
>>
>> Yet that makes it possible to de-reference the pointer. Which, as Andrew
>> explained, is intended to be impossible. If this could be properly
>> replaced (not exactly what Andrew indicated by "file it in /dev/null"),
>> fine. Truly purging the code (i.e. as Andrew suggests) may still be an
>> option, with appropriate justification. But simply adjusting the type
>> and then moving on is too little, imo. Even if you used void * (to make
>> de-referencing impossible) I'd view it as largely papering over an issue;
>> then converting to other pointers (without explicit cast, and hence
>> without making apparent the badness of doing so) would become possible.
> 
> What about converting to uintptr_t (not a pointer)?

That'll lose type checking the compiler does. A type-safe wrapper struct
(like we have for mfn_t and alike in debug builds) may do.

Jan