[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |