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

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

On 2015/6/11 17:23, Jan Beulich wrote:
> > > > On 11.06.15 at 10:23, <tiejun.chen@xxxxxxxxx> wrote:
> > 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."
> Ah, I right, I now remember. In which case checking against
> INVALID_MFN would cover the MMIO case, but not the PoD one.
>
> > 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?
> And it seems to me that this particular combination would need
> special treatment, i.e. you'd need
>
>         if ( p2mt == p2m_invalid ||
>              (p2mt == p2m_mmio_dm && mfn_x(mfn) == INVALID_MFN) )
>
> as long as p2m_invalid isn't the default type lookups return. But
> I'd recommend waiting for Tim to confirm or further adjust that.
Sure.

Thanks
Tiejun

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