Re: [PATCH] amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 03 April 2019 19:08
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu
> <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Brian Woods <brian.woods@amd.com>; Paul
> Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH] amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901
> 
> "amd-iommu: use a bitfield for DTE" renamed iommu_dte_set_guest_cr3()'s gcr3
> parameter to gcr3_mfn but ended up with an off-by-PAGE_SIZE error when
> extracting bits from the address.
> 
> First of all, get_guest_cr3_from_dte() and iommu_dte_set_guest_cr3()
> are (almost) getters and setters for the same field, so should live together.
> 
> Rename them to dte_{get,set}_gcr3_table() to specifically avoid 'guest_cr3' in
> the name.  This field actually points to a table in memory containing an array
> of guest CR3 values.  As these functions are used for different logical
> indirections, they shouldn't use gfn/mfn terminology for their parameters.
> Switch them to use straight uint64_t full addresses.
> 
> Finally, correct the dte_set_gcr3_table() to use the proper shift.  Express
> the shifts visually using MASK_EXTR() and literal masks, which IMO is the
> clearest way to express the logic.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Brian Woods <brian.woods@amd.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> 
> This code is unreachable, so completely untestable, but I think the end result
> is better than it was previously.
> ---
>  xen/drivers/passthrough/amd/iommu_guest.c     | 32 +++++++++++++++++++++++----
>  xen/drivers/passthrough/amd/iommu_map.c       | 21 ------------------
>  xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  2 --
>  3 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 328e750..4691e8f 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -76,10 +76,34 @@ static void guest_iommu_disable(struct guest_iommu *iommu)
>      iommu->enabled = 0;
>  }
> 
> -static uint64_t get_guest_cr3_from_dte(struct amd_iommu_dte *dte)
> +/*
> + * The Guest CR3 Table is a table written by the guest kernel, pointing at
> + * gCR3 values for PASID transactions to use.  The Device Table Entry points
> + * at a system physical address.
> + *
> + * However, these helpers deliberately use untyped parameters without
> + * reference to gfn/mfn because they are used both for programming the real
> + * IOMMU, and interpreting a guests programming of its vIOMMU.
> + */
> +static uint64_t dte_get_gcr3_table(const struct amd_iommu_dte *dte)
>  {
>      return ((dte->gcr3_trp_51_31 << 31) | (dte->gcr3_trp_30_15 << 15) |
> -            (dte->gcr3_trp_14_12 << 12)) >> PAGE_SHIFT;
> +            (dte->gcr3_trp_14_12 << 12));
> +}
> +
> +static void dte_set_gcr3_table(struct amd_iommu_dte *dte, uint16_t dom_id,
> +                               uint64_t addr, uint8_t gv, uint8_t glx)
> +{
> +    /* I bit must be set when gcr3 is enabled */
> +    dte->i = 1;
> +
> +    dte->gcr3_trp_14_12 = MASK_EXTR(addr, 0x0000000000007000ull);
> +    dte->gcr3_trp_30_15 = MASK_EXTR(addr, 0x000000007fff8000ull);
> +    dte->gcr3_trp_51_31 = MASK_EXTR(addr, 0x000fffff80000000ull);

I'm not a fan of the big hex values so I'd prefer to keep...

> +
> +    dte->domain_id = dom_id;
> +    dte->glx = glx;
> +    dte->gv = gv;
>  }
> 
>  static unsigned int host_domid(struct domain *d, uint64_t g_domid)
> @@ -399,7 +423,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>      gdte = &dte_base[gbdf % (PAGE_SIZE / sizeof(struct amd_iommu_dte))];
> 
>      gdom_id = gdte->domain_id;
> -    gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    gcr3_gfn = dte_get_gcr3_table(gdte) >> PAGE_SHIFT;
>      glx = gdte->glx;
>      gv = gdte->gv;
> 
> @@ -429,7 +453,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>      mdte = &dte_base[req_id];
> 
>      spin_lock_irqsave(&iommu->lock, flags);
> -    iommu_dte_set_guest_cr3(mdte, hdom_id, gcr3_mfn, gv, glx);
> +    dte_set_gcr3_table(mdte, hdom_id, gcr3_mfn << PAGE_SHIFT, gv, glx);
> 
>      amd_iommu_flush_device(iommu, req_id);
>      spin_unlock_irqrestore(&iommu->lock, flags);
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index cbf00e9..32a8fdc 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -138,27 +138,6 @@ void __init iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
>      dte->ex = ivrs_dev->dte_allow_exclusion;
>  }
> 
> -void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
> -                             uint64_t gcr3_mfn, uint8_t gv, uint8_t glx)
> -{
> -#define GCR3_MASK(hi, lo) (((1ul << ((hi) + 1)) - 1) & ~((1ul << (lo)) - 1))

... this macro to generate the mask values for MASK_EXTR.

  Paul

> -#define GCR3_SHIFT(lo) ((lo) - PAGE_SHIFT)
> -
> -    /* I bit must be set when gcr3 is enabled */
> -    dte->i = 1;
> -
> -    dte->gcr3_trp_14_12 = (gcr3_mfn & GCR3_MASK(14, 12)) >> GCR3_SHIFT(12);
> -    dte->gcr3_trp_30_15 = (gcr3_mfn & GCR3_MASK(30, 15)) >> GCR3_SHIFT(15);
> -    dte->gcr3_trp_51_31 = (gcr3_mfn & GCR3_MASK(51, 31)) >> GCR3_SHIFT(31);
> -
> -    dte->domain_id = dom_id;
> -    dte->glx = glx;
> -    dte->gv = gv;
> -
> -#undef GCR3_SHIFT
> -#undef GCR3_MASK
> -}
> -
>  /* Walk io page tables and build level page tables if necessary
>   * {Re, un}mapping super page frames causes re-allocation of io
>   * page tables.
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-
> proto.h
> index e0d5d23..c6179dc 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -79,8 +79,6 @@ void amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
>  				   uint8_t paging_mode, uint8_t valid);
>  void iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
>                                  struct ivrs_mappings *ivrs_dev);
> -void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
> -                             uint64_t gcr3_mfn, uint8_t gv, uint8_t glx);
> 
>  /* send cmd to iommu */
>  void amd_iommu_flush_all_pages(struct domain *d);
> --
> 2.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel