| Message ID | 20230311002258.852397-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gvt: KVM: KVMGT fixes and page-track cleanups | expand |
On Saturday, March 11, 2023 8:23 AM, Sean Christopherson wrote: > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index 4ec85308379a..58b9b316ae46 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -1183,6 +1183,10 @@ static int is_2MB_gtt_possible(struct intel_vgpu > *vgpu, > pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); > if (is_error_noslot_pfn(pfn)) > return -EINVAL; > + > + if (!pfn_valid(pfn)) > + return -EINVAL; > + Merge the two errors in one "if" to have less LOC? i.e. if (is_error_noslot_pfn(pfn) || !pfn_valid(pfn)) return -EINVAL;
On 13.03.2023 16:37, Wang, Wei W wrote: > On Saturday, March 11, 2023 8:23 AM, Sean Christopherson wrote: >> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c >> index 4ec85308379a..58b9b316ae46 100644 >> --- a/drivers/gpu/drm/i915/gvt/gtt.c >> +++ b/drivers/gpu/drm/i915/gvt/gtt.c >> @@ -1183,6 +1183,10 @@ static int is_2MB_gtt_possible(struct intel_vgpu >> *vgpu, >> pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); >> if (is_error_noslot_pfn(pfn)) >> return -EINVAL; >> + >> + if (!pfn_valid(pfn)) >> + return -EINVAL; >> + > > Merge the two errors in one "if" to have less LOC? > i.e. > if (is_error_noslot_pfn(pfn) || !pfn_valid(pfn)) > return -EINVAL; you can just replace "if (is_error_noslot_pfn(pfn))" with "if (!pfn_valid(pfn))", it covers both cases. Regards Andrzej
On Wed, Mar 15, 2023, Andrzej Hajda wrote: > On 13.03.2023 16:37, Wang, Wei W wrote: > > On Saturday, March 11, 2023 8:23 AM, Sean Christopherson wrote: > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > > > index 4ec85308379a..58b9b316ae46 100644 > > > --- a/drivers/gpu/drm/i915/gvt/gtt.c > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > > > @@ -1183,6 +1183,10 @@ static int is_2MB_gtt_possible(struct intel_vgpu > > > *vgpu, > > > pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); > > > if (is_error_noslot_pfn(pfn)) > > > return -EINVAL; > > > + > > > + if (!pfn_valid(pfn)) > > > + return -EINVAL; > > > + > > > > Merge the two errors in one "if" to have less LOC? > > i.e. > > if (is_error_noslot_pfn(pfn) || !pfn_valid(pfn)) > > return -EINVAL; > > you can just replace "if (is_error_noslot_pfn(pfn))" with "if > (!pfn_valid(pfn))", it covers both cases. Technically, yes, but the two checks are for very different things. Practically speaking, there can never be false negatives without KVM breaking horribly as overlap between struct page pfns and KVM's error/noslot would prevent mapping legal memory into a KVM guest. But I'd rather not hide the "did KVM find a valid mapping" in the "is this pfn backed by struct page" check, especially since this code goes away entirely by the end of the series.
Reviewed-by: Yan Zhao <yan.y.zhao@intel.com> On Fri, Mar 10, 2023 at 04:22:32PM -0800, Sean Christopherson wrote: > Check that the pfn found by gfn_to_pfn() is actually backed by "struct > page" memory prior to retrieving and dereferencing the page. KVM > supports backing guest memory with VM_PFNMAP, VM_IO, etc., and so > there is no guarantee the pfn returned by gfn_to_pfn() has an associated > "struct page". > > Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > drivers/gpu/drm/i915/gvt/gtt.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c > index 4ec85308379a..58b9b316ae46 100644 > --- a/drivers/gpu/drm/i915/gvt/gtt.c > +++ b/drivers/gpu/drm/i915/gvt/gtt.c > @@ -1183,6 +1183,10 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu, > pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); > if (is_error_noslot_pfn(pfn)) > return -EINVAL; > + > + if (!pfn_valid(pfn)) > + return -EINVAL; > + > return PageTransHuge(pfn_to_page(pfn)); > } > > -- > 2.40.0.rc1.284.g88254d51c5-goog >
diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index 4ec85308379a..58b9b316ae46 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -1183,6 +1183,10 @@ static int is_2MB_gtt_possible(struct intel_vgpu *vgpu, pfn = gfn_to_pfn(vgpu->vfio_device.kvm, ops->get_pfn(entry)); if (is_error_noslot_pfn(pfn)) return -EINVAL; + + if (!pfn_valid(pfn)) + return -EINVAL; + return PageTransHuge(pfn_to_page(pfn)); }
Check that the pfn found by gfn_to_pfn() is actually backed by "struct page" memory prior to retrieving and dereferencing the page. KVM supports backing guest memory with VM_PFNMAP, VM_IO, etc., and so there is no guarantee the pfn returned by gfn_to_pfn() has an associated "struct page". Fixes: b901b252b6cf ("drm/i915/gvt: Add 2M huge gtt support") Signed-off-by: Sean Christopherson <seanjc@google.com> --- drivers/gpu/drm/i915/gvt/gtt.c | 4 ++++ 1 file changed, 4 insertions(+)