Re: [Xen-devel] [BUG] After upgrade to Xen 4.12.0 iommu=no-igfx

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 01, 2019 at 11:25:04AM -0700, Roman Shaposhnik wrote:
> On Thu, Aug 1, 2019 at 1:16 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Wed, Jul 31, 2019 at 02:03:24PM -0700, Roman Shaposhnik wrote:
> > > On Wed, Jul 31, 2019 at 12:46 PM Andrew Cooper
> > > <andrew.cooper3@xxxxxxxxxx> wrote:
> > > >
> > > > On 31/07/2019 20:35, Roman Shaposhnik wrote:
> > > > > On Wed, Jul 31, 2019 at 1:43 AM Roger Pau Monné 
> > > > > <roger.pau@xxxxxxxxxx> wrote:
> > > > >> On Wed, Jul 31, 2019 at 10:36:31AM +0200, Roger Pau Monné wrote:
> > > > >>> On Tue, Jul 30, 2019 at 10:55:24AM -0700, Roman Shaposhnik wrote:
> > > > >>>> Sorry -- got a bit distracted yesterday. Attached is the log with 
> > > > >>>> only
> > > > >>>> your latest patch attached. Interestingly enough the box booted 
> > > > >>>> fine
> > > > >>>> without screen artifacts. So I guess we're getting closer...
> > > > >>>>
> > > > >>>> Thanks for all the help!
> > > > >>> That's quite weird, there's no functional changes between the
> > > > >>> previous patches and this one, the only difference is that this 
> > > > >>> patch
> > > > >>> has more verbose output.
> > > > >>>
> > > > >>> Are you sure you didn't have any local patches on top of Xen that
> > > > >>> could explain this difference in behaviour?
> > > > >> FWIW, can you please try the plain patch again:
> > > > >>
> > > > >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01547.html
> > > > >>
> > > > >> And report back?
> > > > >>
> > > > >> I would like to get this committed ASAP if it does fix your issue.
> > > > > I'd like to say that it did -- but I tried it again just now and it
> > > > > still garbled screen and tons of:
> > > > >
> > > > > (XEN) printk: 26665 messages suppressed.
> > > > > (XEN) [VT-D]DMAR:[DMA Read] Request device [0000:00:02.0] fault addr
> > > > > 8e14c000, iommu reg = ffff82c0008de000
> > > > >
> > > > > I'm very much confused by what's going on, but it seems that's the
> > > > > case -- adding those debug print statements make the issue go away
> > > > >
> > > > > Here are the patches that are being applied:
> > > > >    NOT WORKING:
> > > > > https://github.com/rvs/eve/blob/xen-bug/pkg/xen/01-iommu-mappings.patch
> > > > >
> > > > >    WORKING: 
> > > > > https://github.com/rvs/eve/blob/a1291fcd4e669df2a63285afb5e8b4841f45c1c8/pkg/xen/01-iommu-mappings.patch
> > > > >
> > > > > At this point I'm really not sure what's going on.
> > > >
> > > > Ok.  seeing as you've double checked this, the mystery deepens.
> > > >
> > > > My bet is on the intel_iommu_lookup_page() call having side effects[1].
> > > > If you take out the debugging in the middle of the loop in
> > > > rmrr_identity_mapping(), does the problem reproduce again?
> > > >
> > > > ~Andrew
> > > >
> > > > [1] Looking at the internals of addr_to_dma_page_maddr(), it does 100%
> > > > more memory allocation and higher-level PTE construction than looks wise
> > > > for what is supposed to be a getter.
> > >
> > > Yup. That's what it is -- intel_iommu_lookup_page() seems to be the 
> > > culprit.
> > >
> > > I've did the experiment in the other direction -- adding a dummy call:
> > >      
> > > https://github.com/rvs/eve/blob/36aeeaa7c0a53474fb1ecef2ff587a86637df858/pkg/xen/01-iommu-mappings.patch#L23
> > > on top of original Roger's patch makes system boot NORMALLY.
> >
> > I'm again quite lost, and I don't really understand why mappings added
> > by arch_iommu_hwdom_init seems to work fine while mappings added by
> > rmrr_identity_mapping don't.
> >
> > I have yet another patch for you to try, which attempts to mimic
> > exactly what arch_iommu_hwdom_init does into rmrr_identity_mapping,
> > can you please give it a try?
> >
> > This has the added bonus of limiting the use of
> > {set/clear}_identity_p2m_entry to translated domains only, since
> > rmrr_identity_mapping was the only caller against PV domains.
> >
> > Thanks, Roger.
> > ---8<---
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index fef97c82f6..d36a58b1a6 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1341,10 +1341,8 @@ int set_identity_p2m_entry(struct domain *d, 
> > unsigned long gfn_l,
> >
> >      if ( !paging_mode_translate(p2m->domain) )
> >      {
> > -        if ( !need_iommu_pt_sync(d) )
> > -            return 0;
> > -        return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
> > -                                IOMMUF_readable | IOMMUF_writable);
> > +        ASSERT_UNREACHABLE();
> > +        return -ENXIO;
> >      }
> >
> >      gfn_lock(p2m, gfn, 0);
> > @@ -1432,9 +1430,8 @@ int clear_identity_p2m_entry(struct domain *d, 
> > unsigned long gfn_l)
> >
> >      if ( !paging_mode_translate(d) )
> >      {
> > -        if ( !need_iommu_pt_sync(d) )
> > -            return 0;
> > -        return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
> > +        ASSERT_UNREACHABLE();
> > +        return -ENXIO;
> >      }
> >
> >      gfn_lock(p2m, gfn, 0);
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 5d72270c5b..62df5ca5aa 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1969,6 +1969,7 @@ static int rmrr_identity_mapping(struct domain *d, 
> > bool_t map,
> >      unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> 
> > PAGE_SHIFT_4K;
> >      struct mapped_rmrr *mrmrr;
> >      struct domain_iommu *hd = dom_iommu(d);
> > +    unsigned int flush_flags = 0;
> >
> >      ASSERT(pcidevs_locked());
> >      ASSERT(rmrr->base_address < rmrr->end_address);
> > @@ -1982,7 +1983,7 @@ static int rmrr_identity_mapping(struct domain *d, 
> > bool_t map,
> >          if ( mrmrr->base == rmrr->base_address &&
> >               mrmrr->end == rmrr->end_address )
> >          {
> > -            int ret = 0;
> > +            int ret = 0, err;
> >
> >              if ( map )
> >              {
> > @@ -1995,13 +1996,20 @@ static int rmrr_identity_mapping(struct domain *d, 
> > bool_t map,
> >
> >              while ( base_pfn < end_pfn )
> >              {
> > -                if ( clear_identity_p2m_entry(d, base_pfn) )
> > -                    ret = -ENXIO;
> > +                if ( paging_mode_translate(d) )
> > +                    ret = clear_identity_p2m_entry(d, base_pfn);
> > +                else
> > +                    ret = iommu_unmap(d, _dfn(base_pfn), PAGE_ORDER_4K,
> > +                                      &flush_flags);
> >                  base_pfn++;
> >              }
> >
> >              list_del(&mrmrr->list);
> >              xfree(mrmrr);
> > +            /* Keep the previous error code if there's one. */
> > +            err = iommu_iotlb_flush_all(d, flush_flags);
> > +            if ( !ret )
> > +                ret = err;
> >              return ret;
> >          }
> >      }
> > @@ -2011,8 +2019,13 @@ static int rmrr_identity_mapping(struct domain *d, 
> > bool_t map,
> >
> >      while ( base_pfn < end_pfn )
> >      {
> > -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> > +        int err;
> >
> > +        if ( paging_mode_translate(d) )
> > +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> > +        else
> > +            err = iommu_map(d, _dfn(base_pfn), _mfn(base_pfn), 
> > PAGE_ORDER_4K,
> > +                            IOMMUF_readable | IOMMUF_writable, 
> > &flush_flags);
> >          if ( err )
> >              return err;
> >          base_pfn++;
> > @@ -2026,7 +2039,7 @@ static int rmrr_identity_mapping(struct domain *d, 
> > bool_t map,
> >      mrmrr->count = 1;
> >      list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
> >
> > -    return 0;
> > +    return iommu_iotlb_flush_all(d, flush_flags);
> >  }
> >
> >  static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev)
> 
> This patch completely fixes the problem for me!
> 
> Thanks Roger! I'd love to see this in Xen 4.13

Thanks for testing!

It's still not clear to me why the previous approach didn't work, but
I think this patch is better because it removes the usage of
{set/clear}_identity_p2m_entry from PV domains. I will submit this
formally now.

Roger.

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