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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/10/24 23:11, Jan Beulich wrote:
> > > > On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> 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

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