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/4 16:02, Jan Beulich wrote:
>>>> On 04.11.14 at 02:35, <tiejun.chen@intel.com> wrote:
>> 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,
>
> Yes!
>
>> @@ -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;
>
> But you'll need to make sure that you don't return 1 to the callers:

You're right.

> They expect 0 or negative error codes. But with the model of
> not even populating these regions (or relocating the memory
> before [at boot time] assigning a device associated with an RMRR)
> I think this needs to become an error anyway.

Looks -EINVAL might be used.

>
>> +            else if ( rc < 0 )
>> +            {
>> +                printk(XENLOG_G_WARNING
>> +                       "Dom%d can't check reserved device memory.\n",
>
> Actually, d being the subject domain, please make this more like
> "Can't check reserved device memory for Dom%d\n".

Fixed.

Thanks
Tiejun

>
> Jan
>
>> +                       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
>
>
>
>
>