[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 15:05, Jan Beulich wrote: On 29.07.14 at 08:40, <tiejun.chen@xxxxxxxxx> wrote:+int set_identity_p2m_entry(struct domain *d, unsigned long gfn) +{ + p2m_type_t p2mt; + p2m_access_t a; + mfn_t tmp_mfn, mfn = _mfn(gfn);No need for the mfn variable; instead what currently is tmp_mfn should be named just mfn, and the _mfn(gfn) construction can be done right in the function call. Okay. + 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); + 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, int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn) { int rc = -EINVAL; mfn_t mfn; p2m_access_t a; p2m_type_t t; struct p2m_domain *p2m = p2m_get_hostp2m(d); if ( !paging_mode_translate(d) ) return -EIO; gfn_lock(p2m, gfn, 0); mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);/* Do not use mfn_valid() here as it will usually fail for MMIO pages. */ if ( (INVALID_MFN == mfn_x(mfn)) || (t != p2m_mmio_direct) ) { gdprintk(XENLOG_ERR, "gfn_to_mfn failed! gfn=%08lx type:%d\n", gfn, t); goto out; }rc = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K, p2m_invalid, p2m->default_access); out: gfn_unlock(p2m, gfn, 0); return rc; }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. So I have to try following existing codes. Now I'm curious what we should abide. Thanks Tiejun _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |