From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.9904.1612453879764871281 for ; Thu, 04 Feb 2021 07:51:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=F8EmyTwr; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1612453879; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=03AiQcGrcP7Os/09cXs1lS8Y3+IiIkBEA1Z8uw1gDyY=; b=F8EmyTwrcLW4qgezPaBTluA+W+ounc8cYnjnAqq7+MAIlO4rJEdW+PZaaxOvFkV4btSktd r+IdNllxcr2J6Y3fJqRCwoGFPpkoZnqdPIJNK9UV3rQUXwrwCCN9sL1XwvHB/v7Omj4Gsj xIQ1ZZjWuVeK9yULC5XMa/Wh3SzJlFE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-598-JcYaeg3XOdWIm2qaSEWa2w-1; Thu, 04 Feb 2021 10:51:16 -0500 X-MC-Unique: JcYaeg3XOdWIm2qaSEWa2w-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 83195801962; Thu, 4 Feb 2021 15:51:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-186.ams2.redhat.com [10.36.114.186]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5B8185DEF9; Thu, 4 Feb 2021 15:51:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset To: "Ni, Ray" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Kumar, Rahul1" References: <20210204025921.1428-1-ray.ni@intel.com> <20210204025921.1428-2-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 4 Feb 2021 16:51:12 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/04/21 15:27, Ni, Ray wrote: >>> >>> (1) please align the "res*" on the other lines with this >>> >>> (in the X64 file too) >>> > > OK. > >>>> + mov si, MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart >>> >>> (2) please introduce a macro for this; in my opinion, with the currently >>> proposed change, the code is *harder* to read and modify than before. >>> Now we have a lot of fluff to spell out, for every single field reference. > > I want to use the struc instead of original hardcode offset because > I have headache when removing the Lock field from the C structure. > All the hardcode value should be changed carefully. > Using struc, I can simply remove that field Lock from the struc. Yes, absolutely. > > I originally tried to supply the second parameter to struc for the initial offset > following https://www.nasm.us/doc/nasmdoc5.html#section-5.9.1 > struc MP_CPU_EXCHANGE_INFO (SwitchToRealProcEnd - RendezvousFunnelProcStart) > > So that > mov si, MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart > can be: > mov si, MP_CPU_EXCHANGE_INFO.BufferStart > But somehow NASM compiler doesn't like it. Right, but that's not what I was trying to suggest. Instead, I'm suggesting a very simple macro like FIELD_OFS (BufferStart) that expands to MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart That's all, really. > >> >> (3) I have a further request / suggestion: >> >> (3a) We should extend the following files: >> >> MdePkg/Include/Ia32/Nasm.inc >> MdePkg/Include/X64/Nasm.inc >> >> with a macro that maps UINTN to "resd" versus "resq", as appropriate, > > I am not an expert of NASM or ASM. > Do you mean to use %define as below in Ia32/Nasm.inc? > %define CTYPE_UINTN resd 1 > %define CTYPE_UINT32 resd 1 > %define CTYPE_UINT64 resq 1 > %define CTYPE_UINT8 resb 1 > %define CTYPE_BOOLEAN resb 1 > %define CTYPE_UINT16 resw 1 > > And define below in X64/Nasm.inc? > %define CTYPE_UINTN resq 1 > %define CTYPE_UINT32 resd 1 > %define CTYPE_UINT64 resq 1 > %define CTYPE_UINT8 resb 1 > %define CTYPE_BOOLEAN resb 1 > %define CTYPE_UINT16 resw 1 > > So, the struc definition will be as below? > .StackStart: CTYPE_UINTN > > I intend to use CTYPE_xxx as prefix because simply using UINTN may cause > people think that UINTN is the C keyword UINTN. > Using CTYPE_UINTN so people can at least search this string to understand > the magic. > > Anyway, I just want to use a different name other than UINTN. > Do you agree? Any suggestions on the name? Yes, this is totally what I meant -- I didn't even intend to ask for CTYPE_UINT32 and friends, given that they directly translate to "resd 1" and similar. The only variable size type is UINTN, so I only asked for CTYPE_UINTN. But if you can add all the CTYPE_* macros, that's best! >> (3b) we should reserve space for the IA32_DESCRIPTOR fields in terms of >> UINT16 + UINTN -- you can use a separate "struc IA32_DESCRIPTOR" for >> this that uses the UINTN trick from step (3a) > > Do you mean this? > > struc IA32_DESCRIPTOR > .Limit CTYPE_UINT16 > .Base CTYPE_UINTN > endstruc > > struc MP_CPU_EXCHANGE_INFO > ... > .IdtrProfile: resb IA32_DESCRIPTOR_size More or less, yes. If it's possible to embed IA32_DESCRIPTOR into MP_CPU_EXCHANGE_INFO somehow, so that we could even refer to the individual fields in it (if necessary), that would be even nicer. But it's not really a requirement -- the above should work OK too. >> (3c) use a common definition for "struc MP_CPU_EXCHANGE_INFO", hiding >> the UINTN and IA32_DESCRIPTOR size differences through the above steps. > > I think it's doable. > Thank you! Laszlo