Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 07:58:14 -0300
From:   Wander Lairson Costa <wander@redhat.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
        Sean Christopherson <seanjc@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Joerg Roedel <jroedel@suse.de>,
        Ard Biesheuvel <ardb@kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Ingo Molnar <mingo@redhat.com>,
        Varad Gautam <varad.gautam@suse.com>,
        Dario Faggioli <dfaggioli@suse.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Mike Rapoport <rppt@kernel.org>,
        David Hildenbrand <david@redhat.com>, x86@kernel.org,
        linux-mm@kvack.org, linux-coco@lists.linux.dev,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv5 04/12] x86/boot: Add infrastructure required for
 unaccepted memory support
Message-ID: <YmvExncBpvDdfTjd@fedora>
References: <20220425033934.68551-1-kirill.shutemov@linux.intel.com>
 <20220425033934.68551-5-kirill.shutemov@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220425033934.68551-5-kirill.shutemov@linux.intel.com>
Precedence: bulk

On Mon, Apr 25, 2022 at 06:39:26AM +0300, Kirill A. Shutemov wrote:

[snip]

>  
> +static __always_inline void __set_bit(long nr, volatile unsigned long *addr)

Can't we update the existing set_bit function?

> +{
> +	asm volatile(__ASM_SIZE(bts) " %1,%0" : : "m" (*(volatile long *) addr),

Why do we need the cast here?

> +		     "Ir" (nr) : "memory");

Shouldn't we add "cc" to the clobber list?

> +}
> +
> +static __always_inline void __clear_bit(long nr, volatile unsigned long *addr)
> +{
> +	asm volatile(__ASM_SIZE(btr) " %1,%0" : : "m" (*(volatile long *) addr),
> +		     "Ir" (nr) : "memory");
> +}

Same comments of __set_bit apply here (except there is no clear_bit function)

[snip]

> +
> +static __always_inline unsigned long swab(const unsigned long y)
> +{
> +#if __BITS_PER_LONG == 64
> +	return __builtin_bswap32(y);
> +#else /* __BITS_PER_LONG == 32 */
> +	return __builtin_bswap64(y);

Suppose y = 0x11223344UL, then, the compiler to cast it to a 64 bits
value yielding 0x0000000011223344ULL, __builtin_bswap64 will then
return 0x4433221100000000, and the return value will be casted back
to 32 bits, so swapb will always return 0, won't it?

> +#endif
> +}
> +
> +unsigned long _find_next_bit(const unsigned long *addr1,
> +		const unsigned long *addr2, unsigned long nbits,

The addr2 name seems a bit misleading, it seems to act as some kind of mask,
is that right?

> +		unsigned long start, unsigned long invert, unsigned long le)
> +{
> +	unsigned long tmp, mask;
> +
> +	if (unlikely(start >= nbits))
> +		return nbits;
> +
> +	tmp = addr1[start / BITS_PER_LONG];
> +	if (addr2)
> +		tmp &= addr2[start / BITS_PER_LONG];
> +	tmp ^= invert;
> +
> +	/* Handle 1st word. */
> +	mask = BITMAP_FIRST_WORD_MASK(start);
> +	if (le)
> +		mask = swab(mask);
> +
> +	tmp &= mask;
> +
> +	start = round_down(start, BITS_PER_LONG);
> +
> +	while (!tmp) {
> +		start += BITS_PER_LONG;
> +		if (start >= nbits)
> +			return nbits;
> +
> +		tmp = addr1[start / BITS_PER_LONG];
> +		if (addr2)
> +			tmp &= addr2[start / BITS_PER_LONG];
> +		tmp ^= invert;
> +	}

Isn't better to divide start by BITS_PER_LONG in the beginning of the fuction,
and then multiply it by BITS_PER_LONG when necessary, saving the division operations
in the while loop?

[snip]