diff mbox series

[Xen-devel,09/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

Message ID 20190507151458.29350-10-julien.grall@arm.com
State New
Headers show
Series xen/arm: Properly disable M2P on Arm. | expand

Commit Message

Julien Grall May 7, 2019, 3:14 p.m. UTC
While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
bogus as we directly return the MFN passed in parameter.

Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
are only 3 callers:
    - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
    page-tables when the P2M is not shared with the IOMMU. No issues so
    far as Arm does not yet support non-shared P2M case.
    - memory_exchange: Arm cannot not use it because steal_page is not
    implemented.
    - getdomaininfo: Toolstack may map the shared page. It looks like
    this is mostly used for mapping the P2M of PV guest. Therefore the
    issue might be minor.

Implementing the M2P on Arm is not planned. The M2P would require significant
amount of VA address (very tough on 32-bit) that can hardly be justified with
the current use of mfn_to_gmfn.
    - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
    IOMMU page-tables is delayed until the first device is assigned.
    In the embedded case, we will known in most of the times what
    devices are assigned during the domain creation. So it is possible
    to take to enable the IOMMU from start. See [1] for the patch.
    - memory_exchange: This does not work and I haven't seen any
    request for it so far.
    - getdomaininfo: The structure on Arm does not seem to contain a lot
    of useful information on Arm. It is unclear whether we want to
    allow the toolstack mapping it on Arm.

This patch introduces a config option HAS_M2P to tell whether an
architecture implements the M2P.
    - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
    is not sharing the P2M.
    - memory_exchange: The hypercall is marked as not supported when the
    M2P does not exist.
    - getdomaininfo: A new helper is introduced to wrap the call to
    mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
    will fail.

[1] https://patchwork.kernel.org/patch/9719913/

Signed-off-by Julien Grall <julien.grall@arm.com>

---

Cc: oleksandr_tyshchenko@epam.com
Cc: andrii_anisov@epam.com

    Changes in v2:
        - Add a warning in public headers
        - Constify local variable in domain_shared_info_gfn
        - Invert the naming (_d / d) in domain_shared_info_gfn
        - Use -EOPNOTSUPP rather than -ENOSYS
        - Rework how the memory_exchange hypercall is removed from Arm
---
 xen/arch/x86/Kconfig            | 1 +
 xen/common/Kconfig              | 3 +++
 xen/common/domctl.c             | 2 +-
 xen/common/memory.c             | 4 ++++
 xen/drivers/passthrough/iommu.c | 6 +++++-
 xen/include/asm-arm/domain.h    | 5 +++++
 xen/include/public/domctl.h     | 4 ++++
 xen/include/xen/domain.h        | 8 ++++++++
 8 files changed, 31 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini May 9, 2019, 6:06 p.m. UTC | #1
On Tue, 7 May 2019, Julien Grall wrote:
> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> bogus as we directly return the MFN passed in parameter.
> 
> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> are only 3 callers:
>     - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
>     page-tables when the P2M is not shared with the IOMMU. No issues so
>     far as Arm does not yet support non-shared P2M case.
>     - memory_exchange: Arm cannot not use it because steal_page is not
>     implemented.
>     - getdomaininfo: Toolstack may map the shared page. It looks like
>     this is mostly used for mapping the P2M of PV guest. Therefore the
>     issue might be minor.
> 
> Implementing the M2P on Arm is not planned. The M2P would require significant
> amount of VA address (very tough on 32-bit) that can hardly be justified with
> the current use of mfn_to_gmfn.
>     - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
>     IOMMU page-tables is delayed until the first device is assigned.
>     In the embedded case, we will known in most of the times what
>     devices are assigned during the domain creation. So it is possible
>     to take to enable the IOMMU from start. See [1] for the patch.
>     - memory_exchange: This does not work and I haven't seen any
>     request for it so far.
>     - getdomaininfo: The structure on Arm does not seem to contain a lot
>     of useful information on Arm. It is unclear whether we want to
>     allow the toolstack mapping it on Arm.
> 
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
>     - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
>     is not sharing the P2M.
>     - memory_exchange: The hypercall is marked as not supported when the
>     M2P does not exist.
>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
>     will fail.
> 
> [1] https://patchwork.kernel.org/patch/9719913/
> 
> Signed-off-by Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> Cc: oleksandr_tyshchenko@epam.com
> Cc: andrii_anisov@epam.com
> 
>     Changes in v2:
>         - Add a warning in public headers
>         - Constify local variable in domain_shared_info_gfn
>         - Invert the naming (_d / d) in domain_shared_info_gfn
>         - Use -EOPNOTSUPP rather than -ENOSYS
>         - Rework how the memory_exchange hypercall is removed from Arm
> ---
>  xen/arch/x86/Kconfig            | 1 +
>  xen/common/Kconfig              | 3 +++
>  xen/common/domctl.c             | 2 +-
>  xen/common/memory.c             | 4 ++++
>  xen/drivers/passthrough/iommu.c | 6 +++++-
>  xen/include/asm-arm/domain.h    | 5 +++++
>  xen/include/public/domctl.h     | 4 ++++
>  xen/include/xen/domain.h        | 8 ++++++++
>  8 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 4b8b07b549..52922a87e7 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -16,6 +16,7 @@ config X86
>  	select HAS_IOPORTS
>  	select HAS_KEXEC
>  	select MEM_ACCESS_ALWAYS_ON
> +	select HAS_M2P
>  	select HAS_MEM_PAGING
>  	select HAS_MEM_SHARING
>  	select HAS_NS16550
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index c838506241..df871adc8f 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -63,6 +63,9 @@ config HAS_GDBSX
>  config HAS_IOPORTS
>  	bool
>  
> +config HAS_M2P
> +	bool
> +
>  config NEEDS_LIBELF
>  	bool
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index bade9a63b1..29940fdea5 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>      BUG_ON(SHARED_M2P(info->shared_info_frame));
>  
>      info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 86567e6117..d6a580da31 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -512,6 +512,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  {
> +#ifdef CONFIG_M2P
>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -806,6 +807,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> +#else /* !CONFIG_M2P */
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index a6697d58fb..dbb64b13bc 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>      if ( need_iommu_pt_sync(d) )
>      {
> +        int rc = 0;
> +#ifdef CONFIG_HAS_M2P
>          struct page_info *page;
>          unsigned int i = 0, flush_flags = 0;
> -        int rc = 0;
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          /* Use while-break to avoid compiler warning */
>          while ( iommu_iotlb_flush_all(d, flush_flags) )
>              break;
> +#else
> +        rc = -EOPNOTSUPP;
> +#endif
>  
>          if ( rc )
>              printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 312fec8932..d61b0188da 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
>  
>  static inline void arch_vcpu_block(struct vcpu *v) {}
>  
> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> +{
> +    return INVALID_GFN;
> +}
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 19486d5e32..cac8ffffe9 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a valid GFN.
> +     */
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
>      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 d1bfc82f57..f1761fe183 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -118,4 +118,12 @@ struct vnuma_info {
>  
>  void vnuma_destroy(struct vnuma_info *vnuma);
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d) ({                            \
> +    const struct domain *d_ = (d);                              \
> +                                                                \
> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \

Aren't you missing a _gfn here?

  _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));


> +})
> +#endif
> +
>  #endif /* __XEN_DOMAIN_H__ */
Julien Grall May 9, 2019, 6:12 p.m. UTC | #2
Hi,

On 5/9/19 7:06 PM, Stefano Stabellini wrote:
> On Tue, 7 May 2019, Julien Grall wrote:
>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>> index d1bfc82f57..f1761fe183 100644
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -118,4 +118,12 @@ struct vnuma_info {
>>   
>>   void vnuma_destroy(struct vnuma_info *vnuma);
>>   
>> +#ifdef CONFIG_HAS_M2P
>> +#define domain_shared_info_gfn(d) ({                            \
>> +    const struct domain *d_ = (d);                              \
>> +                                                                \
>> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
> 
> Aren't you missing a _gfn here?
> 
>    _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));

Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So 
the function now return a gfn_t.

Cheers,
Stefano Stabellini May 9, 2019, 6:16 p.m. UTC | #3
On Thu, 9 May 2019, Julien Grall wrote:
> Hi,
> 
> On 5/9/19 7:06 PM, Stefano Stabellini wrote:
> > On Tue, 7 May 2019, Julien Grall wrote:
> > > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> > > index d1bfc82f57..f1761fe183 100644
> > > --- a/xen/include/xen/domain.h
> > > +++ b/xen/include/xen/domain.h
> > > @@ -118,4 +118,12 @@ struct vnuma_info {
> > >     void vnuma_destroy(struct vnuma_info *vnuma);
> > >   +#ifdef CONFIG_HAS_M2P
> > > +#define domain_shared_info_gfn(d) ({                            \
> > > +    const struct domain *d_ = (d);                              \
> > > +                                                                \
> > > +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
> > 
> > Aren't you missing a _gfn here?
> > 
> >    _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));
> 
> Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So the
> function now return a gfn_t.

Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them
from the archive.
Julien Grall May 9, 2019, 6:29 p.m. UTC | #4
Hi Stefano,

On 5/9/19 7:16 PM, Stefano Stabellini wrote:
> On Thu, 9 May 2019, Julien Grall wrote:
>> Hi,
>>
>> On 5/9/19 7:06 PM, Stefano Stabellini wrote:
>>> On Tue, 7 May 2019, Julien Grall wrote:
>>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>>> index d1bfc82f57..f1761fe183 100644
>>>> --- a/xen/include/xen/domain.h
>>>> +++ b/xen/include/xen/domain.h
>>>> @@ -118,4 +118,12 @@ struct vnuma_info {
>>>>      void vnuma_destroy(struct vnuma_info *vnuma);
>>>>    +#ifdef CONFIG_HAS_M2P
>>>> +#define domain_shared_info_gfn(d) ({                            \
>>>> +    const struct domain *d_ = (d);                              \
>>>> +                                                                \
>>>> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
>>>
>>> Aren't you missing a _gfn here?
>>>
>>>     _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));
>>
>> Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So the
>> function now return a gfn_t.
> 
> Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them
> from the archive.

Because they are x86 specific :). The rationale of implementing 
domain_shared_info_gfn() in common code is any arch using M2P should 
provide a similar helper.

Arm doesn't have an M2P, hence why mfn_to_gfn is not existent.

Cheers,
Stefano Stabellini May 9, 2019, 7:49 p.m. UTC | #5
On Thu, 9 May 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 5/9/19 7:16 PM, Stefano Stabellini wrote:
> > On Thu, 9 May 2019, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 5/9/19 7:06 PM, Stefano Stabellini wrote:
> > > > On Tue, 7 May 2019, Julien Grall wrote:
> > > > > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> > > > > index d1bfc82f57..f1761fe183 100644
> > > > > --- a/xen/include/xen/domain.h
> > > > > +++ b/xen/include/xen/domain.h
> > > > > @@ -118,4 +118,12 @@ struct vnuma_info {
> > > > >      void vnuma_destroy(struct vnuma_info *vnuma);
> > > > >    +#ifdef CONFIG_HAS_M2P
> > > > > +#define domain_shared_info_gfn(d) ({                            \
> > > > > +    const struct domain *d_ = (d);                              \
> > > > > +                                                                \
> > > > > +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
> > > > 
> > > > Aren't you missing a _gfn here?
> > > > 
> > > >     _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));
> > > 
> > > Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So
> > > the
> > > function now return a gfn_t.
> > 
> > Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them
> > from the archive.
> 
> Because they are x86 specific :). The rationale of implementing
> domain_shared_info_gfn() in common code is any arch using M2P should provide a
> similar helper.
> 
> Arm doesn't have an M2P, hence why mfn_to_gfn is not existent.

All right. Add my acked-by to this patch.
Jan Beulich May 10, 2019, 12:31 p.m. UTC | #6
>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
>     - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
>     is not sharing the P2M.
>     - memory_exchange: The hypercall is marked as not supported when the
>     M2P does not exist.

Was it you or someone else to suggest it be restricted to non-translated
guests in the first place? I'd prefer this over the #ifdef-ary you add.

>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
>     will fail.

There's no use of mfn_to_gmfn() in either of the wrappers, so why
mention this to-be-removed one?

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));

What is the intended behavior on 32-bit Arm here? Do you really
mean to return a value with 32 bits of ones (instead of 64 bits of
them) in this 64-bit field?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>      if ( need_iommu_pt_sync(d) )
>      {
> +        int rc = 0;
> +#ifdef CONFIG_HAS_M2P
>          struct page_info *page;
>          unsigned int i = 0, flush_flags = 0;
> -        int rc = 0;
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          /* Use while-break to avoid compiler warning */
>          while ( iommu_iotlb_flush_all(d, flush_flags) )
>              break;
> +#else
> +        rc = -EOPNOTSUPP;
> +#endif
>  
>          if ( rc )
>              printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",

Would you mind extending the scope of the #ifdef beyond this printk()?
It seems pretty pointless to me to unconditionally emit a log message
here.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a valid GFN.
> +     */

Along the lines of what I've said above, I think you want to spell out
here what the value is going to be in this case.

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -118,4 +118,12 @@ struct vnuma_info {
>  
>  void vnuma_destroy(struct vnuma_info *vnuma);
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d) ({                            \
> +    const struct domain *d_ = (d);                              \
> +                                                                \
> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
> +})
> +#endif

And an inline function doesn't work here?

Jan
Julien Grall May 10, 2019, 1:22 p.m. UTC | #7
Hi,

On 10/05/2019 13:31, Jan Beulich wrote:
>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>> This patch introduces a config option HAS_M2P to tell whether an
>> architecture implements the M2P.
>>      - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
>>      is not sharing the P2M.
>>      - memory_exchange: The hypercall is marked as not supported when the
>>      M2P does not exist.
> 
> Was it you or someone else to suggest it be restricted to non-translated
> guests in the first place? I'd prefer this over the #ifdef-ary you add.

I never suggested that as I have no idea who is using it on x86.

But then, we would still need to implement mfn_to_gfn on Arm to make it compile. 
I really want to avoid such macro on Arm.

> 
>>      - getdomaininfo: A new helper is introduced to wrap the call to
>>      mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
>>      will fail.
> 
> There's no use of mfn_to_gmfn() in either of the wrappers, so why
> mention this to-be-removed one?

Because code has been changed over the revision and I forgot to update the 
commit message.

> 
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>> xen_domctl_getdomaininfo *info)
>>       info->outstanding_pages = d->outstanding_pages;
>>       info->shr_pages         = atomic_read(&d->shr_pages);
>>       info->paged_pages       = atomic_read(&d->paged_pages);
>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
> 
> What is the intended behavior on 32-bit Arm here? Do you really
> mean to return a value with 32 bits of ones (instead of 64 bits of
> them) in this 64-bit field?
It does not matter as long as the GFN is invalid so it can't be mapped 
afterwards. The exact value is not documented in the header to avoid any assumption.

> 
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>       hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>       if ( need_iommu_pt_sync(d) )
>>       {
>> +        int rc = 0;
>> +#ifdef CONFIG_HAS_M2P
>>           struct page_info *page;
>>           unsigned int i = 0, flush_flags = 0;
>> -        int rc = 0;
>>   
>>           page_list_for_each ( page, &d->page_list )
>>           {
>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>           /* Use while-break to avoid compiler warning */
>>           while ( iommu_iotlb_flush_all(d, flush_flags) )
>>               break;
>> +#else
>> +        rc = -EOPNOTSUPP;
>> +#endif
>>   
>>           if ( rc )
>>               printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> 
> Would you mind extending the scope of the #ifdef beyond this printk()?
> It seems pretty pointless to me to unconditionally emit a log message
> here.

Well, it at least tell you the function can't work. So I think it is still makes 
sense to have it.

> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo {
>>       uint64_aligned_t outstanding_pages;
>>       uint64_aligned_t shr_pages;
>>       uint64_aligned_t paged_pages;
>> +    /*
>> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
>> +     * provide a valid GFN.
>> +     */
> 
> Along the lines of what I've said above, I think you want to spell out
> here what the value is going to be in this case.

Spelling out the exact value gives us less freedom. What matters here is the GFN 
return can't be mapped.

> 
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -118,4 +118,12 @@ struct vnuma_info {
>>   
>>   void vnuma_destroy(struct vnuma_info *vnuma);
>>   
>> +#ifdef CONFIG_HAS_M2P
>> +#define domain_shared_info_gfn(d) ({                            \
>> +    const struct domain *d_ = (d);                              \
>> +                                                                \
>> +    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
>> +})
>> +#endif
> 
> And an inline function doesn't work here?

With enough time to rework the headers maybe. At the moment, no because 
mfn_to_gfn is implemented in p2m.h which depends on domain.h for the definition 
of struct domain d:

In file included from /home/julieng/works/xen/xen/include/xen/sched.h:11:0,
                  from x86_64/asm-offsets.c:9:
/home/julieng/works/xen/xen/include/xen/domain.h: In function 
‘domain_shared_info_gfn’:
/home/julieng/works/xen/xen/include/xen/domain.h:124:12: error: implicit 
declaration of function ‘mfn_to_gfn’ [-Werror=implicit-function-declaration]
      return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
             ^~~~~~~~~~
/home/julieng/works/xen/xen/include/xen/domain.h:124:5: error: nested extern 
declaration of ‘mfn_to_gfn’ [-Werror=nested-externs]
      return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
      ^~~~~~
In file included from /home/julieng/works/xen/xen/include/asm/current.h:12:0,
                  from /home/julieng/works/xen/xen/include/asm/smp.h:10,
                  from /home/julieng/works/xen/xen/include/xen/smp.h:4,
                  from /home/julieng/works/xen/xen/include/asm/processor.h:10,
                  from /home/julieng/works/xen/xen/include/asm/system.h:6,
                  from /home/julieng/works/xen/xen/include/xen/spinlock.h:4,
                  from /home/julieng/works/xen/xen/include/xen/sched.h:6,
                  from x86_64/asm-offsets.c:9:
/home/julieng/works/xen/xen/include/xen/domain.h:124:46: error: dereferencing 
pointer to incomplete type ‘struct domain’
      return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
                                               ^
/home/julieng/works/xen/xen/include/asm/page.h:265:61: note: in definition of 
macro ‘virt_to_maddr’
  #define virt_to_maddr(va)   __virt_to_maddr((unsigned long)(va))
                                                              ^~
/home/julieng/works/xen/xen/include/xen/domain.h:124:31: note: in expansion of 
macro ‘__virt_to_mfn’
      return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
                                ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:217: recipe for target 'asm-offsets.s' failed
make[2]: *** [asm-offsets.s] Error 1
make[2]: Leaving directory '/home/julieng/works/xen/xen/arch/x86'
Makefile:136: recipe for target '/home/julieng/works/xen/xen/xen' failed
make[1]: *** [/home/julieng/works/xen/xen/xen] Error 2
make[1]: Leaving directory '/home/julieng/works/xen/xen'
Makefile:45: recipe for target 'build' failed
make: *** [build] Error 2

Cheers,
Jan Beulich May 10, 2019, 1:32 p.m. UTC | #8
>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote:
> On 10/05/2019 13:31, Jan Beulich wrote:
>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>>> xen_domctl_getdomaininfo *info)
>>>       info->outstanding_pages = d->outstanding_pages;
>>>       info->shr_pages         = atomic_read(&d->shr_pages);
>>>       info->paged_pages       = atomic_read(&d->paged_pages);
>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>> 
>> What is the intended behavior on 32-bit Arm here? Do you really
>> mean to return a value with 32 bits of ones (instead of 64 bits of
>> them) in this 64-bit field?
> It does not matter as long as the GFN is invalid so it can't be mapped 
> afterwards. The exact value is not documented in the header to avoid any 
> assumption.

That's not helpful - how would a consumer know to avoid the mapping
attempt in the first place?

>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>       hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>>       if ( need_iommu_pt_sync(d) )
>>>       {
>>> +        int rc = 0;
>>> +#ifdef CONFIG_HAS_M2P
>>>           struct page_info *page;
>>>           unsigned int i = 0, flush_flags = 0;
>>> -        int rc = 0;
>>>   
>>>           page_list_for_each ( page, &d->page_list )
>>>           {
>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>           /* Use while-break to avoid compiler warning */
>>>           while ( iommu_iotlb_flush_all(d, flush_flags) )
>>>               break;
>>> +#else
>>> +        rc = -EOPNOTSUPP;
>>> +#endif
>>>   
>>>           if ( rc )
>>>               printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>> 
>> Would you mind extending the scope of the #ifdef beyond this printk()?
>> It seems pretty pointless to me to unconditionally emit a log message
>> here.
> 
> Well, it at least tell you the function can't work. So I think it is still makes 
> sense to have it.

I disagree.

Jan
Julien Grall May 10, 2019, 1:41 p.m. UTC | #9
Hi Jan,

On 10/05/2019 14:32, Jan Beulich wrote:
>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote:
>> On 10/05/2019 13:31, Jan Beulich wrote:
>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>>>> xen_domctl_getdomaininfo *info)
>>>>        info->outstanding_pages = d->outstanding_pages;
>>>>        info->shr_pages         = atomic_read(&d->shr_pages);
>>>>        info->paged_pages       = atomic_read(&d->paged_pages);
>>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>>>
>>> What is the intended behavior on 32-bit Arm here? Do you really
>>> mean to return a value with 32 bits of ones (instead of 64 bits of
>>> them) in this 64-bit field?
>> It does not matter as long as the GFN is invalid so it can't be mapped
>> afterwards. The exact value is not documented in the header to avoid any
>> assumption.
> 
> That's not helpful - how would a consumer know to avoid the mapping
> attempt in the first place?

I can't see any issue with the consumer to try to map it. Ok, you will waste a 
couple of cycles, but that should be pretty rare.

The point here, we keep within the hypervisor the idea of what's valid or 
invalid. This allows us more flexibility on the value here (imagine we decide to 
change the value of GFN_INVALID in the future...).

> 
>>>> --- a/xen/drivers/passthrough/iommu.c
>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>        hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>>>        if ( need_iommu_pt_sync(d) )
>>>>        {
>>>> +        int rc = 0;
>>>> +#ifdef CONFIG_HAS_M2P
>>>>            struct page_info *page;
>>>>            unsigned int i = 0, flush_flags = 0;
>>>> -        int rc = 0;
>>>>    
>>>>            page_list_for_each ( page, &d->page_list )
>>>>            {
>>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>            /* Use while-break to avoid compiler warning */
>>>>            while ( iommu_iotlb_flush_all(d, flush_flags) )
>>>>                break;
>>>> +#else
>>>> +        rc = -EOPNOTSUPP;
>>>> +#endif
>>>>    
>>>>            if ( rc )
>>>>                printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>>>
>>> Would you mind extending the scope of the #ifdef beyond this printk()?
>>> It seems pretty pointless to me to unconditionally emit a log message
>>> here.
>>
>> Well, it at least tell you the function can't work. So I think it is still makes
>> sense to have it.
> 
> I disagree.
You disagree because...?

I hope you are aware, this is unlikely going to be printed as the code should 
not be called. If it ever happens, it is easier for a user to grep the code for 
the message rather than having to add some to find out where the -EOPNOTSUPP is 
coming from.

Cheers,
Jan Beulich May 10, 2019, 1:45 p.m. UTC | #10
>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
> On 10/05/2019 14:32, Jan Beulich wrote:
>>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote:
>>> On 10/05/2019 13:31, Jan Beulich wrote:
>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>>> --- a/xen/common/domctl.c
>>>>> +++ b/xen/common/domctl.c
>>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>>>>> xen_domctl_getdomaininfo *info)
>>>>>        info->outstanding_pages = d->outstanding_pages;
>>>>>        info->shr_pages         = atomic_read(&d->shr_pages);
>>>>>        info->paged_pages       = atomic_read(&d->paged_pages);
>>>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>>>>
>>>> What is the intended behavior on 32-bit Arm here? Do you really
>>>> mean to return a value with 32 bits of ones (instead of 64 bits of
>>>> them) in this 64-bit field?
>>> It does not matter as long as the GFN is invalid so it can't be mapped
>>> afterwards. The exact value is not documented in the header to avoid any
>>> assumption.
>> 
>> That's not helpful - how would a consumer know to avoid the mapping
>> attempt in the first place?
> 
> I can't see any issue with the consumer to try to map it. Ok, you will waste a 
> couple of cycles, but that should be pretty rare.

The attempt may result in a log message spit out.

> The point here, we keep within the hypervisor the idea of what's valid or 
> invalid. This allows us more flexibility on the value here (imagine we decide to 
> change the value of GFN_INVALID in the future...).

Exactly, and hence INVALID_GFN should not become visible to the
outside. Hence my request to use an all-ones value here.

>>>>> --- a/xen/drivers/passthrough/iommu.c
>>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>>        hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>>>>        if ( need_iommu_pt_sync(d) )
>>>>>        {
>>>>> +        int rc = 0;
>>>>> +#ifdef CONFIG_HAS_M2P
>>>>>            struct page_info *page;
>>>>>            unsigned int i = 0, flush_flags = 0;
>>>>> -        int rc = 0;
>>>>>    
>>>>>            page_list_for_each ( page, &d->page_list )
>>>>>            {
>>>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>>            /* Use while-break to avoid compiler warning */
>>>>>            while ( iommu_iotlb_flush_all(d, flush_flags) )
>>>>>                break;
>>>>> +#else
>>>>> +        rc = -EOPNOTSUPP;
>>>>> +#endif
>>>>>    
>>>>>            if ( rc )
>>>>>                printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>>>>
>>>> Would you mind extending the scope of the #ifdef beyond this printk()?
>>>> It seems pretty pointless to me to unconditionally emit a log message
>>>> here.
>>>
>>> Well, it at least tell you the function can't work. So I think it is still makes
>>> sense to have it.
>> 
>> I disagree.
> You disagree because...?

Because of what I've said in my initial reply (still quoted above).

> I hope you are aware, this is unlikely going to be printed as the code should 
> not be called.

ASSERT_UNREACHABLE() then?

Jan
Julien Grall May 10, 2019, 2:04 p.m. UTC | #11
Hi Jan,

On 10/05/2019 14:45, Jan Beulich wrote:
>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>> On 10/05/2019 14:32, Jan Beulich wrote:
>>>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote:
>>>> On 10/05/2019 13:31, Jan Beulich wrote:
>>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>>>>>> --- a/xen/common/domctl.c
>>>>>> +++ b/xen/common/domctl.c
>>>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>>>>>> xen_domctl_getdomaininfo *info)
>>>>>>         info->outstanding_pages = d->outstanding_pages;
>>>>>>         info->shr_pages         = atomic_read(&d->shr_pages);
>>>>>>         info->paged_pages       = atomic_read(&d->paged_pages);
>>>>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>>>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>>>>>
>>>>> What is the intended behavior on 32-bit Arm here? Do you really
>>>>> mean to return a value with 32 bits of ones (instead of 64 bits of
>>>>> them) in this 64-bit field?
>>>> It does not matter as long as the GFN is invalid so it can't be mapped
>>>> afterwards. The exact value is not documented in the header to avoid any
>>>> assumption.
>>>
>>> That's not helpful - how would a consumer know to avoid the mapping
>>> attempt in the first place?
>>
>> I can't see any issue with the consumer to try to map it. Ok, you will waste a
>> couple of cycles, but that should be pretty rare.
> 
> The attempt may result in a log message spit out.

I still can't see the issue here. This is nothing different than the frame were 
not setup beforehand.

> 
>> The point here, we keep within the hypervisor the idea of what's valid or
>> invalid. This allows us more flexibility on the value here (imagine we decide to
>> change the value of GFN_INVALID in the future...).
> 
> Exactly, and hence INVALID_GFN should not become visible to the
> outside. Hence my request to use an all-ones value here.
It is only visible if you put an exact value in the documentation. Your 
suggestion is to put a exactly value and I would rather not see that.

>>>>>> --- a/xen/drivers/passthrough/iommu.c
>>>>>> +++ b/xen/drivers/passthrough/iommu.c
>>>>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>>>         hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>>>>>         if ( need_iommu_pt_sync(d) )
>>>>>>         {
>>>>>> +        int rc = 0;
>>>>>> +#ifdef CONFIG_HAS_M2P
>>>>>>             struct page_info *page;
>>>>>>             unsigned int i = 0, flush_flags = 0;
>>>>>> -        int rc = 0;
>>>>>>     
>>>>>>             page_list_for_each ( page, &d->page_list )
>>>>>>             {
>>>>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>>>>>             /* Use while-break to avoid compiler warning */
>>>>>>             while ( iommu_iotlb_flush_all(d, flush_flags) )
>>>>>>                 break;
>>>>>> +#else
>>>>>> +        rc = -EOPNOTSUPP;
>>>>>> +#endif
>>>>>>     
>>>>>>             if ( rc )
>>>>>>                 printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
>>>>>
>>>>> Would you mind extending the scope of the #ifdef beyond this printk()?
>>>>> It seems pretty pointless to me to unconditionally emit a log message
>>>>> here.
>>>>
>>>> Well, it at least tell you the function can't work. So I think it is still makes
>>>> sense to have it.
>>>
>>> I disagree.
>> You disagree because...?
> 
> Because of what I've said in my initial reply (still quoted above).

I still don't see the problem of unconditional log message. It is not really the 
first place we have that.

> 
>> I hope you are aware, this is unlikely going to be printed as the code should
>> not be called.
> 
> ASSERT_UNREACHABLE() then?

And still avoiding the printk?

Cheers,
Jan Beulich May 10, 2019, 2:19 p.m. UTC | #12
>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote:
> On 10/05/2019 14:45, Jan Beulich wrote:
>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>>> The point here, we keep within the hypervisor the idea of what's valid or
>>> invalid. This allows us more flexibility on the value here (imagine we decide to
>>> change the value of GFN_INVALID in the future...).
>> 
>> Exactly, and hence INVALID_GFN should not become visible to the
>> outside. Hence my request to use an all-ones value here.
> It is only visible if you put an exact value in the documentation. Your 
> suggestion is to put a exactly value and I would rather not see that.

I did specifically suggest to _not_ store INVALID_GFN here, but to
store 64-bit bits of ones. Note the difference between the two on
32-bit Arm.

>>>>> Well, it at least tell you the function can't work. So I think it is still makes
>>>>> sense to have it.
>>>>
>>>> I disagree.
>>> You disagree because...?
>> 
>> Because of what I've said in my initial reply (still quoted above).
> 
> I still don't see the problem of unconditional log message. It is not really the 
> first place we have that.
> 
>> 
>>> I hope you are aware, this is unlikely going to be printed as the code should
>>> not be called.
>> 
>> ASSERT_UNREACHABLE() then?
> 
> And still avoiding the printk?

Preferably yes; depends on how exactly you code the assertion.
If you follow the if()-ASSERT_UNREACHABLE()-return style we've
been using elsewhere, then no matter how you place the #else
or #endif the printk() will be compiled out.

Jan
Julien Grall May 10, 2019, 2:34 p.m. UTC | #13
Hi Jan,

On 10/05/2019 15:19, Jan Beulich wrote:
>>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote:
>> On 10/05/2019 14:45, Jan Beulich wrote:
>>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>>>> The point here, we keep within the hypervisor the idea of what's valid or
>>>> invalid. This allows us more flexibility on the value here (imagine we decide to
>>>> change the value of GFN_INVALID in the future...).
>>>
>>> Exactly, and hence INVALID_GFN should not become visible to the
>>> outside. Hence my request to use an all-ones value here.
>> It is only visible if you put an exact value in the documentation. Your
>> suggestion is to put a exactly value and I would rather not see that.
> 
> I did specifically suggest to _not_ store INVALID_GFN here, but to
> store 64-bit bits of ones. Note the difference between the two on
> 32-bit Arm.
Your point of having an exact value is only useful if you want to toolstack to 
silently ignore the missing frame and avoid a call.

The former is pretty much wrong as if you were trying to read the frame then 
most likely you wanted to access it. So a message makes sense here.

For the latter, avoiding the call is only going to save you a couple of cycles 
in a likely cold path.

You really don't need to give an exact (including say all ones). You only need 
to say that the address return may not be mappable. The toolstack will try to 
map it and fail. That's not a big deal.

Anyway, I will wait and see what's the view from the tools maintainer.

> 
> Preferably yes; depends on how exactly you code the assertion.
> If you follow the if()-ASSERT_UNREACHABLE()-return style we've
> been using elsewhere, then no matter how you place the #else
> or #endif the printk() will be compiled out.

I will have a look.

Cheers,
Julien Grall May 22, 2019, 4:03 p.m. UTC | #14
Hi,

Answering to myself.

On 10/05/2019 15:34, Julien Grall wrote:
> On 10/05/2019 15:19, Jan Beulich wrote:
>>>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote:
>>> On 10/05/2019 14:45, Jan Beulich wrote:
>>>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote:
>>>>> The point here, we keep within the hypervisor the idea of what's valid or
>>>>> invalid. This allows us more flexibility on the value here (imagine we 
>>>>> decide to
>>>>> change the value of GFN_INVALID in the future...).
>>>>
>>>> Exactly, and hence INVALID_GFN should not become visible to the
>>>> outside. Hence my request to use an all-ones value here.
>>> It is only visible if you put an exact value in the documentation. Your
>>> suggestion is to put a exactly value and I would rather not see that.
>>
>> I did specifically suggest to _not_ store INVALID_GFN here, but to
>> store 64-bit bits of ones. Note the difference between the two on
>> 32-bit Arm.
> Your point of having an exact value is only useful if you want to toolstack to 
> silently ignore the missing frame and avoid a call.
> 
> The former is pretty much wrong as if you were trying to read the frame then 
> most likely you wanted to access it. So a message makes sense here.
> 
> For the latter, avoiding the call is only going to save you a couple of cycles 
> in a likely cold path.
> 
> You really don't need to give an exact (including say all ones). You only need 
> to say that the address return may not be mappable. The toolstack will try to 
> map it and fail. That's not a big deal.
> 
> Anyway, I will wait and see what's the view from the tools maintainer.

I had a discussion with Ian on IRC regarding the value here. After some debate 
we agreed that specifying a single value would be best.

At the moment, Xen only supports 48-bits address that could be covered by 36-bit 
MFN. Newer Arm revision supports up to 52-bit address, but that's only with 64KB 
page granularity. Even if we were supporting 64-bit address, it would only cover 
48-bit, so all ones should still be invalid. So I am happy with the all ones 
version.

I will introduce a define so the tools can use it rather than an hardcoded value.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 4b8b07b549..52922a87e7 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -16,6 +16,7 @@  config X86
 	select HAS_IOPORTS
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
+	select HAS_M2P
 	select HAS_MEM_PAGING
 	select HAS_MEM_SHARING
 	select HAS_NS16550
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506241..df871adc8f 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -63,6 +63,9 @@  config HAS_GDBSX
 config HAS_IOPORTS
 	bool
 
+config HAS_M2P
+	bool
+
 config NEEDS_LIBELF
 	bool
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index bade9a63b1..29940fdea5 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -205,7 +205,7 @@  void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
-    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
+    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
     BUG_ON(SHARED_M2P(info->shared_info_frame));
 
     info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 86567e6117..d6a580da31 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -512,6 +512,7 @@  static bool propagate_node(unsigned int xmf, unsigned int *memflags)
 
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
+#ifdef CONFIG_M2P
     struct xen_memory_exchange exch;
     PAGE_LIST_HEAD(in_chunk_list);
     PAGE_LIST_HEAD(out_chunk_list);
@@ -806,6 +807,9 @@  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
     if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
         rc = -EFAULT;
     return rc;
+#else /* !CONFIG_M2P */
+    return -EOPNOTSUPP;
+#endif
 }
 
 int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index a6697d58fb..dbb64b13bc 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -188,9 +188,10 @@  void __hwdom_init iommu_hwdom_init(struct domain *d)
     hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
     if ( need_iommu_pt_sync(d) )
     {
+        int rc = 0;
+#ifdef CONFIG_HAS_M2P
         struct page_info *page;
         unsigned int i = 0, flush_flags = 0;
-        int rc = 0;
 
         page_list_for_each ( page, &d->page_list )
         {
@@ -217,6 +218,9 @@  void __hwdom_init iommu_hwdom_init(struct domain *d)
         /* Use while-break to avoid compiler warning */
         while ( iommu_iotlb_flush_all(d, flush_flags) )
             break;
+#else
+        rc = -EOPNOTSUPP;
+#endif
 
         if ( rc )
             printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 312fec8932..d61b0188da 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -267,6 +267,11 @@  static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
 
 static inline void arch_vcpu_block(struct vcpu *v) {}
 
+static inline gfn_t domain_shared_info_gfn(struct domain *d)
+{
+    return INVALID_GFN;
+}
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 19486d5e32..cac8ffffe9 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -118,6 +118,10 @@  struct xen_domctl_getdomaininfo {
     uint64_aligned_t outstanding_pages;
     uint64_aligned_t shr_pages;
     uint64_aligned_t paged_pages;
+    /*
+     * GFN of shared_info struct. Some architectures (e.g Arm) may not
+     * provide a valid GFN.
+     */
     uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
     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 d1bfc82f57..f1761fe183 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -118,4 +118,12 @@  struct vnuma_info {
 
 void vnuma_destroy(struct vnuma_info *vnuma);
 
+#ifdef CONFIG_HAS_M2P
+#define domain_shared_info_gfn(d) ({                            \
+    const struct domain *d_ = (d);                              \
+                                                                \
+    mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));       \
+})
+#endif
+
 #endif /* __XEN_DOMAIN_H__ */