| Message ID | 1554314899-16357-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901 | expand |
> -----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
On 04/04/2019 11:39, Paul Durrant wrote: >> -----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 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
> -----Original Message----- > From: Andrew Cooper > Sent: 04 April 2019 11:46 > To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-devel@lists.xen.org> > Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne > <roger.pau@citrix.com>; Brian Woods <brian.woods@amd.com> > 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@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 > > 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
>>> On 03.04.19 at 20:08, <andrew.cooper3@citrix.com> wrote: > +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) | I'm afraid there's another bug here: gcc, in its default mode at least, doesn't promote bit field values to their declared types. That is, the first of the shifts will truncate the high 20 bits, as it gets carried out as a 32-bit operation. The expression result then also is of type signed int, i.e. it'll get sign-extended to fill the uint64_t. > +static void dte_set_gcr3_table(struct amd_iommu_dte *dte, uint16_t dom_id, > + uint64_t addr, uint8_t gv, uint8_t glx) Perhaps better paddr_t and also bool for gv? > +{ > + /* 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); Like Paul I'm not in favor of such many digit constants, where one always has to actually count digits to verify / understand the actual values. But in the end it's up to the maintainers to judge. > @@ -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; PFN_DOWN() or paddr_to_pfn()? > @@ -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); pfn_to_paddr()? Jan
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); + + 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)) -#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);
"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(-)