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/11/3 19:53, Jan Beulich wrote:
>>>> On 03.11.14 at 12:48, <tiejun.chen@intel.com> wrote:
>> On 2014/11/3 18:03, Jan Beulich wrote:
>>>>>> On 03.11.14 at 10:51, <tiejun.chen@intel.com> wrote:
>>>> On 2014/11/3 17:00, Jan Beulich wrote:
>>>>>>>> On 03.11.14 at 07:20, <tiejun.chen@intel.com> 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 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;
+        a =  p2m->default_access;
+        if ( !is_hardware_domain(d) )
+        {
+            rc = 
iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
+                                                  &gfn);
+            /* We always avoid populating reserved device memory. */
+            if ( rc == 1 )
+                goto out;
+            else if ( rc < 0 )
+            {
+                printk(XENLOG_G_WARNING
+                       "Dom%d can't check reserved device memory.\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. */

Thanks
Tiejun