Received-SPF: pass (google.com: domain of linux-kernel+bounces-87649-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <6d17801e-33be-4f6e-a61b-8d8f43238261@intel.com>
Date: Fri, 1 Mar 2024 11:39:41 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva
Content-Language: en-US
To: Dongli Zhang <dongli.zhang@oracle.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>
 <7fcfc226-0263-0364-bed7-fc95e6c945cb@oracle.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <7fcfc226-0263-0364-bed7-fc95e6c945cb@oracle.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?TTVtdEpPM1hCSG1rZVpIUm1vaVd6aWlkYVRRY2pFS2xaNHlNUjN0aEt1cmpK?=
 =?utf-8?B?YkJZZnE1M3hXa1JXK3dmS1JGdzhnaFhsYkt1Y1E0RWZxcVdCclFSVlB0eFp6?=
 =?utf-8?B?NHptNnJpNDluajBiSytLaHNabFBPL1d3STZBd09TT1lIY0ZFcVVHQnR1cVdz?=
 =?utf-8?B?cnJHc0NJc1E3WE5PY2FnTHB5QTBHZ2dxUEJ6bW8zcjEwSERJb2JSdVZqZWV2?=
 =?utf-8?B?YTdTcVlhWTV2UUpHWlA4RkxMbFdnUDBmVFFhbEFQQjdFWkJuM3c4VmNVZHRX?=
 =?utf-8?B?TkN0Sis0VFBRQkEwbXQvTnhpV2xNWWRycTBERmtuSUo3QlhiY2FMakJmdzIy?=
 =?utf-8?B?aDZ1S0pSVXN1bW1BRnR1WkZTN2xVblBiZk15dUtZVGk3VE1zTG4xck05MHZB?=
 =?utf-8?B?Mm9rck5rbnVlZ2NIZUY2R242QnUyNEdUejlWdGZPZFVZNHY0cndEUjlYY3Zj?=
 =?utf-8?B?Q3VpajdUcTlBWUlQOUdQM2dXMEYyRU1OaTBUcmY0UE45Y3NBdFhUbEdwUUJF?=
 =?utf-8?B?ck51UXB0K2s3Y2o5OUdqNzZaVE52dk92MHkvRDREdDY2Q21adzdMRnZleEVw?=
 =?utf-8?B?YTdvTEtha1BjeHY4U3JLNEZZd2g3RytXdjl4SkIxZ2VMQ3hmRXA0RmpsMWZK?=
 =?utf-8?B?S2g1Y3dMRkdTZjc2VFNja2xvdW42NGVYWkM5eVliYWZpc1VvbWtnZHpHVk1D?=
 =?utf-8?B?U1ZVdVprYlNBS3d3aFp0dzNLUkNmZHVONDVUQWU2aHBocUo1aEJkQk9pa0xE?=
 =?utf-8?B?MTU0cVFDQUhxb0o2MStZZThlY2xGd25yVzNyNnF5aStaWE0rVk5EUk5MN2dC?=
 =?utf-8?B?Q1l3bjZOYUxqanV2WVhlWDBKOUlxSjRlT3ZCN05keEpBR1NtbWRjWktTNXpK?=
 =?utf-8?B?NWV0bTZiNmFvQnozSmJCWGh1OGFYNjExWGdNY1BweDFsZzhxOWtnckU0OFdi?=
 =?utf-8?B?TjhDbzlDNWJlS00zSSs1UWNyS0VIVmprbllWSkorTWdwQXFURDFoYllJR3NO?=
 =?utf-8?B?RXZpMWxaeFRVRXh4a1FmY2JHeTZ2eWhQYmNIY2dQSUIzaU5jRXNVWE1VWDRI?=
 =?utf-8?B?Tndwa0tOeFBqZTVxd0dYeWZrTTh1NUtuYkdaZ0hDRHl3MTRyMmVOcDJLUUJ5?=
 =?utf-8?B?YWVLRnhrZUM2WWZBN2tQSnpWcHRVbFdPWmljbU5hSUpZbFI0eFJZVjhqYlB5?=
 =?utf-8?B?bmEyNlFEYWpMcGZBaFB1VTRubnd3cXFZTm8wajdWR1haK3BHMEdzckRtQ0xw?=
 =?utf-8?B?Y1dwY1BnYyttdlJLcVB6ZEZDS0FkSXNzczdIRk9IVTJHSm9IMkdGQlVBZWFa?=
 =?utf-8?B?RkI2NXJ3WXNvb0pnM0s0Snc4N3ZtM0pxbGF1RnlaMS9WOUFYTFlQUmFxanMy?=
 =?utf-8?B?T2tzYVRXYUhBU1F6b2xRbzNXcHdnUDZHNnNZa1FpcjdLMTBKTHYwUjBFQkNF?=
 =?utf-8?B?SWppdnlweVM1N2VwTlhVQTBNQ3NQY0laMTFncTR1WFlqQUFOWndLWlZaS2VX?=
 =?utf-8?B?KzVza2RrbVI0aklFL0puTGt4dHNrbmNicFo4UWhSa0NCVGFDWE80d3daTnk1?=
 =?utf-8?B?OHhaY0xYMnp4ZFJDRmdDZkkzVjhZTy9qQzB6MnNjTmJsVlg0aGxlL2pxazBS?=
 =?utf-8?B?WlhBbThQcHFCM1BMVmcwV3RUNHAvSGJoam42QkZkb29QV3gvUDVrZ2Eraldo?=
 =?utf-8?B?MHlSeTV6Y3NXSElML1Y3TndZWC9KbGc5QmNKbnp2NGtLZUtVTklQdlBuZHVX?=
 =?utf-8?B?NFJwU2lRbXNlbDlzdGc5ZWl6MkN0Qk5OcU8va0QvVnRiZHR4MUdKdUI3WnM4?=
 =?utf-8?B?MkltcENvbVlmYjdncXNPNDAyWkdYVllVcTVMOEJLQUw1NEdXaXR5L3cyeXBo?=
 =?utf-8?B?NGJ6amZKNGFFZHJOQ29rUFI4UUg3TTZnSk9oWTdKczB5VmJkc2NxRUx1VUVK?=
 =?utf-8?B?YzFldE80eWN2MUNrVUpiK2o3SCtyaWdNQ2xhYkl4akYyOFBlRW96Z1V3VklU?=
 =?utf-8?B?NndwSjBvWTNnZUpxUTdIcnNrWStiRk9MYThPNXVRdFV3Zlh5bE5ZV1BReHpi?=
 =?utf-8?B?WUc3QU03YnZsUVVJMksrTVlrLzdSbVVnN2N0VVQwV01vWDBycUtsM3R2bjk5?=
 =?utf-8?Q?2zQ6Z9NBr+7VZ+1z1u+uGjoWw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2daff21c-686b-497e-2716-08dc397755d4
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 22:39:48.5061
 (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: W0cOf5maxj9q1NjUrLfLcW+KMpvXxddEKb1eN+V/FRKo5Allxj7PP6ECE9brQYENTgJgPHJNgFUkiN/dqM2s7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7929
X-OriginatorOrg: intel.com


On 1/03/2024 11:13 am, Dongli Zhang wrote:
> 
> 
> 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!

Neither of these can justify this patch.

As I replied earlier, _logically_, IIUC kvm_is_error_hva() shouldn't use 
KVM_HVA_ERR_BAD, because the former needs to catch *ALL* bad HVA but the 
latter could be some *RANDOM* bad HVA.