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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.18 at 14:21, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>

Jan



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