[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 20.11.2020 15:19, Andrew Cooper wrote:
> "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.

All of this still looks to belong to "First of all ..." - did you
mean to have more in here, but forgot to actually put it in?

> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -68,11 +68,39 @@ 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 (((uint64_t)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, bool gv, uint8_t glx)
> +{
> +#define GCR3_MASK(hi, lo) (((1ul << ((hi) + 1)) - 1) & ~((1ul << (lo)) - 1))
> +
> +    /* I bit must be set when gcr3 is enabled */
> +    dte->i = true;
> +
> +    dte->gcr3_trp_14_12 = MASK_EXTR(addr, GCR3_MASK(14, 12));
> +    dte->gcr3_trp_30_15 = MASK_EXTR(addr, GCR3_MASK(30, 15));
> +    dte->gcr3_trp_51_31 = MASK_EXTR(addr, GCR3_MASK(51, 31));
> +
> +    dte->domain_id = dom_id;
> +    dte->glx = glx;
> +    dte->gv = gv;
> +
> +#undef GCR3_MASK
>  }

I realize the question is somewhat unrelated, but aren't we updating
a live DTE here? If so, are there no ordering requirements between the
writes? Might be worth putting in barrier(s) right on this occasion.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.