[PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
Zhenyu Wang
zhenyuw at linux.intel.com
Tue Apr 16 05:56:10 UTC 2019
On 2019.04.16 05:57:03 +0000, Wang, Zhi A wrote:
> Ping.
>
> -----Original Message-----
> From: Wang, Zhi A
> Sent: Friday, April 12, 2019 11:21 AM
> To: Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: Xu, Colin <colin.xu at intel.com>; Gimbitskii, Aleksei <aleksei.gimbitskii at intel.com>; intel-gvt-dev at lists.freedesktop.org
> Subject: RE: [PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
>
> Yes. But if we keep the enumeration type here, how about we let the enumeration value begins from 0? Then clockwork would be happy while we keep the enumeration type?
>
Sure, seems no problem to me.
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw at linux.intel.com]
> Sent: Friday, April 12, 2019 10:19 AM
> To: Wang, Zhi A <zhi.a.wang at intel.com>
> Cc: Xu, Colin <colin.xu at intel.com>; Gimbitskii, Aleksei <aleksei.gimbitskii at intel.com>; Zhenyu Wang <zhenyuw at linux.intel.com>; intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH V2 1/6] drm/i915/gvt: Remove typedef intel_gvt_gtt_type_t
>
> 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
--
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/20190416/ce7cd9cf/attachment.sig>
More information about the intel-gvt-dev
mailing list