Re: [v5][PATCH 1/2] xen:x86:mm:p2m: introduce set_identity_p2m_entry

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/7/29 18:27, Jan Beulich wrote:
>>>> On 29.07.14 at 12:18, <tiejun.chen@intel.com> wrote:
>> On 2014/7/29 17:53, Jan Beulich wrote:
>>>>>> On 29.07.14 at 11:11, <tiejun.chen@intel.com> wrote:
>>>> On 2014/7/29 16:19, Jan Beulich wrote:
>>>>> ? Of course it may still be necessary to also inspect the obtained p2mt
>>>>> and a.
>>>>>
>>>>
>>>> Are you saying this?
>>>>
>>>> 	if ( !p2m_is_valid(p2mt) ||
>>>> 	     !mfn_valid(mfn) ||
>>>> 	     (a != p2m_access_rw) )
>>>
>>> I'm afraid that's not enough context to know whether what you
>>> mean to do is sufficient. Plus !p2m_is_valid() is too weak. You
>>> simply need to properly think through what should happen if you
>>> find a valid mapping, but any of the tuple (mfn, p2mt, a) don't
>>> match what you intend to be there.
>>>
>>
>> Actually as I understand we can create these mapping only in one case of
>> !mfn_valid(mfn). For others scenarios we just return with that  warning
>> message no matter what that tuple is explicitly. So here I try to
>> understand why you're saying we need check more by show this condition
>> combination.
>
> Perhaps, but with the exception that at least if the entire tuple
> matches you should return success (and not print anything).
> There might be further cases where an existing mapping would
> be good enough (like a being p2m_access_rwx), but perhaps
> there's not much point in trying to deal with them without explicit
> need.
>

I think the following cases should be enough:

#1: !mfn_valid(mfn)

We can create those mapping safely.

#2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw

We already have these matched mappings.

#3: Others

Return with that waring message: "Cannot identity map d%d:%lx, already 
mapped to %lx but mismatch.\n"

So what about this?

@@ -858,6 +858,35 @@ 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);
  }

+int set_identity_p2m_entry(struct domain *d, unsigned long gfn)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret = -EBUSY;
+
+    gfn_lock(p2m, gfn, 0);
+
+    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
+
+    if ( !mfn_valid(mfn) )
+        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, 
p2m_mmio_direct,
+                            p2m_access_rw);
+    else if ( mfn_x(mfn) == gfn &&
+              p2mt == p2m_mmio_direct &&
+              a == p2m_access_rw )
+        ret = 0;
+    else
+        printk(XENLOG_G_WARNING
+               "Cannot identity map d%d:%lx, already mapped to %lx but 
mismatch.\n",
+               d->domain_id, gfn, mfn_x(mfn));
+
+    gfn_unlock(p2m, gfn, 0);
+
+    return ret;
+}
+
  /* Returns: 0 for success, -errno for failure */
  int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
  {


Thanks
Tiejun