Re: [PATCH v6 01/23] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page

On 28.07.2026 17:29, Oleksii Kurochko wrote:
> On 7/21/26 4:50 PM, Jan Beulich wrote:
>> On 20.07.2026 17:59, Oleksii Kurochko wrote:
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -102,9 +102,14 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>>   #ifdef CONFIG_MEM_PAGING
>>>       info->paged_pages       = atomic_read(&d->paged_pages);
>>>   #endif
>>> -    info->shared_info_frame =
>>> -        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
>>> -    BUG_ON(SHARED_M2P(info->shared_info_frame));
>>> +    if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
>>> +    {
>>> +        info->shared_info_frame =
>>> +            gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
>>> +        BUG_ON(SHARED_M2P(info->shared_info_frame));
>>> +    }
>>> +    else
>>> +        info->shared_info_frame = gfn_x(INVALID_GFN);
>>
>> There's one issue left here: INVALID_GFN is a Xen internal concept. It could
>> change value if we saw a need. Therefore you cannot use that value here, to
>> supply it as hypercall output. It needs to be ~0, and imo the public header
>> also wants amending to indicate the special meaning of this value.
> 
> I think I get your point. I will use ~0 here.
> 
> I don't see INVALID_GFN in public headers. If it exists then it isn't 
> clear why it shouldn't be used here.
> 
> Don't we want to add something like:
> 
> /* Domain has no shared_info page? */
> #define XEN_INVALID_SHARED_INFO_FRAME (~0ULL)
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
> 
> in public/domctl.h.

In principle we could consider doing such, but if we did it needs to be
technically accurate. It may not, as your code fragment does, assume
that unsigned long long and uint64_aligned_t (and for just the purpose
here also uint64_t) are the same thing. Yet at the same time I think we
have successfully avoided use of the fixed-width integer constants
macros in the public headers, first and foremost because plain C89
doesn't offer them.

Hence overall I think introducing such a constant is going too far;
merely stating verbally that the field being set to all-ones has this
particular meaning should be enough.

>> And there's one further aspect to consider: Do we really want to retain the
>> shared_info struct domain field when !HAS_SHARED_INFO? 
> 
> I think we could consider that.
> 
>> Making it conditional
>> would require some adjustment here, but might be tidier overall. In
>> particular doing so would eliminate the risk of new uses of the field
>> appearing, with people not noticing that they'd break RISC-V.
> 
> Would you be okay with the following change:
> 
> +#ifdef CONFIG_HAS_SHARED_INFO
> +    info->shared_info_frame =
> +        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
> +    BUG_ON(SHARED_M2P(info->shared_info_frame));
> +#else
> +    info->shared_info_frame = ~0;
> +#endif

It's not pretty, but acceptable imo.

Jan