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
