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/11/3 17:00, Jan Beulich wrote:
> > > > On 03.11.14 at 07:20, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2014/10/31 16:25, Jan Beulich wrote:
> > > > > > On 31.10.14 at 03:50, <tiejun.chen@xxxxxxxxx> wrote:
> > > > On 2014/10/30 17:24, Jan Beulich wrote:
> > > > > > > > On 30.10.14 at 08:39, <tiejun.chen@xxxxxxxxx> 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.
Fixed.

> > #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?
Thanks
Tiejun

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