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

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

On 2014/10/28 18:12, Jan Beulich wrote:
>>>> On 28.10.14 at 09:26, <tiejun.chen@intel.com> wrote:
>> On 2014/10/27 18:33, Jan Beulich wrote:
>>>>>> On 27.10.14 at 10:05, <tiejun.chen@intel.com> wrote:
>>>> On 2014/10/24 23:11, Jan Beulich wrote:
>>>>>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> wrote:
>>>>>> +            {
>>>>>> +                /*
>>>>>> +                 * Just set p2m_access_n in case of shared-ept
>>>>>> +                 * or non-shared ept but 1:1 mapping.
>>>>>> +                 */
>>>>>> +                if ( iommu_use_hap_pt(d) ||
>>>>>> +                     (!iommu_use_hap_pt(d) && mfn == gfn) )
>>>>>
>>>>> How would, other than by chance, mfn equal gfn here? Also the
>>>>> double use of iommu_use_hap_pt(d) is pointless here.
>>>>
>>>> There are two scenarios we should concern:
>>>>
>>>> #1 in case of shared-ept.
>>>>
>>>> We always need to check so iommu_use_hap_pt(d) is good.
>>>>
>>>> #2 in case of non-sharepd-ept
>>>>
>>>> If mfn != gfn I think guest don't access RMRR range, so its allowed.
>>>
>>> And what if subsequently a device needing a 1:1 mapping at this GFN
>>> gets assigned? (I simply don't see why shared vs non-shared would
>>> matter here.)
>>
>> In case of non-shared ept we just create VT-d table, if we assign a
>> device with 1:1 RMRR mapping. So as long as mfn != gfn, its not
>> necessary to set p2m_access_n.
>
> I think it was mentioned before (and not just by me) that it's at least
> risky to allow the two page tables to get out of sync wrt the
> translations they do (as opposed to permissions they set).

Okay I will remove all condition check here.

>
>>>>>> +                {
>>>>>> +                    rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
>>>>>> +                                       p2m_access_n);
>>>>>> +                    if ( rc )
>>>>>> +                        gdprintk(XENLOG_WARNING, "set rdm p2m failed: (%#lx)\n",
>>>>>> +                                 gfn);
>>>>>
>>>>> Such messages are (due to acting on a foreign domain) relatively
>>>>> useless without also logging the domain that is affected. Conversely,
>>>>> logging the current domain and vCPU (due to using gdprintk()) is
>>>>> rather pointless. Also please drop either the colon or the
>>>>> parentheses in the message.
>>>>
>>>> Can P2M_DEBUG work here?
>>>>
>>>> P2M_DEBUG("set rdm p2m failed: %#lx\n", gfn);
>>>
>>> I don't think this would magically add the missing information. Plus it
>>
>> Sorry, is it okay?
>>
>> gdprintk(XENLOG_WARNING, "Domain %hu set rdm p2m failed: %#lx\n",
>>                                    d->domain_id, gfn);
>
> Almost. Use printk() instead of gdprintk(), and %d instead of %hu.
>
>>> would limit output to the !NDEBUG case, putting the practical
>>> usefulness of this under question even more.
>>>
>>> But anyway, looking at the existing code again, I think you'd be better
>>> off falling through to the p2m_set_entry() that's already there, just
>>
>> Are you saying I should do this in p2m_set_entry()?
>
> No. I said that I'd prefer you to use the _existing call to_
> p2m_set_entry().
>
>>> altering the access permission value you pass. Less code, better
>>
>> But here, guest_physmap_add_entry() just initiate to set p2m_access_n.
>> We will alter the access permission until if we really assign device to
>> create a 1:1 mapping.
>
> And I didn't question that. All I said is to use the existing call by simply
> replacing the unconditional use of the default access value with one
> conditionally set to p2m_access_n.

Okay,

@@ -686,8 +686,19 @@ 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;
+        if ( !is_hardware_domain(d) )
+        {
+            rc = 
iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
+                                                  &gfn);
+            if ( rc < 0 )
+                printk("Domain %d can't can't check reserved device 
memory.\n",
+                       d->domain_id);
+        }
+
+        /* We need to set reserved device memory as p2m_access_n. */
+        a =  ( rc == 1 ) ? p2m_access_n : p2m->default_access;
+        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a);
          if ( rc )
              goto out; /* Failed to update p2m, bail without updating 
m2p. */



Thanks
Tiejun

>
> Jan
>
>