Re: [Qemu-devel] [PATCH] intel_iommu: handle invalid ce for shadow sync

[Peter Xu <peterx@redhat.com>]

On Mon, Oct 08, 2018 at 11:08:31AM +0800, Jason Wang wrote:

[...]

> > +static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
> > +{
> >       int ret;
> > +    VTDContextEntry ce;
> > +    IOMMUNotifier *n;
> > -    if (ce) {
> > -        /* If the caller provided context entry, use it */
> > -        ce_cache = *ce;
> > -    } else {
> > -        /* If the caller didn't provide ce, try to fetch */
> > -        ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> > -                                       vtd_as->devfn, &ce_cache);
> > -        if (ret) {
> > +    ret = vtd_dev_to_context_entry(vtd_as->iommu_state,
> > +                                   pci_bus_num(vtd_as->bus),
> > +                                   vtd_as->devfn, &ce);
> > +    if (ret) {
> > +        if (ret == -VTD_FR_CONTEXT_ENTRY_P) {
> > +            /*
> > +             * It's a valid scenario to have a context entry that is
> > +             * not present.  For example, when a device is removed
> > +             * from an existing domain then the context entry will be
> > +             * zeroed by the guest before it was put into another
> > +             * domain.  When this happens, instead of synchronizing
> > +             * the shadow pages we should invalidate all existing
> > +             * mappings and notify the backends.
> > +             */
> > +            IOMMU_NOTIFIER_FOREACH(n, &vtd_as->iommu) {
> > +                vtd_address_space_unmap(vtd_as, n);
> > +            }
> > +        } else {
> >               /*
> >                * This should not really happen, but in case it happens,
> >                * we just skip the sync for this time.  After all we even
> >                * don't have the root table pointer!
> >                */
> 
> It looks to me the comment is not accurate, no root pointer is not the only
> reason for the failure of vtd_dev_to_context_entry().
> 
> >               error_report_once("%s: invalid context entry for bus 0x%x"
> > -                              " devfn 0x%x",
> > -                              __func__, pci_bus_num(vtd_as->bus),
> > -                              vtd_as->devfn);
> > -            return 0;
> 
> I'm not quite sure error_report_once() is really needed here since all
> failures has been traced.

True; I'll then consider have all of them to be error_report_once()
and drop the one here.

> 
> > +                              " devfn 0x%x", __func__,
> > +                              pci_bus_num(vtd_as->bus), vtd_as->devfn);
> >           }
> > +        return 0;
> >       }
> > -    info.domain_id = VTD_CONTEXT_ENTRY_DID(ce_cache.hi);
> > -
> > -    return vtd_page_walk(&ce_cache, addr, addr + size, &info);
> > -}
> > -
> > -static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
> > -{
> > -    return vtd_sync_shadow_page_table_range(vtd_as, NULL, 0, UINT64_MAX);
> > +    return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
> >   }
> 
> As has been discussed, this will left addr UINT64_MAX, it's better to have
> [start, end] instead of (start, range).

Hmm, this size is inclusive, so we should be fine.  Though I'll take
your advise to use start/end pair to be clearer.

Thanks!

-- 
Peter Xu