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

[Peter Xu]

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?

> +                 * 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.

> +                 * 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 "!="?

> +            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