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

Re: [Xen-devel] [v3][PATCH 02/16] xen/x86/p2m: introduce set_identity_p2m_entry



On 2015/6/11 15:33, Jan Beulich wrote:
On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> wrote:
We will create this sort of identity mapping as follows:

If the gfn space is unoccupied, we just set the mapping. If the space
is already occupied by 1:1 mappings, do nothing. Failed for any
other cases.

Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>

First of all you continue to be copying each patch to every
maintainer involved in some part of the series. Please limit the

I just hope all involved guys can see this series on the whole to review. But,

Cc list of each patch to the actual list of people needing to be
Cc-ed on it (or you know explicitly wanting a copy).

Next, I will just send them to each associated maintainer.


--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -898,6 +898,41 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
gfn, mfn_t mfn,
      return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
  }

+int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
+                           p2m_access_t p2ma)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret;
+
+    if ( paging_mode_translate(p2m->domain) )
+    {
+        gfn_lock(p2m, gfn, 0);
+
+        mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
+
+        if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN )

I'm not fundamentally opposed to this extra INVALID_MFN check, but
could you please clarify for which P2M type you saw INVALID_MFN
coming back here, and for which p2m_invalid cases you didn't also
see INVALID_MFN? I.e. I'd really prefer a single check to be used
when that can cover all cases.

Actually, I initially adopted "!mfn_valid(mfn)" in our previous version. But Tim issued one comment about this,

"I don't think this check is quite right -- for example, this p2m entry
might be an MMIO mapping or a PoD entry.  "if ( p2mt == p2m_invalid )"
would be better."

But if I just keep his recommended check, you can see the following when I pass through IGD,

(XEN) Cannot identity map d1:ad800, already mapped to ffffffffffffffff with p2mt:4.

Looks "4" indicates p2m_mmio_dm, right?


+            ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
+                                p2m_mmio_direct, p2ma);
+        else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
+            ret = 0;
+        else
+        {
+            ret = -EBUSY;
+            printk(XENLOG_G_WARNING
+                   "Cannot identity map d%d:%lx, already mapped to %lx.\n",
+                   d->domain_id, gfn, mfn_x(mfn));
+        }
+
+        gfn_unlock(p2m, gfn, 0);
+        return ret;
+    }
+
+    return 0;
+}

Either have a single return point, or invert the original if() and bail
early (reducing the indentation level on the main body of the code).


Sure, I guess I can follow this patten,

int ret = 0;

if ()
{
    ...
}

return ret;

Thanks
Tiejun

Jan




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