[PATCH 1/5] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
Gimbitskii, Aleksei
aleksei.gimbitskii at intel.com
Tue Apr 9 10:38:55 UTC 2019
Yes, you are absolutely right, changing type does not prevent any errors. It was done to pass klocwork requirements, seems like code analyzer cannot fully analyze all possible values of custom data types, it just take it at full range. Also I was assuming that driver would not create memory page with invalid data type. But I double checked my previous assumption, yes indeed according to gtt_type_table_entry, entry_type is always valid value. However next_pt_type may be equal to GTT_TYPE_INVALID in some cases, so I found one place where some additional check should be added, will submit it as separate patch.
- BR, Aleksei.
-----Original Message-----
From: Xu, Colin
Sent: Monday, April 8, 2019 9:48 AM
To: Gimbitskii, Aleksei <aleksei.gimbitskii at intel.com>; intel-gvt-dev at lists.freedesktop.org
Cc: Wang, Zhi A <zhi.a.wang at intel.com>; Zhenyu Wang <zhenyuw at linux.intel.com>
Subject: Re: [PATCH 1/5] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
Better have a cover-letter for a patch set.
On 2019-04-08 13:54, Aleksei Gimbitskii wrote:
> The klocwork static code analyzer takes the enumeration as the full
> range of intel_gvt_gtt_type_t. But the full range of
> intel_gvt_gtt_type_t will never be used in full range. For example,
> the GTT_TYPE_INVALID will never be used as an index of an array. So we
> will not use it as a type but only the enumeration.
>
> This patch fixed the critial issues #483, #551, #665 reported by
> klockwork.
Changing function param from intel_gvt_gtt_type_t to int doesn't actually avoid an invalid index into the array. If the caller pass an invalid type, indexing to scratch_pt could still overflow. Compared with int, enum can provide natural protection by only allow possible values.
Better find another way to solve the issues.
> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii at intel.com>
> Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang at intel.com>
> ---
> drivers/gpu/drm/i915/gvt/gtt.c | 12 ++++++------
> drivers/gpu/drm/i915/gvt/gtt.h | 14 +++++++-------
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> b/drivers/gpu/drm/i915/gvt/gtt.c index 88ed2e9df253..8dcb6223b985
> 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -805,7 +805,7 @@ static int reclaim_one_ppgtt_mm(struct intel_gvt
> *gvt);
>
> /* Allocate shadow page table without guest page. */
> static struct intel_vgpu_ppgtt_spt *ppgtt_alloc_spt(
> - struct intel_vgpu *vgpu, intel_gvt_gtt_type_t type)
> + struct intel_vgpu *vgpu, int type)
> {
> struct device *kdev = &vgpu->gvt->dev_priv->drm.pdev->dev;
> struct intel_vgpu_ppgtt_spt *spt = NULL; @@ -855,7 +855,7 @@ static
> struct intel_vgpu_ppgtt_spt *ppgtt_alloc_spt(
>
> /* Allocate shadow page table associated with specific gfn. */
> static struct intel_vgpu_ppgtt_spt *ppgtt_alloc_spt_gfn(
> - struct intel_vgpu *vgpu, intel_gvt_gtt_type_t type,
> + struct intel_vgpu *vgpu, int type,
> unsigned long gfn, bool guest_pde_ips)
> {
> struct intel_vgpu_ppgtt_spt *spt;
> @@ -930,7 +930,7 @@ static int ppgtt_invalidate_spt_by_shadow_entry(struct intel_vgpu *vgpu,
> {
> struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> struct intel_vgpu_ppgtt_spt *s;
> - intel_gvt_gtt_type_t cur_pt_type;
> + int cur_pt_type;
>
> GEM_BUG_ON(!gtt_type_is_pt(get_next_pt_type(e->type)));
>
> @@ -1849,7 +1849,7 @@ static void vgpu_free_mm(struct intel_vgpu_mm *mm)
> * Zero on success, negative error code in pointer if failed.
> */
> struct intel_vgpu_mm *intel_vgpu_create_ppgtt_mm(struct intel_vgpu *vgpu,
> - intel_gvt_gtt_type_t root_entry_type, u64 pdps[])
> + int root_entry_type, u64 pdps[])
> {
> struct intel_gvt *gvt = vgpu->gvt;
> struct intel_vgpu_mm *mm;
> @@ -2303,7 +2303,7 @@ int intel_vgpu_emulate_ggtt_mmio_write(struct intel_vgpu *vgpu,
> }
>
> static int alloc_scratch_pages(struct intel_vgpu *vgpu,
> - intel_gvt_gtt_type_t type)
> + int type)
> {
> struct intel_vgpu_gtt *gtt = &vgpu->gtt;
> struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops; @@
> -2588,7 +2588,7 @@ struct intel_vgpu_mm *intel_vgpu_find_ppgtt_mm(struct intel_vgpu *vgpu,
> * Zero on success, negative error code if failed.
> */
> struct intel_vgpu_mm *intel_vgpu_get_ppgtt_mm(struct intel_vgpu *vgpu,
> - intel_gvt_gtt_type_t root_entry_type, u64 pdps[])
> + int root_entry_type, u64 pdps[])
> {
> struct intel_vgpu_mm *mm;
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h
> b/drivers/gpu/drm/i915/gvt/gtt.h index 32c573aea494..645ddc1bd223
> 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.h
> +++ b/drivers/gpu/drm/i915/gvt/gtt.h
> @@ -95,7 +95,7 @@ struct intel_gvt_gtt {
> unsigned long scratch_mfn;
> };
>
> -typedef enum {
> +enum {
> GTT_TYPE_INVALID = -1,
>
> GTT_TYPE_GGTT_PTE,
> @@ -124,7 +124,7 @@ typedef enum {
> GTT_TYPE_PPGTT_PML4_PT,
>
> GTT_TYPE_MAX,
> -} intel_gvt_gtt_type_t;
> +};
>
> enum intel_gvt_mm_type {
> INTEL_GVT_MM_GGTT,
> @@ -148,7 +148,7 @@ struct intel_vgpu_mm {
>
> union {
> struct {
> - intel_gvt_gtt_type_t root_entry_type;
> + int root_entry_type;
> /*
> * The 4 PDPs in ring context. For 48bit addressing,
> * only PDP0 is valid and point to PML4. For 32it @@ -169,7
> +169,7 @@ struct intel_vgpu_mm {
> };
>
> struct intel_vgpu_mm *intel_vgpu_create_ppgtt_mm(struct intel_vgpu *vgpu,
> - intel_gvt_gtt_type_t root_entry_type, u64 pdps[]);
> + int root_entry_type, u64 pdps[]);
>
> static inline void intel_vgpu_mm_get(struct intel_vgpu_mm *mm)
> {
> @@ -233,7 +233,7 @@ struct intel_vgpu_ppgtt_spt {
> struct intel_vgpu *vgpu;
>
> struct {
> - intel_gvt_gtt_type_t type;
> + int type;
> bool pde_ips; /* for 64KB PTEs */
> void *vaddr;
> struct page *page;
> @@ -241,7 +241,7 @@ struct intel_vgpu_ppgtt_spt {
> } shadow_page;
>
> struct {
> - intel_gvt_gtt_type_t type;
> + int type;
> bool pde_ips; /* for 64KB PTEs */
> unsigned long gfn;
> unsigned long write_cnt;
> @@ -267,7 +267,7 @@ struct intel_vgpu_mm *intel_vgpu_find_ppgtt_mm(struct intel_vgpu *vgpu,
> u64 pdps[]);
>
> struct intel_vgpu_mm *intel_vgpu_get_ppgtt_mm(struct intel_vgpu *vgpu,
> - intel_gvt_gtt_type_t root_entry_type, u64 pdps[]);
> + int root_entry_type, u64 pdps[]);
>
> int intel_vgpu_put_ppgtt_mm(struct intel_vgpu *vgpu, u64 pdps[]);
>
--
Best Regards,
Colin Xu
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
More information about the intel-gvt-dev
mailing list