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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 04 April 2019 11:46
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Xen-devel 
> <xen-devel@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>; Brian Woods <brian.woods@xxxxxxx>
> Subject: Re: [PATCH] amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901
> 
> 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.

The problem is that looking at 0x000000007fff8000, it doesn't immediately 
spring out at you that the top bit is 30 (although it's a bit more obvious that 
the bottom bit is 15). IMO a macro that takes the stated high and low bits and 
hands you back an inclusive mask can be more easily reasoned about. Let's see 
what others think...

  Paul

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