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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.11.14 at 07:20, <tiejun.chen@intel.com> wrote:
> On 2014/10/31 16:25, Jan Beulich wrote:
>>>>> On 31.10.14 at 03:50, <tiejun.chen@intel.com> wrote:
>>> On 2014/10/30 17:24, Jan Beulich wrote:
>>>>>>> On 30.10.14 at 08:39, <tiejun.chen@intel.com> wrote:
>>>>> @@ -686,8 +686,22 @@ 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 need to set reserved device memory as p2m_access_n. */
>>>>> +            if ( rc == 1 )
>>>>> +                a = p2m_access_n;
>>>>> +            else if ( rc < 0 )
>>>>> +                printk(XENLOG_WARNING
>>>>> +                       "Domain %d can't check reserved device memory.\n",
>>>>> +                       d->domain_id);
>>>>> +        }
>>>>> +
>>>>> +        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a);
>>>>>             if ( rc )
>>>>>                 goto out; /* Failed to update p2m, bail without updating m2p.
>>> */
>>>>
>>>> The handling of "a" looks good now, but the error handling and
>>>> logging is still as broken as it was before.
>>>
>>> Do you mean I'm missing some necessary info? Like gfn and mfn, so domain
>>> id, gfn and mfn can show enough message.
>>>
>>> Sorry I'm poor to understand what you expect.
>>
>> But I explained it already, and that explanation is still visible in
>> the quotes above. But to avoid any doubt, I'll repeat: "And
> 
> I tried to understand what you said but felt a confusion so ask if you 
> show me directly.
> 
>> properly handle the error case (just logging a message - which
>> btw lacks a proper XENLOG_G_* prefix - doesn't seem enough
>> to me)."
> 
> Looks there are two problems:
> 
> #1: the error message
> 
> If current line is not fine,
> 	printk(XENLOG_G_WARNING "Domain %d can't check reserved device 
> memory.\n", d->domain_id);
> 
> I mean could you change this directly.

This looks reasonable, albeit we generally prefer Dom%d or dom%d
so that messages are somewhat grep-able.

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

Jan