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

Re: [Xen-devel] [v7][RFC][PATCH 08/13] xen/x86/p2m: set p2m_access_n for reserved device memory mapping



On 2014/11/3 17:00, Jan Beulich wrote:
On 03.11.14 at 07:20, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/31 16:25, Jan Beulich wrote:
On 31.10.14 at 03:50, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/10/30 17:24, Jan Beulich wrote:
On 30.10.14 at 08:39, <tiejun.chen@xxxxxxxxx> wrote:
@@ -686,8 +686,22 @@ guest_physmap_add_entry(struct domain *d, unsigned long 
gfn,
         /* Now, actually do the two-way mapping */
         if ( mfn_valid(_mfn(mfn)) )
         {
-        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
-                           p2m->default_access);
+        rc = 0;
+        a =  p2m->default_access;
+        if ( !is_hardware_domain(d) )
+        {
+            rc = 
iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
+                                                  &gfn);
+            /* We need to set reserved device memory as p2m_access_n. */
+            if ( rc == 1 )
+                a = p2m_access_n;
+            else if ( rc < 0 )
+                printk(XENLOG_WARNING
+                       "Domain %d can't check reserved device memory.\n",
+                       d->domain_id);
+        }
+
+        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a);
             if ( rc )
                 goto out; /* Failed to update p2m, bail without updating m2p.
*/

The handling of "a" looks good now, but the error handling and
logging is still as broken as it was before.

Do you mean I'm missing some necessary info? Like gfn and mfn, so domain
id, gfn and mfn can show enough message.

Sorry I'm poor to understand what you expect.

But I explained it already, and that explanation is still visible in
the quotes above. But to avoid any doubt, I'll repeat: "And

I tried to understand what you said but felt a confusion so ask if you
show me directly.

properly handle the error case (just logging a message - which
btw lacks a proper XENLOG_G_* prefix - doesn't seem enough
to me)."

Looks there are two problems:

#1: the error message

If current line is not fine,
        printk(XENLOG_G_WARNING "Domain %d can't check reserved device
memory.\n", d->domain_id);

I mean could you change this directly.

This looks reasonable, albeit we generally prefer Dom%d or dom%d
so that messages are somewhat grep-able.

Fixed.


#2 the error handling

In an error case what should I do? Currently we still create these
mapping as normal. This means these mfns will be valid so later we can't
set them again then device can't be assigned as passthrough. I think
this makes sense. Or we should just stop them from setting 1:1 mapping?

You should, with very few exceptions, not ignore errors (which
includes "handling" them by just logging a message. Instead, you
should propagate the error back up the call chain.


Do you mean in your patch,

+int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    if ( !iommu_enabled || !ops->get_reserved_device_memory )
+        return 0;
+
+    return ops->get_reserved_device_memory(func, ctxt);
+}
+

I shouldn't return that directly. Then instead, we should handle all error scenarios here?

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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