Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/09/2020 19:20, Julien Grall wrote:
>>> +
>>>   #endif /* __ASM_DOMAIN_H__ */
>>>     /*
>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>>> index 5c5e55ebcb76..7564df5e8374 100644
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -136,6 +136,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)
>>
>> We've already got INVALID_GFN as a constant used in the interface.  Lets
>> not proliferate more.
>
> This was my original approach (see [1]) but this was reworked because:
>    1) INVALID_GFN is not technically defined in the ABI. So the
> toolstack has to hardcode the value in the check.
>    2) The value is different between 32-bit and 64-bit Arm as
> INVALID_GFN is defined as an unsigned long.
>
> So providing a new define is the right way to go.

There is nothing special about this field.  It should not have a
dedicated constant, when a general one is the appropriate one to use.

libxl already has LIBXL_INVALID_GFN, which is already used.

If this isn't good enough, them the right thing to do is put a proper
INVALID_GFN in the tools interface.

>>>       uint64_aligned_t cpu_time;
>>>       uint32_t nr_online_vcpus;    /* Number of VCPUs currently
>>> online. */
>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>> index cde0d9c7fe63..7281eb7b36c7 100644
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>>>   static inline void vnuma_destroy(struct vnuma_info *vnuma) {
>>> ASSERT(!vnuma); }
>>>   #endif
>>>   +#ifdef CONFIG_HAS_M2P
>>> +#define domain_shared_info_gfn(d) ({                            \
>>> +    const struct domain *d_ = (d);                              \
>>> +    gfn_t gfn_;                                                 \
>>> +                                                                \
>>> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
>>> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
>>> +                                                                \
>>> +    gfn_;                                                       \
>>> +})
>>
>> ... this wants to be
>>
>> #ifndef arch_shared_info_gfn
>> static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
>> return INVALID_GFN; }
>> #endif
>>
>> with
>>
>> gfn_t arch_shared_info_gfn(const struct domain *d);
>> #define arch_shared_info_gfn arch_shared_info_gfn
>>
>> in asm-x86/domain.h
>>
>> and the actual implementation in arch/x86/domain.c
>
> What's wrong with implement it in xen/domain.h? After all there is
> nothing x86 specific in the implementation...

d->shared_info is allocated in arch specific code, not common code. 
This macro assumes that __virt_to_mfn() is safe to call on the pointer.

For an approaching obsolete part of the API/ABI (particularly given the
new HVM plans), I'd just stuff it in x86 and call it done.  Its easy
enough to re-evaluate if a second appears.

~Andrew