[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v5][PATCH 1/2] xen:x86:mm:p2m: introduce set_identity_p2m_entry



On 2014/7/29 16:19, Jan Beulich wrote:
On 29.07.14 at 09:35, <tiejun.chen@xxxxxxxxx> wrote:
On 2014/7/29 15:05, Jan Beulich wrote:
On 29.07.14 at 08:40, <tiejun.chen@xxxxxxxxx> wrote:
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret = -EBUSY;
+
+    gfn_lock(p2m, gfn, 0);
+
+    tmp_mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
+
+    if ( mfn_valid(tmp_mfn) )
+    {
+        gdprintk(XENLOG_ERR,
+                 "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)gfn);

Pointless cast: Just use %lx in the format string. Additionally I don't
think the message text is correct: You don't really know whether
what's there is another RMRR (or that the context you're being
called in refers to an RMRR at all). On the contrary - if it was an
RMRR (or to be precise, a previously established identity mapping),
you'd want to report success. And generally we have no stop at
the end of log messages.

So just print this,

+        gdprintk(XENLOG_ERR,
+                 "Overlapping at %lx.\n", (paddr_t)gfn);

Of course not - such a message is really meaningless.

printk(XENLOG_G_WARNING "Cannot identity map %d:%lx, already mapped to %lx\n",
        d->domain_id, gfn, mfn_x(mfn));


Thanks your further help.

would be a message conveying all information necessary to
gain initial understanding of what the issue is.


+        goto out;

Once again, when error handling is that simple please avoid using
"goto".


Its make no sense to me.

Did you see this function in this same file,

Referring to existing bad examples is never going to help.

But sometimes someone who are not familiar with this case may have no choice because something is really tricky.


Yes, previously I really can't understand what's that code style in xen.
So as I remember I ask you guy if xen has checkpatch.pl like Linux, qemu
or other stuff, but you didn't reply this point.

We just have none, and for the specific case of using or not using
"goto" it wouldn't help you anyway.

So I have to try
following existing codes. Now I'm curious what we should abide.

Where is the problem with just writing

I doesn't mean I have a problem to write such codes but I think we need an unified coding style.

Anyway I can do as you show.


int k(struct domain *d, unsigned long gfn)
{
     p2m_type_t p2mt;
     p2m_access_t a;
     mfn_t mfn;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int ret;

     gfn_lock(p2m, gfn, 0);

     mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
     if ( !mfn_valid(mfn) )
         ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, 
p2m_mmio_direct,
                             p2m_access_rw);
     else if ( mfn_x(mfn) == gfn )
        ret = 0;
     else
     {
         printk(XENLOG_G_WARNING
                "Cannot identity map %d:%lx, already mapped to %lx\n",
                d->domain_id, gfn, mfn_x(mfn));
         ret = -EBUSY;
     }

     gfn_unlock(p2m, gfn, 0);

     return ret;
}

? Of course it may still be necessary to also inspect the obtained p2mt
and a.


Are you saying this?

        if ( !p2m_is_valid(p2mt) ||
             !mfn_valid(mfn) ||
             (a != p2m_access_rw) )


Thanks
Tiejun

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.