Re: [PATCH 1/1] KVM: use KVM_HVA_ERR_BAD to check bad hva

[Huang, Kai]

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@xxxxxxxxxx>
> > > ---
> > >    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.