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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.11.14 at 02:35, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/11/3 19:53, Jan Beulich wrote:
>>>>> On 03.11.14 at 12:48, <tiejun.chen@xxxxxxxxx> wrote:
>>> On 2014/11/3 18:03, Jan Beulich wrote:
>>>>>>> On 03.11.14 at 10:51, <tiejun.chen@xxxxxxxxx> wrote:
>>>>> On 2014/11/3 17:00, Jan Beulich wrote:
>>>>>>>>> On 03.11.14 at 07:20, <tiejun.chen@xxxxxxxxx> 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:
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.

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

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




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel