[PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
Zhenyu Wang
zhenyuw at linux.intel.com
Fri Apr 12 07:19:24 UTC 2019
On 2019.04.12 06:20:26 +0000, Wang, Zhi A wrote:
> Also, the other reason to remove the typedef is:
> https://yarchive.net/comp/linux/typedefs.html
>
> i915 has already removed almost all of them because of this.
>
Remove typedef is ok, I think everyone agree but would be better still keep enum type.
>
> -----Original Message-----
> From: Wang, Zhi A
> Sent: Friday, April 12, 2019 8:44 AM
> To: Xu, Colin <colin.xu at intel.com>; Gimbitskii, Aleksei <aleksei.gimbitskii at intel.com>; Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> Subject: RE: [PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
>
> I think klocwork is going a bit too far actually. It always takes the full range would be used if we go enumeration type. For option 2, I guess we have to add a lot of useless check of GTT_TYPE_INVALID in every place we are going to use the enumeration as array index, which looks very ugly.
>
> I'd like to go practical. We need the enumeration for sure and we check the GTT_TYPE_INVALID in the place we think necessary and let clockwork be happy and also not make the code ugly.
>
> -----Original Message-----
> From: Xu, Colin
> Sent: Friday, April 12, 2019 4:29 AM
> To: Gimbitskii, Aleksei <aleksei.gimbitskii at intel.com>; Wang, Zhi A <zhi.a.wang at intel.com>; Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
>
> Seems like V2 of this patch doesn't get updated?
>
> Hi Zhi, Zhenyu, from architecture point of view, what kind of change is better?
> - Change parameter type from enum type to int looks "cheat" klocwork static analysis, but lose natural protection since any int could be passed into the function.
> - If we keep using enum type as the function parameter, we may need extract check so that klocwork static analysis will consider as safe.
> Any comments? I prefer the latter.
>
> On 2019-04-11 18:46, 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.
> >
> > 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
>
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20190412/105db582/attachment.sig>
More information about the intel-gvt-dev
mailing list