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: <5a634c10-103e-6f3e-e51b-db26b2bc90a5@intel.com>
Date:   Tue, 17 May 2022 09:49:22 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.9.0
Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
Content-Language: en-US
To:     Fenghua Yu <fenghua.yu@intel.com>,
        Stephane Eranian <eranian@google.com>
CC:     <linux-kernel@vger.kernel.org>, <babu.moger@amd.com>,
        <x86@kernel.org>
References: <20220517001234.3137157-1-eranian@google.com>
 <YoPOWC0waMuSlvI6@fyu1.sc.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <YoPOWC0waMuSlvI6@fyu1.sc.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QVVvSWgzNDFiUWxvYlozNkJxd0xVdGpwWERNcm9xdE95OU8wamdOMlpXWXZS?=
 =?utf-8?B?bks5dHYxWEVzNFp6Nm16VCttRnlnOFhKY3E0dldxWFZ1VW1sNmc4WkhxU2lm?=
 =?utf-8?B?TEhtSjNrWEZOWDQ4YkhWcm5WSnRWK2lUcXBSMUoycHJuNm4yVmE0WXpKRUhX?=
 =?utf-8?B?c3VhTzRvbzhpOWEzWUJtL2xzbjEyZnZUU081SkxycTlNUnNjSDRTOXBEME1s?=
 =?utf-8?B?akZQblpSaXhOcFVIaFgrYy9hb3VYSmdXS083UkkvT2NaWTZUam03T2tMT0Vi?=
 =?utf-8?B?eWxmdXdtMG5YcUkzbzc4RGY5SWxkLzMyU2RYSEdqcjE3YlR4eUZKZXlUd3h2?=
 =?utf-8?B?L3VhVmZBNDJHcG5tL1FRQldsTWc0NDA1TUNQVVdqRmp6NndCQ3AxQkdpS3Uw?=
 =?utf-8?B?d3IvUDViM1BpbGZ6M0p6aXFHMHN6VWQ0RkI4NDY0d3JJdlZvRStQMDZmQWxa?=
 =?utf-8?B?TDVhbm1McWR2ZHBad1Q1WWp3SDBnZXlvTzBuOVRQWXZad0hvdWE5NVlJdVR5?=
 =?utf-8?B?WGxaYjk3KzZReEtCVmNVUDdZYWk5djRYcmJRUi9UVm0rZjFjdnMzT0lraThL?=
 =?utf-8?B?cmpZenBMK09wVE9CQWYvZWkrQ2VWUHlCUFE0V3ZFOE5JOUVYQm96RERIL2dP?=
 =?utf-8?B?QkdvK3FHRGRNTm9MVXNvalI4ZmxhSVdabVJzekY5Vkd5Qm1sd3hjRDRxSzUv?=
 =?utf-8?B?SG82TWVHd294M2szSEJqMmYyUnFtbGtONU9BWjU3aktrQ1BoTWN2MGtEWUpC?=
 =?utf-8?B?NmlpMGxHTWFiRUdsWGczSjhnalpLNlV3SjB6OFg0L2E5S09kMmdscWVMWVRT?=
 =?utf-8?B?TXZtL2I1Y3VuSnFhUGV6d3NvUXBKNXZ3R1lHVi9QbGNwNWpMTkRscXpRcmo3?=
 =?utf-8?B?YSs1ZU11S3R1L2pSSmpxSFRtMm5xK3dIM0ZnVHUwbUFaSXpFUktQVW5HTzJM?=
 =?utf-8?B?UGdsRHZzMXpjZ3BYOWdXbkhIb0xKa0EvbjFGZDhERm16WTlVSXg4RWIxa25J?=
 =?utf-8?B?dnZEM2MvemJnaTNzeXAwb1pwOUduc2pUbGtmZ3NOcm5STUpJL1grMXFIV0dQ?=
 =?utf-8?B?TjdKUHJwcmdJSEZLYjZpOTlNejMvY1IrOGw1eHY4N2JNVUV0NWc5MG5KamNZ?=
 =?utf-8?B?OGxFVk1DR2xCRWVhMTNlRHlLNWpZbmd0Sit1blpsdk5Zc2d6NjZOamRwbkYx?=
 =?utf-8?B?MkYyL1lWaFFDMnJ3NXRCaE1mbG1UQWY1VTlKNFowZUluNVdJR09DaDV1MlI0?=
 =?utf-8?B?cEFScFBGVWU4SFJtWWRKK2pmUVVicmtlNzBkR2FuZjR6RVFqYXZXZmFZTTJH?=
 =?utf-8?B?ckgvTEViN0VLa0pOVFFTYVA0eGtRUG5MaFNVdUJEMFZtNEJid3JZc3g2dy80?=
 =?utf-8?B?WisxMnF3Z0FzUEM2TEd2cVIvYzBqVGZYWkZvWGdTYVQ0amhRb3N3Nm81ODZk?=
 =?utf-8?B?TS8wd3NIelk0NThpcnE4OVdlODR6ejBGNFZEVHJQcElMcTcwVUlNRFdTSytF?=
 =?utf-8?B?eXVLZHByV2ovaG5FOWFpdVNEelNZOFZNS0Z2eFNKNlJLd0VkY2pyNFhUUDBY?=
 =?utf-8?B?amM1Yk00S2o0TWcwSU9aREdVYUpVNGtUUUh6NmtGeG1sblNCeHdyM09URmxv?=
 =?utf-8?B?bU8rTjB6bzNFZWIyV2Y3bzk4R1A3SzBvT3ZaQTh6Nk1vM0YwZmNBSXFHOUww?=
 =?utf-8?B?dmM5M2Y3WU50aWVVVmpTRnRxU0JoSFFkQWZNRmxSRTV4Vm8yMTQyQ2FjVWpk?=
 =?utf-8?B?aUVEd0RYckxlVWF4ZHR6TVlONU5XRGwwbjMvSHhCMFZ6ZGl4elJYYi94V1I1?=
 =?utf-8?B?UmwzSVQ1RFJpODVaYU42cGdCYUlxcldxbThFcWRTa0kwb1NLaDRCSlIwQU1I?=
 =?utf-8?B?aHVEdTBZTU1Gbml5Ky84anJSRGc0WlA5MFZUbExFbWtRbFJWNnI3QlJUUDBV?=
 =?utf-8?B?ODZSMUYvMnN1eldrR2tENlk4eCtUTlc3VEUyT0o5WG4xYUxCMWlyckR2U2tC?=
 =?utf-8?B?dHFpMHMxYmtoMHl5NmJBZ3NMZnlNWEtvOG0vOFUzb2xTc3h6VG5IMVpGOC9F?=
 =?utf-8?B?QmMrSTlxTE9ZZGoraHp1Q3BZSHdEcmtSSEZaTzk2bEg1RGkrczhncWxHY2Vt?=
 =?utf-8?B?c0hrVXZuRE80WmlORzRLK1VZcmEzb0c0MnNDRTRxcGgvakJLeXZvOThmTjZE?=
 =?utf-8?B?aUsrUU8zVDF1d1M5Wm1SU2JUN1UyL3g3RnJrUUkva2dKaklzekc5ek9kcDZz?=
 =?utf-8?B?WENwVWd6ZTFsOXVDVzBuU2dydXZyUzJvOFFjWkZXVHlJaE85L2szc1dQTDIy?=
 =?utf-8?B?Mm5xb3M0aDA4S1RydkRjWGlTeFJvQnFoYzc1d043YkdEOTdReWdiMEMrcEdj?=
 =?utf-8?Q?9MUww1GznUylKZxs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ceafbf5-25ff-4e18-1e18-08da3825338e
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2022 16:49:25.7805
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7+V9SLm1JKHUe45llKn6T+GjtKhGmnILhtdr1sriIs/26hV7yGvKbDBwqPI8609UvRCAX7KPcMgT9SDt0GFhG1CW8Cd2oDEre8KnJxExta4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB3935
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Fenghua,

On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> Hi, Eranian,
> 
> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> ...
>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  	first_bit = find_first_bit(&val, cbm_len);
>>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>>  
>> +	/* no need to check bits if arch supports no bits set */
>> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
>> +		goto done;
>> +
>>  	/* Are non-contiguous bitmaps allowed? */
>>  	if (!r->cache.arch_has_sparse_bitmaps &&
>>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  				    r->cache.min_cbm_bits);
>>  		return false;
>>  	}
>> -
>> +done:
>>  	*data = val;
>>  	return true;
>>  }
> 
> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> Is the following patch a better fix? I don't have AMD machine and cannot
> test the patch.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 6055d05af4cc..031d77dd982d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
>  			r->cache.arch_has_sparse_bitmaps = true;
>  			r->cache.arch_has_empty_bitmaps = true;
>  			r->cache.arch_has_per_cpu_cfg = true;
> +			r->cache.min_cbm_bits = 0;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>  			hw_res->msr_update = mba_wrmsr_amd;

That is actually what Stephane's V1 [1] did and I proposed that
he fixes it with (almost) what he has in V2 (I think the check
can be moved earlier before any bits are searched for).

The reasons why I proposed this change are:
- min_cbm_bits is a value that is exposed to user space and from the
  time AMD was supported this has always been 1 for those systems. I
  do not know how user space uses this value and unless I can be certain
  making this 0 will not affect user space I would prefer not to
  make such a change.

- this fix restores original behavior that was changed in the patch noted
  in the Fixes link.

- this fix itself relies on math on error returns of bit checking on an empty
  bitmap. I find that hides what the code does and this fix is more obvious.
  You can see this feedback in my response to V1. 

- a fix like the above snippet is incomplete. To be appropriate 
  the initialization of rdt_resources_all[] needs to be changed to
  not initialize min_cbm_bits anymore and move the platform specific bits
  to rdt_init_res_defs_amd() and rdt_init_res_defs_intel() respectively.


Reinette

[1] https://lore.kernel.org/lkml/20220516055055.2734840-1-eranian@google.com/