[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/4 16:02, Jan Beulich wrote:
On 04.11.14 at 02:35, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/11/3 19:53, Jan Beulich wrote:
On 03.11.14 at 12:48, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/11/3 18:03, Jan Beulich wrote:
On 03.11.14 at 10:51, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/11/3 17:00, Jan Beulich wrote:
On 03.11.14 at 07:20, <tiejun.chen@xxxxxxxxx> wrote:
#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?

No. All error scenarios are already being handled here (by
propagating the error code to the caller).

Sorry, how to propagate the error code?

Return it to the caller (and it will do so onwards, until it reaches
[presumably] the entity having invoked a hypercall).

I guess you mean we should return out in this error case,


@@ -686,8 +686,25 @@ guest_physmap_add_entry(struct domain *d, unsigned long 
       /* 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 = 
+                                                  &gfn);
+            /* We always avoid populating reserved device memory. */
+            if ( rc == 1 )
+                goto out;

But you'll need to make sure that you don't return 1 to the callers:

You're right.

They expect 0 or negative error codes. But with the model of
not even populating these regions (or relocating the memory
before [at boot time] assigning a device associated with an RMRR)
I think this needs to become an error anyway.

Looks -EINVAL might be used.

+            else if ( rc < 0 )
+            {
+                printk(XENLOG_G_WARNING
+                       "Dom%d can't check reserved device memory.\n",

Actually, d being the subject domain, please make this more like
"Can't check reserved device memory for Dom%d\n".




+                       d->domain_id);
+                goto out;
+            }
+        }
+        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a);
           if ( rc )
               goto out; /* Failed to update p2m, bail without updating m2p. */


Xen-devel mailing list



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