Re: [Xen-devel] [PATCH v3.1 08/15] x86/vtd: fix mapping of RMRR regions

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Nov 04, 2016 at 04:34:58AM -0600, Jan Beulich wrote:
> >>> On 04.11.16 at 10:45, <roger.pau@xxxxxxxxxx> wrote:
> > On Fri, Nov 04, 2016 at 03:16:47AM -0600, Jan Beulich wrote:
> >> >>> On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/mm/p2m.c
> >> > +++ b/xen/arch/x86/mm/p2m.c
> >> > @@ -1049,22 +1049,29 @@ int set_identity_p2m_entry(struct domain *d, 
> >> > unsigned long gfn,
> >> >  
> >> >      mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
> >> >  
> >> > -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> >> > +    switch ( p2mt )
> >> > +    {
> >> > +    case p2m_invalid:
> >> > +    case p2m_mmio_dm:
> >> >          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;
> >> > -        /*
> >> > -         * PVH fixme: during Dom0 PVH construction, p2m entries are 
> >> > being set
> >> > -         * but iomem regions are not mapped with IOMMU. This makes sure 
> >> > that
> >> > -         * RMRRs are correctly mapped with IOMMU.
> >> > -         */
> >> > -        if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
> >> > +        if ( ret )
> >> > +            break;
> >> > +        /* fallthrough */
> >> > +    case p2m_mmio_direct:
> >> > +        if ( p2mt == p2m_mmio_direct && a != p2ma )
> >> 
> >> I don't understand the removal of the MFN == GFN check, and it
> >> also isn't being explained in the commit message.
> > 
> > Maybe I'm not understanding the logic of this function correctly, but it 
> > seems extremely bogus, and behaves quite differently depending on whether 
> > gfn == mfn and whether the domain is the hardware domain.
> 
> I can't exclude there's something wrong here, but you're removing
> a safety belt. Before touching this, did you go back in history to
> find out why things are the way they are? I remember it having
> taken quite a bit of discussion to reach a mostly acceptable flow
> here.

As said, I agree that the gfn == mfn check should be kept.

I've looked at 0e9e09 and 5ae039, but I cannot really understand how 5ae039 
was supposed to work in the first place, and to create the proper IOMMU 
mappings for RMRR regions. It replaced a call to intel_iommu_map_page with a 
call to set_identity_p2m_entry, and this newly introduced function 
(set_identity_p2m_entry) will only setup the p2m mappings for the required 
page, but it will completely ignore to setup any IOMMU mappings if the pt is 
not shared between HAP and the IOMMU.

Then 0e9e09 is a fixup for PVH guests, which really require RMRR regions 
properly mapped in the IOMMU in order to run. Since on PVH guests holes and 
reserved regions are identity mapped in the p2m, RMRR regions should already 
be mapped in the p2m, so 0e9e09 just added the IOMMU mappings if the pt was 
not shared.

But yet I think that 0e9e09 is wrong, and that it fixed RMRR mappings for 
hardware that shares the pt between HAP and the IOMMU while breaking it for 
hardware that doesn't share the pt between HAP and the IOMMU.
 
> > If gfn == mfn (so the page is already mapped in the p2m) and the domain is 
> > the hardware domain, an IOMMU mapping would be established. If gfn is not 
> > set, we will just set the p2m entry, but the IOMMU is not going to be 
> > properly configured, unless it shares the pt with p2m.
> 
> Well, that's why the comment says "PVH fixme". The issue is not
> the code here, but the code which established the mapping we
> found here. That code fails to also do the IOMMU mapping when
> needed. The only correct course of action, afaict, would be to
> fix that other code (wherever that is) and remove the comment
> together with the bogus code here (which would lead to just
> "ret = 0" remaining.

On classic PVH all holes or reserved regions in the memory map are identity 
mapped into the p2m, this is why RMRR regions where expected to be already 
mapped in the p2m. This is no longer true for PVHv2 domains, and holes or 
reserved regions are no longer mapped by default into the p2m.

> > This patch fixes the behavior of the function so it's consistent, and we 
> > can guarantee that after calling it a proper mapping in the p2m and the 
> > IOMMU 
> > will exist, and that it's going to be gfn == mfn, or else an error will be 
> > returned.
> > 
> > I agree with you that the mfn == gfn check should be kept, so the condition 
> > above should be:
> > 
> >     if ( p2mt == p2m_mmio_direct && (a != p2ma || gfn != mfn) )
> > 
> > But please see below.
> > 
> >> And then following a case label with a comparison of the respective
> >> switch expression against the very value from the case label is
> >> certainly odd. I'm pretty sure a better structure of the code could be
> >> found.
> > 
> > Hm, the comparison is there because of the fallthrough in the above case. I 
> > could remove it by also setting the IOMMU entry in the above case, if 
> > that's 
> > better, so it would look like:
> > 
> > case p2m_invalid:
> > case p2m_mmio_dm:
> >     ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> >                         p2m_mmio_direct, p2ma);
> >     if ( ret )
> >         break;
> >     if ( !iommu_use_hap_pt(d) )
> >         ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
> >     break;
> > case p2m_mmio_direct:
> >     if ( a != p2ma || gfn != mfn )
> >     {
> >         printk(XENLOG_G_WARNING
> >                "Cannot setup identity map d%d:%lx, already mapped with "
> >                "different access type or mfn\n", d->domain_id, gfn);
> >         ret = (flag & XEN_DOMCTL_DEV_RDM_RELAXED) ? 0 : -EBUSY;
> >         break;
> >     }
> >     if ( !iommu_use_hap_pt(d) )
> >         ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
> 
> Well, since according to what I've said above this code should
> really not be here, I think the code structuring question is moot
> now. The conditional call to iommu_map_page() really just needs
> adding alongside the p2m_set_entry() call.

OK, so if the gfn is already mapped into the p2m we don't care whether it 
has a valid IOMMU mapping or not?

Roger.

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