RE: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Friday, April 3, 2020 10:46 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context 
> cache
> invalidation
> 
> On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote:
> > This patch replays guest pasid bindings after context cache
> > invalidation. This is a behavior to ensure safety. Actually,
> > programmer should issue pasid cache invalidation with proper
> > granularity after issuing a context cache invalidation.
> >
> > Cc: Kevin Tian <address@hidden>
> > Cc: Jacob Pan <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > Cc: Yi Sun <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Cc: Eduardo Habkost <address@hidden>
> > Signed-off-by: Liu Yi L <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c          | 51
> ++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/intel_iommu_internal.h |  6 ++++-
> >  hw/i386/trace-events           |  1 +
> >  3 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index d87f608..883aeac 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -68,6 +68,10 @@ static void
> vtd_address_space_refresh_all(IntelIOMMUState *s);
> >  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
> >
> >  static void vtd_pasid_cache_reset(IntelIOMMUState *s);
> > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > +                                 VTDPASIDCacheInfo *pc_info);
> > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> > +                                  VTDBus *vtd_bus, uint16_t devfn);
> >
> >  static void vtd_panic_require_caching_mode(void)
> >  {
> > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState
> *s)
> >
> >  static void vtd_context_global_invalidate(IntelIOMMUState *s)
> >  {
> > +    VTDPASIDCacheInfo pc_info;
> > +
> >      trace_vtd_inv_desc_cc_global();
> > +
> >      /* Protects context cache */
> >      vtd_iommu_lock(s);
> >      s->context_cache_gen++;
> > @@ -1870,6 +1877,9 @@ static void
> vtd_context_global_invalidate(IntelIOMMUState *s)
> >       * VT-d emulation codes.
> >       */
> >      vtd_iommu_replay_all(s);
> > +
> > +    pc_info.flags = VTD_PASID_CACHE_GLOBAL;
> > +    vtd_pasid_cache_sync(s, &pc_info);
> >  }
> >
> >  /**
> > @@ -2005,6 +2015,22 @@ static void
> vtd_context_device_invalidate(IntelIOMMUState *s,
> >                   * happened.
> >                   */
> >                  vtd_sync_shadow_page_table(vtd_as);
> > +                /*
> > +                 * Per spec, context flush should also followed with PASID
> > +                 * cache and iotlb flush. Regards to a device selective
> > +                 * context cache invalidation:
> 
> If context entry flush should also follow another pasid cache flush,
> then this is still needed?  Shouldn't the pasid flush do the same
> thing again?

yes, but how about guest software failed to follow it? It will do
the same thing when pasid cache flush comes. But this only happens
for the rid2pasid case (the IOVA page table).

> > +                 * if (emaulted_device)
> > +                 *    modify the pasid cache gen and pasid-based iotlb gen
> > +                 *    value (will be added in following patches)
> 
> Let's avoid using "following patches" because it'll be helpless after
> merged.  Also, the pasid cache gen is gone.

got it. will modify the description here.

> > +                 * else if (assigned_device)
> > +                 *    check if the device has been bound to any pasid
> > +                 *    invoke pasid_unbind regards to each bound pasid
> > +                 * Here, we have vtd_pasid_cache_devsi() to invalidate 
> > pasid
> > +                 * caches, while for piotlb in QEMU, we don't have it yet, 
> > so
> > +                 * no handling. For assigned device, host iommu driver 
> > would
> > +                 * flush piotlb when a pasid unbind is pass down to it.
> > +                 */
> > +                 vtd_pasid_cache_devsi(s, vtd_bus, devfn_it);
> >              }
> >          }
> >      }
> > @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, 
> > gpointer
> value,
> >          /* Fall through */
> >      case VTD_PASID_CACHE_GLOBAL:
> >          break;
> > +    case VTD_PASID_CACHE_DEVSI:
> > +        if (pc_info->vtd_bus != vtd_bus ||
> > +            pc_info->devfn == devfn) {
> 
> Do you mean "!="?

exactly. thanks for catching it.

Regards,
Yi Liu

> > +            return false;
> > +        }
> > +        break;
> >      default:
> >          error_report("invalid pc_info->flags");
> >          abort();
> > @@ -2827,6 +2859,11 @@ static void
> vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> >          walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK;
> >          /* loop all assigned devices */
> >          break;
> > +    case VTD_PASID_CACHE_DEVSI:
> > +        walk_info.vtd_bus = pc_info->vtd_bus;
> > +        walk_info.devfn = pc_info->devfn;
> > +        vtd_replay_pasid_bind_for_dev(s, start, end, &walk_info);
> > +        return;
> >      case VTD_PASID_CACHE_FORCE_RESET:
> >          /* For force reset, no need to go further replay */
> >          return;
> > @@ -2912,6 +2949,20 @@ static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> >      vtd_iommu_unlock(s);
> >  }
> >
> > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> > +                                  VTDBus *vtd_bus, uint16_t devfn)
> > +{
> > +    VTDPASIDCacheInfo pc_info;
> > +
> > +    trace_vtd_pasid_cache_devsi(devfn);
> > +
> > +    pc_info.flags = VTD_PASID_CACHE_DEVSI;
> > +    pc_info.vtd_bus = vtd_bus;
> > +    pc_info.devfn = devfn;
> > +
> > +    vtd_pasid_cache_sync(s, &pc_info);
> > +}
> > +
> >  /**
> >   * Caller of this function should hold iommu_lock
> >   */
> > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index b9e48ab..9122601 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -529,14 +529,18 @@ struct VTDPASIDCacheInfo {
> >  #define VTD_PASID_CACHE_GLOBAL         (1ULL << 1)
> >  #define VTD_PASID_CACHE_DOMSI          (1ULL << 2)
> >  #define VTD_PASID_CACHE_PASIDSI        (1ULL << 3)
> > +#define VTD_PASID_CACHE_DEVSI          (1ULL << 4)
> >      uint32_t flags;
> >      uint16_t domain_id;
> >      uint32_t pasid;
> > +    VTDBus *vtd_bus;
> > +    uint16_t devfn;
> >  };
> >  #define VTD_PASID_CACHE_INFO_MASK    (VTD_PASID_CACHE_FORCE_RESET |
> \
> >                                        VTD_PASID_CACHE_GLOBAL  | \
> >                                        VTD_PASID_CACHE_DOMSI  | \
> > -                                      VTD_PASID_CACHE_PASIDSI)
> > +                                      VTD_PASID_CACHE_PASIDSI | \
> > +                                      VTD_PASID_CACHE_DEVSI)
> >  typedef struct VTDPASIDCacheInfo VTDPASIDCacheInfo;
> >
> >  /* PASID Table Related Definitions */
> > diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> > index 60d20c1..3853fa8 100644
> > --- a/hw/i386/trace-events
> > +++ b/hw/i386/trace-events
> > @@ -26,6 +26,7 @@ vtd_pasid_cache_gsi(void) ""
> >  vtd_pasid_cache_reset(void) ""
> >  vtd_pasid_cache_dsi(uint16_t domain) "Domian slective PC invalidation 
> > domain
> 0x%"PRIx16
> >  vtd_pasid_cache_psi(uint16_t domain, uint32_t pasid) "PASID slective PC
> invalidation domain 0x%"PRIx16" pasid 0x%"PRIx32
> > +vtd_pasid_cache_devsi(uint16_t devfn) "Dev selective PC invalidation dev:
> 0x%"PRIx16
> >  vtd_re_not_present(uint8_t bus) "Root entry bus %"PRIu8" not present"
> >  vtd_ce_not_present(uint8_t bus, uint8_t devfn) "Context entry bus %"PRIu8"
> devfn %"PRIu8" not present"
> >  vtd_iotlb_page_hit(uint16_t sid, uint64_t addr, uint64_t slpte, uint16_t 
> > domain)
> "IOTLB page hit sid 0x%"PRIx16" iova 0x%"PRIx64" slpte 0x%"PRIx64" domain
> 0x%"PRIx16
> > --
> > 2.7.4
> >
> 
> --
> Peter Xu