Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <60bf1aa7-b004-0ea7-7efc-37b4a1ea2461@intel.com>
Date:   Mon, 28 Mar 2022 13:30:01 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 09/21] x86/virt/tdx: Get information about TDX module
 and convertible memory
Content-Language: en-US
To:     Isaku Yamahata <isaku.yamahata@gmail.com>,
        Kai Huang <kai.huang@intel.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        seanjc@google.com, pbonzini@redhat.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, peterz@infradead.org,
        tony.luck@intel.com, ak@linux.intel.com, dan.j.williams@intel.com,
        isaku.yamahata@intel.com
References: <cover.1647167475.git.kai.huang@intel.com>
 <98c1010509aa412e7f05b12187cacf40451d5246.1647167475.git.kai.huang@intel.com>
 <20220324174301.GA1212881@ls.amr.corp.intel.com>
 <f211441a6d23321e22517684159e2c28c8492b86.camel@intel.com>
 <20220328202225.GA1525925@ls.amr.corp.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20220328202225.GA1525925@ls.amr.corp.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/28/22 13:22, Isaku Yamahata wrote:
>>>> +	/*
>>>> +	 * Also a sane BIOS should never generate invalid CMR(s) between
>>>> +	 * two valid CMRs.  Sanity check this and simply return error in
>>>> +	 * this case.
>>>> +	 */
>>>> +	for (j = i; j < cmr_num; j++)
>>>> +		if (cmr_valid(&cmr_array[j])) {
>>>> +			pr_err("Firmware bug: invalid CMR(s) among valid CMRs.\n");
>>>> +			return -EFAULT;
>>>> +		}
>>> This check doesn't make sense because above i-for loop has break.
>> The break in above i-for loop will hit at the first invalid CMR entry.  Yes "j =
>> i" will make double check on this invalid CMR entry, but it should have no
>> problem.  Or we can change to "j = i + 1" to skip the first invalid CMR entry.
>>
>> Does this make sense?
> It makes sense. Somehow I missed j = i. I scratch my review.

You can also take it as something you might want to refactor, add
comments, or work on better variable names.  If it confused one person,
it will confuse more in the future.