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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/2019 11:39, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
>> Sent: 03 April 2019 19:08
>> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
>> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich 
>> <JBeulich@xxxxxxxx>; Wei Liu
>> <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; Brian Woods 
>> <brian.woods@xxxxxxx>; Paul
>> Durrant <Paul.Durrant@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Brian Woods <brian.woods@xxxxxxx>
>> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>
>> 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

Except they are very very deliberate to avoid the possibility of
repeating the previous mistake, and avoid any confusion about how addr
should be interpreted.

~Andrew

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