Re: [Xen-devel] [PATCH 3/4] x86/mce: Translate passed-in GPA to host machine address

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Tue, Sep 15, 2015 at 07:24:34AM -0600, Jan Beulich wrote:
> >>> On 15.09.15 at 10:29, <haozhong.zhang@xxxxxxxxx> wrote:
> > @@ -1422,6 +1423,38 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) 
> > u_xen_mc)
> >          if (mc_msrinject->mcinj_count == 0)
> >              return 0;
> >
> > +        if (mc_msrinject->mcinj_flags & MC_MSRINJ_F_GPADDR) {
>
> Perhaps you earlier patch fixing coding style didn't go far enough:
> Patch context as well as you additions make immediately clear that
> this file (or just this function) doesn't follow either Xen or Linux
> coding style. Going through the file as whole I see attempts to
> follow Xen style, so please at least make sure new addition follow
> that style rather than the surrounding broken one.
>

I was also confused by the mixed code style in this file and just
followed the surrounding one. I'll update my changes to follow Xen
style in the next version.

> > +            struct domain *d;
> > +            struct mcinfo_msr *msr;
> > +            int i;
>
> unsigned int
>

will fix in the next version

> > +            uint64_t gaddr, gpfn, mfn, haddr;
>
> paddr_t and unsigned long.

will fix in the next version

>
> > +            p2m_type_t t;
> > +
> > +            d = get_domain_by_id(mc_msrinject->mcinj_domid);
> > +            if (d == NULL)
> > +                return x86_mcerr("do_mca inject: illegal domain id", 
> > -EINVAL);
> > +
> > +            for (i = 0, msr = &mc_msrinject->mcinj_msr[0];
> > +                 i < mc_msrinject->mcinj_count; i++, msr++) {
> > +                gaddr = msr->value;
> > +                gpfn = gaddr >> PAGE_SHIFT;
>
> PFN_DOWN()
>

will fix in the next version

> > +                mfn = mfn_x(get_gfn(d, gpfn, &t));
> > +
> > +                if (mfn == INVALID_MFN) {
> > +                    put_domain(d);
> > +                    return x86_mcerr("do_mca inject: illegal MSR value",
> > +                                     -EINVAL);
>
> This message should be better distinguishable from the one further
> down (or else it's pretty pointless) - perhaps you mean GPFN instead
> of MSR?
>

Yes, I'll change it to "do_mca_inject: illegal GPFN".

> > +                }
> > +
> > +                haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>
> pfn_to_paddr()
>

will fix in the next version

> > +                msr->value = haddr;
>
> I also don't see the purpose of the intermediate variable here.
>

will be merged with the previous line

> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel