Hi,
On 10/05/2019 13:31, Jan Beulich wrote:
On 07.05.19 at 17:14, <julien.gr...@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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
