| Message ID | 20240618183128.1981751-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD/IOMMU: Improve register_iommu_exclusion_range() | expand |
On 18.06.2024 20:31, Andrew Cooper wrote: > * Use 64bit accesses instead of 32bit accesses > * Simplify the constant names > * Pull base into a local variable to avoid it being reloaded because of the > memory clobber in writeq(). > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > > RFC. This is my proposed way of cleaning up the whole IOMMU file. The > diffstat speaks for itself. Absolutely. > I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are > permitted: > > 3.4 IOMMU MMIO Registers: > > Software access to IOMMU registers may not be larger than 64 bits. Accesses > must be aligned to the size of the access and the size in bytes must be a > power of two. Software may use accesses as small as one byte. I take it that the use of 32-bit writes was because of the past need also work in a 32-bit hypervisor, not because of perceived restrictions by the spec. > --- a/xen/drivers/passthrough/amd/iommu-defs.h > +++ b/xen/drivers/passthrough/amd/iommu-defs.h > @@ -338,22 +338,10 @@ union amd_iommu_control { > }; > > /* Exclusion Register */ > -#define IOMMU_EXCLUSION_BASE_LOW_OFFSET 0x20 > -#define IOMMU_EXCLUSION_BASE_HIGH_OFFSET 0x24 > -#define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET 0x28 > -#define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET 0x2C > -#define IOMMU_EXCLUSION_BASE_LOW_MASK 0xFFFFF000U > -#define IOMMU_EXCLUSION_BASE_LOW_SHIFT 12 > -#define IOMMU_EXCLUSION_BASE_HIGH_MASK 0xFFFFFFFFU > -#define IOMMU_EXCLUSION_BASE_HIGH_SHIFT 0 > -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK 0x00000001U > -#define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT 0 > -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK 0x00000002U > -#define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT 1 > -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK 0xFFFFF000U > -#define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT 12 > -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK 0xFFFFFFFFU > -#define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT 0 > +#define IOMMU_MMIO_EXCLUSION_BASE 0x20 > +#define EXCLUSION_RANGE_ENABLE (1 << 0) > +#define EXCLUSION_ALLOW_ALL (1 << 1) > +#define IOMMU_MMIO_EXCLUSION_LIMIT 0x28 Just one question here: Previously you suggested we switch to bitfields for anything like this, and we've already done so with e.g. union amd_iommu_control and union amd_iommu_ext_features. IOW I wonder if we wouldn't better strive to be consistent in this regard. Or if not, what the (written or unwritten) guidelines are when to use which approach. Jan
On 19.06.2024 09:45, Jan Beulich wrote: > On 18.06.2024 20:31, Andrew Cooper wrote: >> I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are >> permitted: >> >> 3.4 IOMMU MMIO Registers: >> >> Software access to IOMMU registers may not be larger than 64 bits. Accesses >> must be aligned to the size of the access and the size in bytes must be a >> power of two. Software may use accesses as small as one byte. > > I take it that the use of 32-bit writes was because of the past need > also work in a 32-bit hypervisor, not because of perceived restrictions > by the spec. In fact it looks like we're already halfway through converting to writeq(). Jan
On 19/06/2024 8:45 am, Jan Beulich wrote: > On 18.06.2024 20:31, Andrew Cooper wrote: >> * Use 64bit accesses instead of 32bit accesses >> * Simplify the constant names >> * Pull base into a local variable to avoid it being reloaded because of the >> memory clobber in writeq(). >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> >> RFC. This is my proposed way of cleaning up the whole IOMMU file. The >> diffstat speaks for itself. > Absolutely. > >> I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are >> permitted: >> >> 3.4 IOMMU MMIO Registers: >> >> Software access to IOMMU registers may not be larger than 64 bits. Accesses >> must be aligned to the size of the access and the size in bytes must be a >> power of two. Software may use accesses as small as one byte. > I take it that the use of 32-bit writes was because of the past need > also work in a 32-bit hypervisor, not because of perceived restrictions > by the spec. I recall having problems getting writeq() acked in the past, even after we'd dropped 32bit. But this is the first time that I've positively found anything in the spec saying that 64bit accesses are ok. > >> --- a/xen/drivers/passthrough/amd/iommu-defs.h >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h >> @@ -338,22 +338,10 @@ union amd_iommu_control { >> }; >> >> /* Exclusion Register */ >> -#define IOMMU_EXCLUSION_BASE_LOW_OFFSET 0x20 >> -#define IOMMU_EXCLUSION_BASE_HIGH_OFFSET 0x24 >> -#define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET 0x28 >> -#define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET 0x2C >> -#define IOMMU_EXCLUSION_BASE_LOW_MASK 0xFFFFF000U >> -#define IOMMU_EXCLUSION_BASE_LOW_SHIFT 12 >> -#define IOMMU_EXCLUSION_BASE_HIGH_MASK 0xFFFFFFFFU >> -#define IOMMU_EXCLUSION_BASE_HIGH_SHIFT 0 >> -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK 0x00000001U >> -#define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT 0 >> -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK 0x00000002U >> -#define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT 1 >> -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK 0xFFFFF000U >> -#define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT 12 >> -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK 0xFFFFFFFFU >> -#define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT 0 >> +#define IOMMU_MMIO_EXCLUSION_BASE 0x20 >> +#define EXCLUSION_RANGE_ENABLE (1 << 0) >> +#define EXCLUSION_ALLOW_ALL (1 << 1) >> +#define IOMMU_MMIO_EXCLUSION_LIMIT 0x28 > Just one question here: Previously you suggested we switch to bitfields > for anything like this, and we've already done so with e.g. > union amd_iommu_control and union amd_iommu_ext_features. IOW I wonder > if we wouldn't better strive to be consistent in this regard. Or if not, > what the (written or unwritten) guidelines are when to use which > approach. We've got two very different kinds of things here. The device table/etc are in-memory WB datastructure which we're interpreting and editing routinely. It's completely full of bits and small fields, and letting the compiler do the hard work there is preferable; certainly in terms of legibility. This example is an MMIO register (in a bar on the IOMMU PCI device, even though we find the address in the IVRS). We set it up once at boot and don't touch it afterwards. So while we could make a struct for it, we'd still need to get it into a form that we can writeq(), and that's more code than the single case were we need to put two metadata bits into an address. ~Andrew
On 19.06.2024 18:22, Andrew Cooper wrote: > On 19/06/2024 8:45 am, Jan Beulich wrote: >> On 18.06.2024 20:31, Andrew Cooper wrote: >>> * Use 64bit accesses instead of 32bit accesses >>> * Simplify the constant names >>> * Pull base into a local variable to avoid it being reloaded because of the >>> memory clobber in writeq(). >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> >>> RFC. This is my proposed way of cleaning up the whole IOMMU file. The >>> diffstat speaks for itself. >> Absolutely. >> >>> I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are >>> permitted: >>> >>> 3.4 IOMMU MMIO Registers: >>> >>> Software access to IOMMU registers may not be larger than 64 bits. Accesses >>> must be aligned to the size of the access and the size in bytes must be a >>> power of two. Software may use accesses as small as one byte. >> I take it that the use of 32-bit writes was because of the past need >> also work in a 32-bit hypervisor, not because of perceived restrictions >> by the spec. > > I recall having problems getting writeq() acked in the past, even after > we'd dropped 32bit. That's odd, as per my subsequent reply. > But this is the first time that I've positively found anything in the > spec saying that 64bit accesses are ok. > >>> --- a/xen/drivers/passthrough/amd/iommu-defs.h >>> +++ b/xen/drivers/passthrough/amd/iommu-defs.h >>> @@ -338,22 +338,10 @@ union amd_iommu_control { >>> }; >>> >>> /* Exclusion Register */ >>> -#define IOMMU_EXCLUSION_BASE_LOW_OFFSET 0x20 >>> -#define IOMMU_EXCLUSION_BASE_HIGH_OFFSET 0x24 >>> -#define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET 0x28 >>> -#define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET 0x2C >>> -#define IOMMU_EXCLUSION_BASE_LOW_MASK 0xFFFFF000U >>> -#define IOMMU_EXCLUSION_BASE_LOW_SHIFT 12 >>> -#define IOMMU_EXCLUSION_BASE_HIGH_MASK 0xFFFFFFFFU >>> -#define IOMMU_EXCLUSION_BASE_HIGH_SHIFT 0 >>> -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK 0x00000001U >>> -#define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT 0 >>> -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK 0x00000002U >>> -#define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT 1 >>> -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK 0xFFFFF000U >>> -#define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT 12 >>> -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK 0xFFFFFFFFU >>> -#define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT 0 >>> +#define IOMMU_MMIO_EXCLUSION_BASE 0x20 >>> +#define EXCLUSION_RANGE_ENABLE (1 << 0) >>> +#define EXCLUSION_ALLOW_ALL (1 << 1) >>> +#define IOMMU_MMIO_EXCLUSION_LIMIT 0x28 >> Just one question here: Previously you suggested we switch to bitfields >> for anything like this, and we've already done so with e.g. >> union amd_iommu_control and union amd_iommu_ext_features. IOW I wonder >> if we wouldn't better strive to be consistent in this regard. Or if not, >> what the (written or unwritten) guidelines are when to use which >> approach. > > We've got two very different kinds of things here. > > The device table/etc are in-memory WB datastructure which we're > interpreting and editing routinely. It's completely full of bits and > small fields, and letting the compiler do the hard work there is > preferable; certainly in terms of legibility. And it was specifically not the DTE I used as example in my reply, ... > This example is an MMIO register (in a bar on the IOMMU PCI device, even > though we find the address in the IVRS). We set it up once at boot and > don't touch it afterwards. ... but other MMIO registers. > So while we could make a struct for it, we'd still need to get it into a > form that we can writeq(), and that's more code than the single case > were we need to put two metadata bits into an address. See those other examples, which are usable with writeq() by way of their "raw" fields. Jan
diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h index c145248f9af1..9cf509b1f78b 100644 --- a/xen/drivers/passthrough/amd/iommu-defs.h +++ b/xen/drivers/passthrough/amd/iommu-defs.h @@ -338,22 +338,10 @@ union amd_iommu_control { }; /* Exclusion Register */ -#define IOMMU_EXCLUSION_BASE_LOW_OFFSET 0x20 -#define IOMMU_EXCLUSION_BASE_HIGH_OFFSET 0x24 -#define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET 0x28 -#define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET 0x2C -#define IOMMU_EXCLUSION_BASE_LOW_MASK 0xFFFFF000U -#define IOMMU_EXCLUSION_BASE_LOW_SHIFT 12 -#define IOMMU_EXCLUSION_BASE_HIGH_MASK 0xFFFFFFFFU -#define IOMMU_EXCLUSION_BASE_HIGH_SHIFT 0 -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK 0x00000001U -#define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT 0 -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK 0x00000002U -#define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT 1 -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK 0xFFFFF000U -#define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT 12 -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK 0xFFFFFFFFU -#define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT 0 +#define IOMMU_MMIO_EXCLUSION_BASE 0x20 +#define EXCLUSION_RANGE_ENABLE (1 << 0) +#define EXCLUSION_ALLOW_ALL (1 << 1) +#define IOMMU_MMIO_EXCLUSION_LIMIT 0x28 /* Extended Feature Register */ #define IOMMU_EXT_FEATURE_MMIO_OFFSET 0x30 diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 6c0dc2d5cb69..bcf1903e716e 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -223,40 +223,19 @@ static void set_iommu_command_buffer_control(struct amd_iommu *iommu, static void register_iommu_exclusion_range(struct amd_iommu *iommu) { - u32 addr_lo, addr_hi; - u32 entry; - - addr_lo = iommu->exclusion_limit; - addr_hi = iommu->exclusion_limit >> 32; - - set_field_in_reg_u32((u32)addr_hi, 0, - IOMMU_EXCLUSION_LIMIT_HIGH_MASK, - IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT, &entry); - writel(entry, iommu->mmio_base+IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET); - - set_field_in_reg_u32((u32)addr_lo >> PAGE_SHIFT, 0, - IOMMU_EXCLUSION_LIMIT_LOW_MASK, - IOMMU_EXCLUSION_LIMIT_LOW_SHIFT, &entry); - writel(entry, iommu->mmio_base+IOMMU_EXCLUSION_LIMIT_LOW_OFFSET); - - addr_lo = iommu->exclusion_base & DMA_32BIT_MASK; - addr_hi = iommu->exclusion_base >> 32; + void *__iomem base = iommu->mmio_base; + uint64_t val; - entry = 0; - iommu_set_addr_hi_to_reg(&entry, addr_hi); - writel(entry, iommu->mmio_base+IOMMU_EXCLUSION_BASE_HIGH_OFFSET); - - entry = 0; - iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT); + /* Exclusion Limit */ + val = iommu->exclusion_limit & PAGE_MASK; + writeq(val, base + IOMMU_MMIO_EXCLUSION_LIMIT); - set_field_in_reg_u32(iommu->exclusion_allow_all, entry, - IOMMU_EXCLUSION_ALLOW_ALL_MASK, - IOMMU_EXCLUSION_ALLOW_ALL_SHIFT, &entry); + /* Exclusion Base, inc control bits. */ + val = ((iommu->exclusion_base & PAGE_MASK) | + (iommu->exclusion_allow_all ? EXCLUSION_ALLOW_ALL : 0) | + (iommu->exclusion_enable ? EXCLUSION_RANGE_ENABLE : 0)); - set_field_in_reg_u32(iommu->exclusion_enable, entry, - IOMMU_EXCLUSION_RANGE_ENABLE_MASK, - IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT, &entry); - writel(entry, iommu->mmio_base+IOMMU_EXCLUSION_BASE_LOW_OFFSET); + writeq(val, base + IOMMU_MMIO_EXCLUSION_BASE); } static void cf_check set_iommu_event_log_control(
* Use 64bit accesses instead of 32bit accesses * Simplify the constant names * Pull base into a local variable to avoid it being reloaded because of the memory clobber in writeq(). No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> RFC. This is my proposed way of cleaning up the whole IOMMU file. The diffstat speaks for itself. I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are permitted: 3.4 IOMMU MMIO Registers: Software access to IOMMU registers may not be larger than 64 bits. Accesses must be aligned to the size of the access and the size in bytes must be a power of two. Software may use accesses as small as one byte. If we want to further simplify the logic, we could reject non-page-aligned base/limits when parsing IVRS. Also, these registers don't exist in newer AMD systems: When the system is SNP-enabled, the contents of the Exclusion range base address field are locked and re- purposed as the Completion store base address field. This contains bits [51:12] of the 4Kbyte-aligned base address that defines the starting address range that host COMPLETION_WAIT stores may target I take this to mean the writes are discarded. --- xen/drivers/passthrough/amd/iommu-defs.h | 20 +++--------- xen/drivers/passthrough/amd/iommu_init.c | 41 ++++++------------------ 2 files changed, 14 insertions(+), 47 deletions(-)