From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata Date: Fri, 30 Sep 2016 15:46:48 +0100 Message-ID: References: <1475157221-21828-1-git-send-email-igor.druzhinin@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------CCC523E3445DEBC7FB7FF60C" Return-path: In-Reply-To: <1475157221-21828-1-git-send-email-igor.druzhinin@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Igor Druzhinin , xen-devel@lists.xen.org Cc: george.dunlap@eu.citrix.com, Andrew Cooper , Olaf Hering , Jan Beulich List-Id: xen-devel@lists.xenproject.org --------------CCC523E3445DEBC7FB7FF60C Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit On 29/09/16 14:53, Igor Druzhinin wrote: > As long as t_info_first_offset is calculated in uint32_t offsets we need to > multiply it by sizeof(uint32_t) in order to get the right number of pages > for trace metadata. Not doing that makes it impossible to read the trace > buffer correctly from userspace for some corner cases. > > Signed-off-by: Igor Druzhinin Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace: correct formula to calculate t_info_pages". But that one was presumably written (and Acked by me) because the variable name there, t_info_first_offset, is confusing. The other mistake in fbf96e6 is that before t_info_words was actually denominated in words; but after it's denominated in bytes (which is again confusing). What about something like the attached instead? This should fix your problem while making the code clearer. -George --------------CCC523E3445DEBC7FB7FF60C Content-Type: text/x-diff; name="0001-xen-trace-Fix-trace-metadata-page-count-calculation-.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename*0="0001-xen-trace-Fix-trace-metadata-page-count-calculation-.pa"; filename*1="tch" =46rom b0252cec4a96fea28faa85c47ab0f5d5997444ad Mon Sep 17 00:00:00 2001 From: George Dunlap Date: Fri, 30 Sep 2016 15:42:56 +0100 Subject: [PATCH] xen/trace: Fix trace metadata page count calculation (re= vert fbf96e6) Changeset fbf96e6, "xentrace: correct formula to calculate t_info_pages", broke the trace metadata page count calculation, by mistaking t_info_first_offset as denominated in bytes, when in fact it is denominated in words (uint32_t). Effectively revert that change, and put a comment there to reduce the chance that someone will make that mistake in the future. Spotted-by: Igor Druzhinin Signed-off-by: George Dunlap --- CC: Olaf Hering CC: Andrew Cooper CC: Jan Beulich --- xen/common/trace.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/xen/common/trace.c b/xen/common/trace.c index f651cf3..2f4ecca 100644 --- a/xen/common/trace.c +++ b/xen/common/trace.c @@ -148,8 +148,12 @@ static int calculate_tbuf_size(unsigned int pages, u= int16_t t_info_first_offset) pages =3D max_pages; } =20 - t_info_words =3D num_online_cpus() * pages * sizeof(uint32_t); - t_info_pages =3D PFN_UP(t_info_first_offset + t_info_words); + /*=20 + * NB this calculation is correct, because t_info_first_offset is + * in words, not bytes, not bytes + */ + t_info_words =3D num_online_cpus() * pages + t_info_first_offset; + t_info_pages =3D PFN_UP(t_info_words * sizeof(uint32_t)); printk(XENLOG_INFO "xentrace: requesting %u t_info pages " "for %u trace pages on %u cpus\n", t_info_pages, pages, num_online_cpus()); --=20 2.1.4 --------------CCC523E3445DEBC7FB7FF60C Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --------------CCC523E3445DEBC7FB7FF60C--