Re: [PATCH v5 2/4] xen: arm: Stop returning a bogus GFN for the shared info

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2021 19:11, Julien Grall wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
> 
> While Arm never had a M2P, the implementation of mfn_to_gfn() is pretty
> bogus as we directly return the MFN passed in parameter.
> 
> The last use of mfn_to_gfn() on Arm is in getdomaininfo(). It looks
> like this is mostly used for mapping the P2M Of PV guest. Furthermore,
> the structure on Arm doesn't seem to contain a lot of useful
> information. Therefore it is unclear whether we want to allow the
> toolstack to map it.
> 
> As there is a high change that RISC-V will not implement an M2P,
> provide a new wrapper that will by default return an invalid GFN and
> move the code to find the GFN in an x86 specific helper.
> 
> If in the future we want to map the shared info, then we should
> consider to do it using the acquire hypercall.
> 
> Note that as INVALID_GFN is unsigned long, we can't return directly
> because the value would differ between 64-bit and 32-bit. Instead,
> a fixed value needs to be introduced.
> 
> While the fixed value could be shared with other field storing a
> GFN, we unfortunately use a mix of type (unsigned long, uint64_t)
> for exposing it externally. So to avoid any misuse, it is better to
> define a fixed value for just the shared_info_gfn field.
> 
> Signed-off-by Julien Grall <julien.grall@xxxxxxx>
> 
> ---
>     I am not comfortable with introduce a generic invalid GFN fixed
>     value because we don't use a common type. I also didn't get any
>     feedback on whether it would be acceptable to focus on one type.

It's been quite a while since the prior discussion, so "I am not
comfortable" may refer to something you do here but that you'd
prefer not to do, or something that you elected not to do because
you dislike it. Unfortunately "because we don't use a common type"
is insufficient context to tell, and hence I can't very well
support your view or object to it.

>     So the fixed value has not been changed. I think this is acceptable
>     because this a DOMCTL and therefore not stable. If someone still
>     disagree, then please provide concrete steps how to solve
>     the mixing of type.

Mixing of which types? You have explicit translation between
internal and external representation in getdomaininfo() - this
looks okay to me, fwiw.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2553,6 +2553,15 @@ void domain_pause_for_debugger(void)
>  #endif
>  }
>  
> +gfn_t arch_shared_info_gfn(const struct domain *d)
> +{
> +    gfn_t gfn = mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));

In order to be able to easily and immediately spot these once
virt_to_mfn() finally becomes a global type-safe wrapper around
__virt_to_mfn(), please retain the prior use of the not-
underscore-prefixed variant.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -64,6 +64,9 @@ config MEM_ACCESS
>         Framework to configure memory access types for guests and receive
>         related events in userspace.
>  
> +config HAS_M2P
> +     bool
> +
>  config NEEDS_LIBELF
>       bool
>  

Stale change?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -141,6 +141,12 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a mappable address in the field. In that case, the field
> +     * will be set to XEN_INVALID_SHARED_INFO_FRAME.
> +     */
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */

Since you repeat it anyway in the new comment, may I ask that you drop
the old one, thus getting rid of one more instance of "GMFN"?

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -133,4 +133,11 @@ static inline void vnuma_destroy(struct vnuma_info 
> *vnuma) { ASSERT(!vnuma); }
>  
>  extern bool vmtrace_available;
>  
> +#ifndef arch_shared_info_gfn
> +static inline gfn_t arch_shared_info_gfn(const struct domain *d)
> +{
> +    return INVALID_GFN;
> +}
> +#endif

This doesn't need to live in a header, does it? I don't think there's
any remote expectation that a 2nd caller may surface.

Jan