Re: [PATCH 10/14] x86/mm: Switch {get, put}_gfn() infrastructure to using gfn_t

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Xen-devel <xen-devel@lists.xen.org>,
	Paul Durrant <paul.durrant@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Brian Woods <brian.woods@amd.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 10/14] x86/mm: Switch {get, put}_gfn() infrastructure to using gfn_t
Date: Thu, 22 Nov 2018 08:34:39 -0700	[thread overview]
Message-ID: <5BF6CC8F02000078001FF045@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <1542806482-24030-11-git-send-email-andrew.cooper3@citrix.com>

>>> On 21.11.18 at 14:21, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -512,7 +512,7 @@ int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct 
> domain *d,
>   * XXX following situation missed:
>   * PoD, Foreign mapped, Granted, Shared
>   */
> -int unmmap_broken_page(struct domain *d, mfn_t mfn, unsigned long gfn)
> +int unmmap_broken_page(struct domain *d, mfn_t mfn, gfn_t gfn)

Since you touch it and its caller anyway - could I talk you into dropping
the stray extra 'm' from the name at the same time?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2199,7 +2199,8 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
>              p2m_type_t t;
>              mfn_t mfn;
>  
> -            mfn = get_gfn_query_unlocked(d, gfn_x(vcpu_altp2m(v).veinfo_gfn), &t);
> +            /* TODO: This is a security issue... */
> +            mfn = get_gfn_query_unlocked(d, vcpu_altp2m(v).veinfo_gfn, &t);

I think the comment could do with being slightly less vague. You
and I can see right now what the issue is, but I'm not sure I'll
remember in a year or two without having to spend extra time.

> @@ -3418,7 +3418,7 @@ int shadow_track_dirty_vram(struct domain *d,
>               * write access */
>              for ( i = begin_pfn; i < end_pfn; i++ )
>              {
> -                mfn_t mfn = get_gfn_query_unlocked(d, i, &t);
> +                mfn_t mfn = get_gfn_query_unlocked(d, _gfn(i), &t);
>                  if ( !mfn_eq(mfn, INVALID_MFN) )

Would you mind adding the missing blank line at this occasion? You
already do so elsewhere.

In any event:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel