Re: [Qemu-devel] [RFC v1 2/3] intel_iommu: add 256 bits qi_desc support

[Peter Xu <peterx@redhat.com>]

On Wed, Jan 30, 2019 at 01:09:12PM +0800, Yi Sun wrote:
> From: "Liu, Yi L" <yi.l.liu@intel.com>
> 
> Per Intel(R) VT-d 3.0, the qi_desc is 256 bits in Scalable
> Mode. This patch adds emulation of 256bits qi_desc.
> 
> [Yi Sun is co-developer to rebase and refine the patch.]
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>

Same here; I think you should have your s-o-b last. ;)

> ---
>  hw/i386/intel_iommu.c          | 182 +++++++++++++++++++++++++----------------
>  hw/i386/intel_iommu_internal.h |   8 +-
>  include/hw/i386/intel_iommu.h  |   1 +
>  3 files changed, 116 insertions(+), 75 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 396ac8e..3664a00 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -38,6 +38,7 @@
>  #include "trace.h"
>  
>  #define vtd_devfn_check(devfn) ((devfn & VTD_DEVFN_CHECK_MASK) ? true : false)
> +#define vtd_ecap_smts(s) ((s)->ecap & VTD_ECAP_SMTS)

I'd prefer capital letters for macros.  Your call.

>  
>  /* context entry operations */
>  #define vtd_get_ce_size(s, ce) \
> @@ -65,6 +66,9 @@
>  #define vtd_pe_get_slpt_base(pe) ((pe)->val[0] & VTD_SM_PASID_ENTRY_SLPTPTR)
>  #define vtd_pe_get_domain_id(pe) VTD_SM_PASID_ENTRY_DID((pe)->val[1])
>  
> +/* invalidation desc */
> +#define vtd_get_inv_desc_width(s) ((s)->iq_dw ? 32 : 16)

Nit: I'll prefer dropping all the "get" wordings in these macros to be
"vtd_inv_desc_width" since that "get" doesn't help much on
understanding its meanings.  But it's personal preference too.

And since you've already have the iq_dw variable - why not store the
width directly into it?  An uint8_t would suffice.

> +
>  static void vtd_address_space_refresh_all(IntelIOMMUState *s);
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
>  
> @@ -1759,6 +1763,11 @@ static void vtd_root_table_setup(IntelIOMMUState *s)
>      s->root_scalable = s->root & VTD_RTADDR_SMT;
>      s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);
>  
> +    /* if Scalable mode is not enabled, enforce iq_dw to be 16 byte */
> +    if (!s->root_scalable) {
> +        s->iq_dw = 0;

This is ok but I feel it a bit awkward to change IQ setup specifically
in root table setup.  You can simply do this:

- in vtd_init(), always set iq_dw=16.  This will cover system resets
  or IOMMU device reset, then

- only update iq_dw to 32 in vtd_mem_write() where guest specified

> +    }
> +
>      trace_vtd_reg_dmar_root(s->root, s->root_extended);
>  }
>  
> @@ -2052,7 +2061,7 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, bool en)
>      if (en) {
>          s->iq = iqa_val & VTD_IQA_IQA_MASK(s->aw_bits);
>          /* 2^(x+8) entries */
> -        s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8);
> +        s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8 - (s->iq_dw ? 1 : 0));
>          s->qi_enabled = true;
>          trace_vtd_inv_qi_setup(s->iq, s->iq_size);
>          /* Ok - report back to driver */
> @@ -2219,54 +2228,66 @@ static void vtd_handle_iotlb_write(IntelIOMMUState *s)
>  }
>  
>  /* Fetch an Invalidation Descriptor from the Invalidation Queue */
> -static bool vtd_get_inv_desc(dma_addr_t base_addr, uint32_t offset,
> +static bool vtd_get_inv_desc(IntelIOMMUState *s,
>                               VTDInvDesc *inv_desc)
>  {
> -    dma_addr_t addr = base_addr + offset * sizeof(*inv_desc);
> -    if (dma_memory_read(&address_space_memory, addr, inv_desc,
> -        sizeof(*inv_desc))) {
> -        error_report_once("Read INV DESC failed");
> -        inv_desc->lo = 0;
> -        inv_desc->hi = 0;
> +    dma_addr_t base_addr = s->iq;
> +    uint32_t offset = s->iq_head;
> +    uint32_t dw = vtd_get_inv_desc_width(s);
> +    dma_addr_t addr = base_addr + offset * dw;
> +
> +    /* init */
> +    inv_desc->val[0] = 0;
> +    inv_desc->val[1] = 0;
> +    inv_desc->val[2] = 0;
> +    inv_desc->val[3] = 0;

No need?

> +
> +    if (dma_memory_read(&address_space_memory, addr, inv_desc, dw)) {
> +        error_report_once("Read INV DESC failed.");
>          return false;
>      }
> -    inv_desc->lo = le64_to_cpu(inv_desc->lo);
> -    inv_desc->hi = le64_to_cpu(inv_desc->hi);
> +    inv_desc->val[0] = le64_to_cpu(inv_desc->val[0]);
> +    inv_desc->val[1] = le64_to_cpu(inv_desc->val[1]);
> +    inv_desc->val[2] = le64_to_cpu(inv_desc->val[2]);
> +    inv_desc->val[3] = le64_to_cpu(inv_desc->val[3]);

Similar comments here on VTDInvDesc - you can keep the hi/lo fields,
but instead you can define the val[4] into the union too.  Then:

  if (dw == 16) {
    inv_desc->lo = ...
    inv_desc->hi = ...
  } else {
    inv_desc->val[0] = ...
    inv_desc->val[1] = ...
    inv_desc->val[2] = ...
    inv_desc->val[3] = ...
  }

Then this patch can be greatly simplified too since you don't need to
switch VTDInvDesc.{lo|hi} to val[0|1].

[...]

> @@ -2525,7 +2558,7 @@ static void vtd_handle_iqt_write(IntelIOMMUState *s)
>  {
>      uint64_t val = vtd_get_quad_raw(s, DMAR_IQT_REG);
>  
> -    s->iq_tail = VTD_IQT_QT(val);
> +    s->iq_tail = VTD_IQT_QT(s->iq_dw, val);

You can check against bit 4 when iq_dw==32 and report error otherwise.
That's explicitly mentioned by the spec (chap 10.4.22).

>      trace_vtd_inv_qi_tail(s->iq_tail);
>  
>      if (s->qi_enabled && !(vtd_get_long_raw(s, DMAR_FSTS_REG) & VTD_FSTS_IQE)) {

[...]


Regards,

-- 
Peter Xu