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

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

[Tiejun Chen]

Its used conveniently to create RMRR mapping in shared EPT case.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/mm/p2m.c     | 26 ++++++++++++++++++++++++++
 xen/include/asm-x86/p2m.h |  3 +++
 2 files changed, 29 insertions(+)

v5:

* Rename this function as set_identity_p2m_entry()
* Get mfn directly inside set_identity_p2m_entry()

v4:

* new patch to combine get and set together to create RMRR mapping.

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..f83f194 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -858,6 +858,32 @@ 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 tmp_mfn, mfn = _mfn(gfn);
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret = -EBUSY;
+
+    gfn_lock(p2m, gfn, 0);
+
+    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
+
+    if ( mfn_valid(tmp_mfn) )
+    {
+        gdprintk(XENLOG_ERR,
+                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
+        goto out;
+    }
+
+    ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct,
+                        p2m_access_rw);
+ out:
+    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)
 {
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0ddbadb..d130f9a 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -536,6 +536,9 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
 int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
 
+/* Set identity addresses in the p2m table (for pass-through) */
+int set_identity_p2m_entry(struct domain *d, unsigned long gfn);
+
 /* Add foreign mapping to the guest's p2m table. */
 int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
                     unsigned long gpfn, domid_t foreign_domid);
-- 
1.9.1

[v5][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tiejun Chen]

intel_iommu_map_page() does nothing if VT-d shares EPT page table.
So rmrr_identity_mapping() never create RMRR mapping but in some
cases like some GFX drivers it still need to access RMRR.

Here we will create those RMRR mappings even in shared EPT case.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

v5:

* Don't pass mfn directly to set_identity_p2m_entry() since it can get that.

v4:

* After introduce set_rmrr_p2m_entry we can go there directly.

v3:

* Use set_mmio_p2m_entry() to replace p2m_set_entry()
* Use ASSERT to check

v2:

* Fix coding style.
* Still need to abide intel_iommu_map_page(), so we should do nothing
  if dom0 and iommu supports pass thru.

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..bc50793 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
 
     while ( base_pfn < end_pfn )
     {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
-                                  IOMMUF_readable|IOMMUF_writable) )
+        if ( iommu_use_hap_pt(d) )
+        {
+            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
+            if ( set_identity_p2m_entry(d, base_pfn) )
+                return -1;
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+                                       IOMMUF_readable|IOMMUF_writable) )
             return -1;
         base_pfn++;
     }
-- 
1.9.1

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

[Jan Beulich]

>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn)
> +{
> +    p2m_type_t p2mt;
> +    p2m_access_t a;
> +    mfn_t tmp_mfn, mfn = _mfn(gfn);

No need for the mfn variable; instead what currently is tmp_mfn
should be named just mfn, and the _mfn(gfn) construction can be
done right in the function call.

> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int ret = -EBUSY;
> +
> +    gfn_lock(p2m, gfn, 0);
> +
> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
> +
> +    if ( mfn_valid(tmp_mfn) )
> +    {
> +        gdprintk(XENLOG_ERR,
> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);

Pointless cast: Just use %lx in the format string. Additionally I don't
think the message text is correct: You don't really know whether
what's there is another RMRR (or that the context you're being
called in refers to an RMRR at all). On the contrary - if it was an
RMRR (or to be precise, a previously established identity mapping),
you'd want to report success. And generally we have no stop at
the end of log messages.

> +        goto out;

Once again, when error handling is that simple please avoid using
"goto".

Jan

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

[Chen, Tiejun]

On 2014/7/29 15:05, Jan Beulich wrote:
>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn)
>> +{
>> +    p2m_type_t p2mt;
>> +    p2m_access_t a;
>> +    mfn_t tmp_mfn, mfn = _mfn(gfn);
>
> No need for the mfn variable; instead what currently is tmp_mfn
> should be named just mfn, and the _mfn(gfn) construction can be
> done right in the function call.
>

Okay.

>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    int ret = -EBUSY;
>> +
>> +    gfn_lock(p2m, gfn, 0);
>> +
>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>> +
>> +    if ( mfn_valid(tmp_mfn) )
>> +    {
>> +        gdprintk(XENLOG_ERR,
>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>
> Pointless cast: Just use %lx in the format string. Additionally I don't
> think the message text is correct: You don't really know whether
> what's there is another RMRR (or that the context you're being
> called in refers to an RMRR at all). On the contrary - if it was an
> RMRR (or to be precise, a previously established identity mapping),
> you'd want to report success. And generally we have no stop at
> the end of log messages.

So just print this,

+        gdprintk(XENLOG_ERR,
+                 "Overlapping at %lx.\n", (paddr_t)gfn);


>
>> +        goto out;
>
> Once again, when error handling is that simple please avoid using
> "goto".
>

Its make no sense to me.

Did you see this function in this same file,

int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn)
{
     int rc = -EINVAL;
     mfn_t mfn;
     p2m_access_t a;
     p2m_type_t t;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);

     if ( !paging_mode_translate(d) )
         return -EIO;

     gfn_lock(p2m, gfn, 0);
     mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);

     /* Do not use mfn_valid() here as it will usually fail for MMIO 
pages. */
     if ( (INVALID_MFN == mfn_x(mfn)) || (t != p2m_mmio_direct) )
     {
         gdprintk(XENLOG_ERR,
                  "gfn_to_mfn failed! gfn=%08lx type:%d\n", gfn, t);
         goto out;
     }
     rc = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K, 
p2m_invalid,
                        p2m->default_access);

  out:
     gfn_unlock(p2m, gfn, 0);

     return rc;
}

Yes, previously I really can't understand what's that code style in xen. 
So as I remember I ask you guy if xen has checkpatch.pl like Linux, qemu 
or other stuff, but you didn't reply this point. So I have to try 
following existing codes. Now I'm curious what we should abide.

Thanks
Tiejun

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

[Jan Beulich]

>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>> +    int ret = -EBUSY;
>>> +
>>> +    gfn_lock(p2m, gfn, 0);
>>> +
>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>> +
>>> +    if ( mfn_valid(tmp_mfn) )
>>> +    {
>>> +        gdprintk(XENLOG_ERR,
>>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>>
>> Pointless cast: Just use %lx in the format string. Additionally I don't
>> think the message text is correct: You don't really know whether
>> what's there is another RMRR (or that the context you're being
>> called in refers to an RMRR at all). On the contrary - if it was an
>> RMRR (or to be precise, a previously established identity mapping),
>> you'd want to report success. And generally we have no stop at
>> the end of log messages.
> 
> So just print this,
> 
> +        gdprintk(XENLOG_ERR,
> +                 "Overlapping at %lx.\n", (paddr_t)gfn);

Of course not - such a message is really meaningless.

printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to %lx\n",
       d->domain_id, gfn, mfn_x(mfn));

would be a message conveying all information necessary to
gain initial understanding of what the issue is.

>>
>>> +        goto out;
>>
>> Once again, when error handling is that simple please avoid using
>> "goto".
>>
> 
> Its make no sense to me.
> 
> Did you see this function in this same file,

Referring to existing bad examples is never going to help.

> Yes, previously I really can't understand what's that code style in xen. 
> So as I remember I ask you guy if xen has checkpatch.pl like Linux, qemu 
> or other stuff, but you didn't reply this point.

We just have none, and for the specific case of using or not using
"goto" it wouldn't help you anyway.

> So I have to try 
> following existing codes. Now I'm curious what we should abide.

Where is the problem with just writing

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;

    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 )
       ret = 0;
    else
    {
        printk(XENLOG_G_WARNING
               "Cannot identity map %d:%lx, already mapped to %lx\n",
               d->domain_id, gfn, mfn_x(mfn));
        ret = -EBUSY;
    }

    gfn_unlock(p2m, gfn, 0);

    return ret;
}

? Of course it may still be necessary to also inspect the obtained p2mt
and a.

Jan

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

[Andrew Cooper]

On 29/07/2014 09:19, Jan Beulich wrote:
>>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
>> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> +    int ret = -EBUSY;
>>>> +
>>>> +    gfn_lock(p2m, gfn, 0);
>>>> +
>>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>> +
>>>> +    if ( mfn_valid(tmp_mfn) )
>>>> +    {
>>>> +        gdprintk(XENLOG_ERR,
>>>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>>> Pointless cast: Just use %lx in the format string. Additionally I don't
>>> think the message text is correct: You don't really know whether
>>> what's there is another RMRR (or that the context you're being
>>> called in refers to an RMRR at all). On the contrary - if it was an
>>> RMRR (or to be precise, a previously established identity mapping),
>>> you'd want to report success. And generally we have no stop at
>>> the end of log messages.
>> So just print this,
>>
>> +        gdprintk(XENLOG_ERR,
>> +                 "Overlapping at %lx.\n", (paddr_t)gfn);
> Of course not - such a message is really meaningless.
>
> printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to %lx\n",
>        d->domain_id, gfn, mfn_x(mfn));
>
> would be a message conveying all information necessary to
> gain initial understanding of what the issue is.

This would be better as "map d%d:%lx" to indicate that it is a domid
before the colon.

~Andrew

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

[Jan Beulich]

>>> On 29.07.14 at 10:46, <andrew.cooper3@citrix.com> wrote:
> On 29/07/2014 09:19, Jan Beulich wrote:
>>>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>> +    int ret = -EBUSY;
>>>>> +
>>>>> +    gfn_lock(p2m, gfn, 0);
>>>>> +
>>>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>>> +
>>>>> +    if ( mfn_valid(tmp_mfn) )
>>>>> +    {
>>>>> +        gdprintk(XENLOG_ERR,
>>>>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>>>> Pointless cast: Just use %lx in the format string. Additionally I don't
>>>> think the message text is correct: You don't really know whether
>>>> what's there is another RMRR (or that the context you're being
>>>> called in refers to an RMRR at all). On the contrary - if it was an
>>>> RMRR (or to be precise, a previously established identity mapping),
>>>> you'd want to report success. And generally we have no stop at
>>>> the end of log messages.
>>> So just print this,
>>>
>>> +        gdprintk(XENLOG_ERR,
>>> +                 "Overlapping at %lx.\n", (paddr_t)gfn);
>> Of course not - such a message is really meaningless.
>>
>> printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to 
> %lx\n",
>>        d->domain_id, gfn, mfn_x(mfn));
>>
>> would be a message conveying all information necessary to
>> gain initial understanding of what the issue is.
> 
> This would be better as "map d%d:%lx" to indicate that it is a domid
> before the colon.

I actually meant it to be that way - thanks for noticing.

Jan

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

[Chen, Tiejun]

On 2014/7/29 16:19, Jan Beulich wrote:
>>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
>> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> +    int ret = -EBUSY;
>>>> +
>>>> +    gfn_lock(p2m, gfn, 0);
>>>> +
>>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>> +
>>>> +    if ( mfn_valid(tmp_mfn) )
>>>> +    {
>>>> +        gdprintk(XENLOG_ERR,
>>>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>>>
>>> Pointless cast: Just use %lx in the format string. Additionally I don't
>>> think the message text is correct: You don't really know whether
>>> what's there is another RMRR (or that the context you're being
>>> called in refers to an RMRR at all). On the contrary - if it was an
>>> RMRR (or to be precise, a previously established identity mapping),
>>> you'd want to report success. And generally we have no stop at
>>> the end of log messages.
>>
>> So just print this,
>>
>> +        gdprintk(XENLOG_ERR,
>> +                 "Overlapping at %lx.\n", (paddr_t)gfn);
>
> Of course not - such a message is really meaningless.
>
> printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to %lx\n",
>         d->domain_id, gfn, mfn_x(mfn));
>

Thanks your further help.

> would be a message conveying all information necessary to
> gain initial understanding of what the issue is.
>
>>>
>>>> +        goto out;
>>>
>>> Once again, when error handling is that simple please avoid using
>>> "goto".
>>>
>>
>> Its make no sense to me.
>>
>> Did you see this function in this same file,
>
> Referring to existing bad examples is never going to help.

But sometimes someone who are not familiar with this case may have no 
choice because something is really tricky.

>
>> Yes, previously I really can't understand what's that code style in xen.
>> So as I remember I ask you guy if xen has checkpatch.pl like Linux, qemu
>> or other stuff, but you didn't reply this point.
>
> We just have none, and for the specific case of using or not using
> "goto" it wouldn't help you anyway.
>
>> So I have to try
>> following existing codes. Now I'm curious what we should abide.
>
> Where is the problem with just writing

I doesn't mean I have a problem to write such codes but I think we need 
an unified coding style.

Anyway I can do as you show.

>
> int k(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;
>
>      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 )
>         ret = 0;
>      else
>      {
>          printk(XENLOG_G_WARNING
>                 "Cannot identity map %d:%lx, already mapped to %lx\n",
>                 d->domain_id, gfn, mfn_x(mfn));
>          ret = -EBUSY;
>      }
>
>      gfn_unlock(p2m, gfn, 0);
>
>      return ret;
> }
>
> ? 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) )


Thanks
Tiejun

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

[Chen, Tiejun]

On 2014/7/29 16:46, Andrew Cooper wrote:
> On 29/07/2014 09:19, Jan Beulich wrote:
>>>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>> +    int ret = -EBUSY;
>>>>> +
>>>>> +    gfn_lock(p2m, gfn, 0);
>>>>> +
>>>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>>> +
>>>>> +    if ( mfn_valid(tmp_mfn) )
>>>>> +    {
>>>>> +        gdprintk(XENLOG_ERR,
>>>>> +                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);
>>>> Pointless cast: Just use %lx in the format string. Additionally I don't
>>>> think the message text is correct: You don't really know whether
>>>> what's there is another RMRR (or that the context you're being
>>>> called in refers to an RMRR at all). On the contrary - if it was an
>>>> RMRR (or to be precise, a previously established identity mapping),
>>>> you'd want to report success. And generally we have no stop at
>>>> the end of log messages.
>>> So just print this,
>>>
>>> +        gdprintk(XENLOG_ERR,
>>> +                 "Overlapping at %lx.\n", (paddr_t)gfn);
>> Of course not - such a message is really meaningless.
>>
>> printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to %lx\n",
>>         d->domain_id, gfn, mfn_x(mfn));
>>
>> would be a message conveying all information necessary to
>> gain initial understanding of what the issue is.
>
> This would be better as "map d%d:%lx" to indicate that it is a domid
> before the colon.
>

"d" seems hard to understand so just pdate this as "map Dom%d:%lx"

Thanks
Tiejun

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

[Andrew Cooper]

On 29/07/14 10:20, Chen, Tiejun wrote:
> On 2014/7/29 16:46, Andrew Cooper wrote:
>> On 29/07/2014 09:19, Jan Beulich wrote:
>>>>>> On 29.07.14 at 09:35, <tiejun.chen@intel.com> wrote:
>>>> On 2014/7/29 15:05, Jan Beulich wrote:
>>>>>>>> On 29.07.14 at 08:40, <tiejun.chen@intel.com> wrote:
>>>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>>> +    int ret = -EBUSY;
>>>>>> +
>>>>>> +    gfn_lock(p2m, gfn, 0);
>>>>>> +
>>>>>> +    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>>>> +
>>>>>> +    if ( mfn_valid(tmp_mfn) )
>>>>>> +    {
>>>>>> +        gdprintk(XENLOG_ERR,
>>>>>> +                 "Overlapping RMRRs at %"PRIx64".\n",
>>>>>> (paddr_t)gfn);
>>>>> Pointless cast: Just use %lx in the format string. Additionally I
>>>>> don't
>>>>> think the message text is correct: You don't really know whether
>>>>> what's there is another RMRR (or that the context you're being
>>>>> called in refers to an RMRR at all). On the contrary - if it was an
>>>>> RMRR (or to be precise, a previously established identity mapping),
>>>>> you'd want to report success. And generally we have no stop at
>>>>> the end of log messages.
>>>> So just print this,
>>>>
>>>> +        gdprintk(XENLOG_ERR,
>>>> +                 "Overlapping at %lx.\n", (paddr_t)gfn);
>>> Of course not - such a message is really meaningless.
>>>
>>> printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped
>>> to %lx\n",
>>>         d->domain_id, gfn, mfn_x(mfn));
>>>
>>> would be a message conveying all information necessary to
>>> gain initial understanding of what the issue is.
>>
>> This would be better as "map d%d:%lx" to indicate that it is a domid
>> before the colon.
>>
>
> "d" seems hard to understand so just pdate this as "map Dom%d:%lx"
>
> Thanks
> Tiejun
>

d%d is the standard notation for domains (with d%dv%u for domain/vcpus
tuples) used increasingly over the Xen codebase.

~Andrew

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

[Jan Beulich]

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

Jan

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

[Chen, Tiejun]

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.

Thanks
Tiejun

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

[Jan Beulich]

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

Jan

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

[Chen, Tiejun]

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

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

[Jan Beulich]

>>> On 29.07.14 at 13:08, <tiejun.chen@intel.com> wrote:
> #3: Others
> 
> Return with that waring message: "Cannot identity map d%d:%lx, already 
> mapped to %lx but mismatch.\n"
> 
> So what about this?

Fine, except for the " but mismatch." part of the message.

Jan