[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:45, Jan Beulich wrote:
On 10.05.19 at 15:41, <julien.grall@xxxxxxx> wrote:
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 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,

--
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®.