Re: struct mctelem_cookie missing definition

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan