[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Jan,

On 10/05/2019 14:32, Jan Beulich wrote:
On 10.05.19 at 15:22, <julien.grall@xxxxxxx> wrote:
On 10/05/2019 13:31, Jan Beulich wrote:
On 07.05.19 at 17:14, <julien.grall@xxxxxxx> 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.