Re: [Xen-devel] [v3][PATCH 02/16] xen/x86/p2m: introduce set_identity_p2m_entry

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/11 15:33, Jan Beulich wrote:
> > > > On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> wrote:
> > We will create this sort of identity mapping as follows:
> >
> > If the gfn space is unoccupied, we just set the mapping. If the space
> > is already occupied by 1:1 mappings, do nothing. Failed for any
> > other cases.
> >
> > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> First of all you continue to be copying each patch to every
> maintainer involved in some part of the series. Please limit the
I just hope all involved guys can see this series on the whole to 
review. But,
> Cc list of each patch to the actual list of people needing to be
> Cc-ed on it (or you know explicitly wanting a copy).
Next, I will just send them to each associated maintainer.

> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -898,6 +898,41 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
> > gfn, mfn_t mfn,
> >       return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
> >   }
> >
> > +int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> > +                           p2m_access_t p2ma)
> > +{
> > +    p2m_type_t p2mt;
> > +    p2m_access_t a;
> > +    mfn_t mfn;
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    int ret;
> > +
> > +    if ( paging_mode_translate(p2m->domain) )
> > +    {
> > +        gfn_lock(p2m, gfn, 0);
> > +
> > +        mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
> > +
> > +        if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN )
> I'm not fundamentally opposed to this extra INVALID_MFN check, but
> could you please clarify for which P2M type you saw INVALID_MFN
> coming back here, and for which p2m_invalid cases you didn't also
> see INVALID_MFN? I.e. I'd really prefer a single check to be used
> when that can cover all cases.
Actually, I initially adopted "!mfn_valid(mfn)" in our previous version. 
But Tim issued one comment about this,
"I don't think this check is quite right -- for example, this p2m entry
might be an MMIO mapping or a PoD entry.  "if ( p2mt == p2m_invalid )"
would be better."

But if I just keep his recommended check, you can see the following when 
I pass through IGD,
(XEN) Cannot identity map d1:ad800, already mapped to ffffffffffffffff 
with p2mt:4.
Looks "4" indicates p2m_mmio_dm, right?

> > +            ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> > +                                p2m_mmio_direct, p2ma);
> > +        else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> > +            ret = 0;
> > +        else
> > +        {
> > +            ret = -EBUSY;
> > +            printk(XENLOG_G_WARNING
> > +                   "Cannot identity map d%d:%lx, already mapped to %lx.\n",
> > +                   d->domain_id, gfn, mfn_x(mfn));
> > +        }
> > +
> > +        gfn_unlock(p2m, gfn, 0);
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> Either have a single return point, or invert the original if() and bail
> early (reducing the indentation level on the main body of the code).
Sure, I guess I can follow this patten,

int ret = 0;

if ()
{
    ...
}

return ret;

Thanks
Tiejun

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