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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/11/2020 14:32, Jan Beulich wrote:
> 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?

No - I deleted the bit which has caused this to be blocked on minutia
for nearly 2 years.l

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

I don't know.  Honestly, its not relevant either as this is code motion.

This entire file is full of security holes.  None of it is fit for use
in its current form.

~Andrew