Received-SPF: pass (google.com: domain of linux-kernel+bounces-87626-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <7fcfc226-0263-0364-bed7-fc95e6c945cb@oracle.com>
Date: Thu, 29 Feb 2024 14:13:49 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva
To: "Huang, Kai" <kai.huang@intel.com>, kvm@vger.kernel.org
Cc: pbonzini@redhat.com, linux-kernel@vger.kernel.org
References: <20240229212522.34515-1-dongli.zhang@oracle.com>
 <04398f4e-6098-4559-9604-b9810753801e@intel.com>
Content-Language: en-US
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <04398f4e-6098-4559-9604-b9810753801e@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?UTMrZ2xiNjFYMG1HTWJwZjJKNFNCWjdNVXdNcUV3TWJ0ZFhZMGlDdW4xeEJ6?=
 =?utf-8?B?Q2dMK3czcHJqT3Z0Z3FUem5hTys0ZXdZL01YcmVxOEZIQlJxd01Sb3N2WUd4?=
 =?utf-8?B?aFpsWGlXME50RCszaEFlK2tyaXE1NWFDQmtyQlJjbTJRSkFLRmRSY3o4SEhG?=
 =?utf-8?B?WVNoWWx2UnpLZzFhZTNSUElhU1VOdWRKeVVzWXU2dExFeWNGUUs0bDlRQkwv?=
 =?utf-8?B?dFdXSXhLcFgrYXEyc2ZwYlVsZ3F6SHZDa3h2TVJITzdkekg4c0VNc1JtUXVi?=
 =?utf-8?B?clRyTGFEcUZQWFVSU05HQ2gvNGs5OFhrL0lFa2lKRTRReGkxVjFZbzFPdCtP?=
 =?utf-8?B?RlpWYmVLclg5MEJQV3M2Uk42eTJXRGR0dnRFcDNoU29nYS9icmtYUEIwL2Zx?=
 =?utf-8?B?TVR6a0w5cW44K1pBVnE2bFE1WFlRaVZlN1hleHV2SGdvdmdvMTM3b3BMTnlP?=
 =?utf-8?B?dVkyL1lwODFKNGZtcTJzemRPMGlHZUdBQUUrUXpXanhkR2pDdHVGTm1ncG1t?=
 =?utf-8?B?SUVjTDZ3MmJad2RsM1BhcnVRZkxVZ3JmVGZpeWxCS1kwT2lFMnA4YUdpdXRa?=
 =?utf-8?B?OWlBcmdEVG9WZ0pTZHpRWmtTSmZqdGo3ajRuZkJYSEU1aEpLRTV5eWhZczdv?=
 =?utf-8?B?QVJlYTNoTi95YVVpTENMWlE2cFQ0UlU1dC9KOWEya2VLK2wxVXp0eTlld1pp?=
 =?utf-8?B?c3V1NzYzVDBtRzBhb2kwWlZWMDJzNk0rTlBGYU9xQkhiQXBmNVB0UW4xcmVE?=
 =?utf-8?B?QmpWaFpwZXdlUFEyWDlSamFOcjZjRkNLNlJ1YVJyeVY4enBTVlNseEN0dzlF?=
 =?utf-8?B?cVhTc1F2dVVRWE44QkFpME9qZFRVNlZ2b0lEMlJiM0FWeGphbTJxNklMZDJl?=
 =?utf-8?B?d2pYQ2F2cngvdExRa0JFODJJSTRmaWUrQy9jSUJMbG5aSmtxanpVY29LTEJ0?=
 =?utf-8?B?VFVzd2doWElKOWlXUjBaZ0lrUFVYYzFQOXZyblRPNEZHdTBWNmVHRWxWUndV?=
 =?utf-8?B?ZmVzRnBucGQyK1NNQ29DSWFaZ0duSVUzc3E1SjdwaTdqNHVtcGQ3eFRGOG5z?=
 =?utf-8?B?WGl0QUQyUDFOZXhOMGtXVXZDZlNjVjlmM1NGWW1yYTF1NjdqTmUzN0NDT0hl?=
 =?utf-8?B?akJjSzJYdy9taE5jUFNMV1ptV0UrYkZEMExCQTVpRW9kQ1BzZkJqbGFqWmpp?=
 =?utf-8?B?V0hUL2dnL20zd2lnTTZhNys4WGpSbFVRdm1aZVAwY0hPdzA5Y3luMTJZYVFB?=
 =?utf-8?B?UVVBYjU5UjNxTjlrb05XQ1JSK3VXYUxlNlR6c3cwMW0vS1FjbnVSMEM0bDda?=
 =?utf-8?B?Tm9DOVYvNllRMUNvTWNnUzhYckNrSVAvU3d2N20vY01OOUNIcDVpck1jcXJH?=
 =?utf-8?B?UXAzM0pHaHhZZ3VWN2V0LzhZZmxUV2c0WTZFMStIMmZUVFpUNVNOSUJCbWFz?=
 =?utf-8?B?ZU5sOHpJQzlIWXpxREh6MjlnbTFtcUFqSmYrN0YzM1NCeU1oa2w3akNEMC9q?=
 =?utf-8?B?Z0ltSW84RVd1WmMrM0pMVnkxQ3VJVzVtK0gwUjFtUzdiM2ZXOUFENTZRS2Zj?=
 =?utf-8?B?RzZ6a29RSmU1dlpsb3RMa3hTOWx1YitjSFB6eUFvUmh5emMxblFLN2FWWGNy?=
 =?utf-8?B?U242azhSZmdqQk84WU9tdWJBcFNCK1A2eUJOZ1JJMmJVUTBtMDlRdlQ5QlYx?=
 =?utf-8?B?RXVuQzUrL1c1VllGZjlPVFcrSWhoVGpxRm9zSTdXUzNyQURwbXBlSEFFZXA5?=
 =?utf-8?B?bkV0TDdaVUFlRTg2M1RPNkM2MUJ0NnZuU2IybXFqaldhWkt4M2c0N2xETnBa?=
 =?utf-8?B?WFB6MFZNM2dEQWlERUNrdWZkc3g3aUhWZS9pUUlMeWUvNzhCTnZRU05iS2RM?=
 =?utf-8?B?VElKT21WSHl5amJtL1ppWkJ1Mk8zeHUwMTVFTjZtN29mOVZtQkxDQlNyNGRU?=
 =?utf-8?B?ZHJIYkdEUDVvdG85dTg3b0JobTczTy9GUFRHbXVDWTBGUndacHVaWGJFZWgx?=
 =?utf-8?B?NTFrejgxR1NiTmhZR2E5MGVwUmdleitFblQ4Q2llNzV2bkJ6WEFPTGNNS0lN?=
 =?utf-8?B?M001T1VUSmhUYW9sbS90WTRTWnJCV2VncldIbzVQUHk0Ty81QTBDcWI1STFs?=
 =?utf-8?B?Mmh6ZnN0RWdTUnQrZTFPK3AzUGhpNHh5cFNsK0VwVlBFeGhINzhYd1VpaCtz?=
 =?utf-8?B?YWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	lx7k5cx2tLgjOMMyO9NGIokDZh3qdjXxjEawZm7eyQAsbK306RxNLP/ZM9bay4yrJ1Ja3GhzIM6Pg3etQkVvcz59SRH3CqzPvhh9Uc3hy/F4LpMGWxyEXoYiEA3LdKGpJLb7RfmfNdxCV8shBUbzVD/hU/jrigabf9c0XAvXkFQfmpNePTevbZ71VebJgH2HcbTpopkGdgi7MZIPXrOlsgPv40Jim3TicbgEWaTSveLFHv+Cp05NfT01FPgaUaYlNY2avxuge+E6z3LuIuQMxWLJvPnsrmIRZMXU36mntv0oGbK06ZGN8RNCK67g09/tgFGoBhGp47rXxsOJ+WjkKbetbgE7Ttd3D355yDswGFsPXv9+xoV/SCTCuTGrKDnrXWFnJSGaikBFFEJjYVt+rQXRAX1iK1pZnLX5SYhM8bSHFK6t7yznq4dhVgH54jCje16B1Wp4CKcixZHlCOKCdDE15/aQNOr+YY15LMFCAsD1yfhcQITSPVrzTkFY+gA/WnhIImmbs3n0c4VkcK+Csvx+239O1VvpGwtbX9AP+/LLrFE7VKUujGHxWW9IamgZgAeUef+SRztUMtCZcTINkluZxT0pz9EsZDWLi1omBL4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 47d7ed05-4817-4d5f-47f2-08dc3973b573
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 22:13:50.8950
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bYanpWMMhKg9HddmtmiQ6enlb06oLB+tHzekJkYh51ecl4/jmHAkii7omX74qPzQfR1I7+aADqRLzlF610wZJQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR10MB6003
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-29_06,2024-02-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0
 mlxlogscore=999 malwarescore=0 spamscore=0 mlxscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402290174
X-Proofpoint-ORIG-GUID: pi51hwFQAYyS0Y7JP49C7QuyoyD8aXWI
X-Proofpoint-GUID: pi51hwFQAYyS0Y7JP49C7QuyoyD8aXWI


On 2/29/24 13:53, Huang, Kai wrote:
> 
> 
> On 1/03/2024 10:25 am, Dongli Zhang wrote:
>> Replace PAGE_OFFSET with KVM_HVA_ERR_BAD, to facilitate the cscope when
>> looking for where KVM_HVA_ERR_BAD is used.
>>
>> Every time I use cscope to query the functions that are impacted by the
>> return value (KVM_HVA_ERR_BAD) of __gfn_to_hva_many(), I may miss
>> kvm_is_error_hva().
> 
> I am not sure "to facilitate cscope" could be a justification to do some code
> change in the kernel.
> 
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>   include/linux/kvm_host.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7e7fd25b09b3..4dc0300e7766 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -143,7 +143,7 @@ static inline bool is_noslot_pfn(kvm_pfn_t pfn)
>>     static inline bool kvm_is_error_hva(unsigned long addr)
>>   {
>> -    return addr >= PAGE_OFFSET;
>> +    return addr >= KVM_HVA_ERR_BAD;
>>   }
>>     #endif
> 
> 
> Also, IIUC the KVM_HVA_ERR_BAD _theoretically_ can be any random value that can
> make kvm_is_error_hva() return false, while kvm_is_error_hva() must catch all
> error HVAs.
> 
> E.g., if we ever change KVM_HVA_ERR_BAD to use any other value (although I don't
> see why this could ever happen), then using KVM_HVA_ERR_BAD in
> kvm_is_error_hva() would be broken.
> 
> In other words, it seems to me we should just use PAGE_OFFSET in
> kvm_is_error_hva().
> 


At least so far PAGE_OFFSET is the same value as KVM_HVA_ERR_BAD (except
mips/s390), as line 141. Therefore, this is "No functional change".

It indicates the userspace VMM can never have hva in the range of kernel space.

 139 #ifndef KVM_HVA_ERR_BAD
 140
 141 #define KVM_HVA_ERR_BAD         (PAGE_OFFSET)
 142 #define KVM_HVA_ERR_RO_BAD      (PAGE_OFFSET + PAGE_SIZE)
 143
 144 static inline bool kvm_is_error_hva(unsigned long addr)
 145 {
 146         return addr >= PAGE_OFFSET;
 147 }
 148
 149 #endif


Regarding to "facilitate cscope", this happened since long time ago when I read
about ept_violation/mmio path.

1. The __gfn_to_hva_many() may return KVM_HVA_ERR_BAD for mmio.
2. Then I used cscope to find the location of KVM_HVA_ERR_BAD.
3. The kvm_is_error_hva() is not in the results.
4. It took me a while to figure out that the 'KVM_HVA_ERR_BAD' is indirectly
used by kvm_is_error_hva().

This is just based on my own experience when reading mmio code path. Thank you
very much!

Dongli Zhang