From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aUCiV-00026i-K3 for mharc-grub-devel@gnu.org; Fri, 12 Feb 2016 07:20:55 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47798) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUCiT-00026Z-QJ for grub-devel@gnu.org; Fri, 12 Feb 2016 07:20:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUCiS-0006Oq-Kf for grub-devel@gnu.org; Fri, 12 Feb 2016 07:20:53 -0500 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:36353) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUCiS-0006Ok-9n for grub-devel@gnu.org; Fri, 12 Feb 2016 07:20:52 -0500 Received: by mail-wm0-x244.google.com with SMTP id 128so2373432wmz.3 for ; Fri, 12 Feb 2016 04:20:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type; bh=/iWdmumrt0f3wNa+O1YjpRQcDULG94sXgkuhRvx1wKA=; b=dYoq09UmxDyA0ZD85hLRHtPuNf33Ux31M0jlPKJDxizzUHqRaHIXu2iJ6XgSBYSk5W +ceHcrCCHqvAz4XDpkCrvMcoFzWztr4hSrSXXoImVhpkUvaKpSdrHYyNT7pQS14u/c2O w2hlQDAZfohSni9GoD0eNCHRBnu/iDXNOxaPGGvh4NqBAqR3xZE1U7jQVKe78zkWQ1ok fM8S5Oyl5cQ3ls9Uh51hanKwMaToCYShTduwuyXr2eROETIqjlv3LxN97DsiOs5kVOib xj4f5E46ntv1WNZL4arB73d9AyXo65iyfvklihFHUbS9sZV0pIJZ7kvO703mEvVibt92 eFhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type; bh=/iWdmumrt0f3wNa+O1YjpRQcDULG94sXgkuhRvx1wKA=; b=QoepTB6lCZOSqL2Ayz/s8jrIObl3NGNXy8YAMWbnstZHottyxglh1ZLaFVraBD5JDS 6NIdrGuhRixygxodV/e9bATIQ3PLo+EUkqJU/yjPkfJK5j07NU6lTdN+5LASpn61rvUJ EngjupA+VAJOjxTlqf66dSoE3q2cZdOyZ2NrJbuP7h7kgUaNC8gt4dMy17hgXfrSm0PY oNeCYm0bEbJGW0kWLDP7itQahSNmMjR7RtGEcCoxSNhT1PmW9GPxwR4VPPJtKjCCUd6j njqIvR5MlkL4IQGOwTDtOoZjPrqX8fuh57km6JLql3CxHs+rqraHSp/SFaoDxguPJt7r TxTg== X-Gm-Message-State: AG10YOTnNT9LWn4yJYFJbRUVy92vNpX7sEEkHwm6E+DTQBarN4LTN5hYWy0UbAc0TfqZsg== X-Received: by 10.28.109.150 with SMTP id b22mr2873879wmi.27.1455279651478; Fri, 12 Feb 2016 04:20:51 -0800 (PST) Received: from ?IPv6:2620:0:105f:fd00:a2a8:cdff:fe64:b3b5? ([2620:0:105f:fd00:a2a8:cdff:fe64:b3b5]) by smtp.gmail.com with ESMTPSA id g1sm7401593wmc.0.2016.02.12.04.20.49 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 12 Feb 2016 04:20:49 -0800 (PST) Subject: Re: [PATCH v2 3/6] xen: factor out allocation of page tables into separate function To: Juergen Gross , Daniel Kiper References: <1455177206-8974-1-git-send-email-jgross@suse.com> <1455177206-8974-4-git-send-email-jgross@suse.com> <20160211122732.GC3482@olila.local.net-space.pl> <56BC845F.8080705@suse.com> From: =?UTF-8?Q?Vladimir_'=cf=86-coder/phcoder'_Serbinenko?= Message-ID: <56BDCE16.6020403@gmail.com> Date: Fri, 12 Feb 2016 13:20:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.5.0 MIME-Version: 1.0 In-Reply-To: <56BC845F.8080705@suse.com> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="hhsXgg49KivfHeQDbbnno8w3s7GT50cel" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:400c:c09::244 Cc: grub-devel@gnu.org, mchang@suse.com, xen-devel@lists.xen.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Feb 2016 12:20:55 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hhsXgg49KivfHeQDbbnno8w3s7GT50cel Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11.02.2016 13:53, Juergen Gross wrote: > On 11/02/16 13:27, Daniel Kiper wrote: >> On Thu, Feb 11, 2016 at 08:53:23AM +0100, Juergen Gross wrote: >>> Do the allocation of page tables in a separate function. This will >>> allow to do the allocation at different times of the boot preparation= s >>> depending on the features the kernel is supporting. >>> >>> Signed-off-by: Juergen Gross >>> --- >>> grub-core/loader/i386/xen.c | 82 ++++++++++++++++++++++++++++-------= ---------- >>> 1 file changed, 51 insertions(+), 31 deletions(-) >>> >>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.= c >>> index e48cc3f..65cec27 100644 >>> --- a/grub-core/loader/i386/xen.c >>> +++ b/grub-core/loader/i386/xen.c >>> @@ -56,6 +56,9 @@ static struct grub_relocator_xen_state state; >>> static grub_xen_mfn_t *virt_mfn_list; >>> static struct start_info *virt_start_info; >>> static grub_xen_mfn_t console_pfn; >>> +static grub_uint64_t *virt_pgtable; >>> +static grub_uint64_t pgtbl_start; >>> +static grub_uint64_t pgtbl_end; >> >> Same as in patches #1 and #2. >=20 > Yep. >=20 >> >>> #define PAGE_SIZE 4096 >>> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_li= st)) >>> @@ -106,17 +109,17 @@ get_pgtable_size (grub_uint64_t total_pages, gr= ub_uint64_t virt_base) >>> >>> static void >>> generate_page_table (grub_uint64_t *where, grub_uint64_t paging_star= t, >>> - grub_uint64_t total_pages, grub_uint64_t virt_base, >>> - grub_xen_mfn_t *mfn_list) >>> + grub_uint64_t paging_end, grub_uint64_t total_pages, >>> + grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list) >>> { >>> if (!virt_base) >>> - total_pages++; >>> + paging_end++; >>> >>> grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS]; >>> grub_uint64_t nlx, nls, sz =3D 0; >>> int l; >>> >>> - nlx =3D total_pages; >>> + nlx =3D paging_end; >>> nls =3D virt_base >> PAGE_SHIFT; >>> for (l =3D 0; l < NUMBER_OF_LEVELS; l++) >>> { >>> @@ -160,7 +163,7 @@ generate_page_table (grub_uint64_t *where, grub_u= int64_t paging_start, >>> if (pr) >>> pg +=3D POINTERS_PER_PAGE; >>> >>> - for (j =3D 0; j < total_pages; j++) >>> + for (j =3D 0; j < paging_end; j++) >>> { >>> if (j >=3D paging_start && j < lp) >>> pg[j + lxs[0]] =3D page2offset (mfn_list[j]) | 5; >>> @@ -261,24 +264,12 @@ grub_xen_special_alloc (void) >>> } >>> >>> static grub_err_t >>> -grub_xen_boot (void) >>> +grub_xen_pt_alloc (void) >>> { >>> grub_relocator_chunk_t ch; >>> grub_err_t err; >>> grub_uint64_t nr_info_pages; >>> grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; >>> - struct gnttab_set_version gnttab_setver; >>> - grub_size_t i; >>> - >>> - if (grub_xen_n_allocated_shared_pages) >>> - return grub_error (GRUB_ERR_BUG, "active grants"); >>> - >>> - err =3D grub_xen_p2m_alloc (); >>> - if (err) >>> - return err; >>> - err =3D grub_xen_special_alloc (); >>> - if (err) >>> - return err; >>> >>> next_start.pt_base =3D max_addr + xen_inf.virt_base; >>> state.paging_start =3D max_addr >> PAGE_SHIFT; >>> @@ -298,30 +289,59 @@ grub_xen_boot (void) >>> nr_pages =3D nr_need_pages; >>> } >>> >>> - grub_dprintf ("xen", "bootstrap domain %llx+%llx\n", >>> - (unsigned long long) xen_inf.virt_base, >>> - (unsigned long long) page2offset (nr_pages)); >>> - >>> err =3D grub_relocator_alloc_chunk_addr (relocator, &ch, >>> max_addr, page2offset (nr_pt_pages)); >>> if (err) >>> return err; >>> >>> + virt_pgtable =3D get_virtual_current_address (ch); >>> + pgtbl_start =3D max_addr >> PAGE_SHIFT; >>> + max_addr +=3D page2offset (nr_pt_pages); >>> + state.stack =3D max_addr + STACK_SIZE + xen_inf.virt_base; >>> + state.paging_size =3D nr_pt_pages; >>> + next_start.nr_pt_frames =3D nr_pt_pages; >>> + max_addr =3D page2offset (nr_pages); >>> + pgtbl_end =3D nr_pages; >>> + >>> + return GRUB_ERR_NONE; >>> +} >>> + >>> +static grub_err_t >>> +grub_xen_boot (void) >>> +{ >>> + grub_err_t err; >>> + grub_uint64_t nr_pages; >>> + struct gnttab_set_version gnttab_setver; >>> + grub_size_t i; >>> + >>> + if (grub_xen_n_allocated_shared_pages) >>> + return grub_error (GRUB_ERR_BUG, "active grants"); >>> + >>> + err =3D grub_xen_p2m_alloc (); >>> + if (err) >>> + return err; >>> + err =3D grub_xen_special_alloc (); >>> + if (err) >>> + return err; >>> + err =3D grub_xen_pt_alloc (); >>> + if (err) >>> + return err; >> >> Should not we free memory allocated by grub_xen_p2m_alloc() and >> grub_xen_special_alloc() if grub_xen_pt_alloc() failed? >=20 > Hmm, why? If I don't miss anything freeing memory in case of error isn'= t > done anywhere (at least not in this source file). >=20 If we don't it's a bug and not an excuse to continue doing bad things > Juergen >=20 > . >=20 --hhsXgg49KivfHeQDbbnno8w3s7GT50cel Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iF4EAREKAAYFAla9ziEACgkQmBXlbbo5nOv5rAEAmZt34iLGMe9rhQ5p2l4Nbgcg vojQ6gSCIbBDy7ltJrsA/3LtysNY50w9Df9OaKbIUEiCPB6A4RlmXB+45mrJS4BJ =fvif -----END PGP SIGNATURE----- --hhsXgg49KivfHeQDbbnno8w3s7GT50cel--