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/24 23:11, Jan Beulich wrote:
>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -686,6 +686,30 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>>       /* Now, actually do the two-way mapping */
>>       if ( mfn_valid(_mfn(mfn)) )
>>       {
>> +
>> +        if ( !is_hardware_domain(d) )
>> +        {
>> +            rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
>> +                                                  &gfn);
>
> Okay, no I see what that function is needed for. It being an inline
> function is of course very questionable looking at this use site.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -556,6 +556,17 @@ guest_physmap_remove_page(struct domain *d, 
unsigned long gfn,
      gfn_unlock(p2m, gfn, page_order);
  }

+/* Check if we are accessing rdm. */
+int p2m_check_reserved_device_memory(xen_pfn_t start,
+                                     xen_ulong_t nr,
+                                     void *d)
+{
+    unsigned long *gfn = d;
+    xen_pfn_t end = start + nr;
+
+   return ( *gfn >= start && *gfn <= end ) ? 1 : 0;
+}
+
  int
  guest_physmap_add_entry(struct domain *d, unsigned long gfn,
                          unsigned long mfn, unsigned int page_order,

>
>> +            if ( rc )
>
> And the return value from the called function is of type int -
> non-zero may not just mean "true" but (when negative) also
> "error". You need to distinguish these cases.

But in our case its impossible to get a negative value.

>
>> +            {
>> +                /*
>> +                 * 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.

>
>> +                {
>> +                    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);

Thanks
Tiejun