The DTE validation method verifies that all bits in reserved DTE fields are
unset. Update them according to the latest definition available in AMD I/O
Virtualization Technology (IOMMU) Specification - Section 2.2.2.1 Device
Table Entry Format. Remove the magic numbers and use a macro helper to
generate bitmasks covering the specified ranges for better legibility.
Note that some reserved fields specify that events are generated when they
contain non-zero bits, or checks are skipped under certain configurations.
This change only updates the reserved masks, checks for special conditions
are not yet implemented.
Cc: qemu-stable@nongnu.org
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
hw/i386/amd_iommu.c | 7 ++++---
hw/i386/amd_iommu.h | 9 ++++++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 068eeb0cae..8b97abe28c 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -848,9 +848,10 @@ static inline uint64_t amdvi_get_perms(uint64_t entry)
static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid,
uint64_t *dte)
{
- if ((dte[0] & AMDVI_DTE_LOWER_QUAD_RESERVED)
- || (dte[1] & AMDVI_DTE_MIDDLE_QUAD_RESERVED)
- || (dte[2] & AMDVI_DTE_UPPER_QUAD_RESERVED) || dte[3]) {
+ if ((dte[0] & AMDVI_DTE_QUAD0_RESERVED) ||
+ (dte[1] & AMDVI_DTE_QUAD1_RESERVED) ||
+ (dte[2] & AMDVI_DTE_QUAD2_RESERVED) ||
+ (dte[3] & AMDVI_DTE_QUAD3_RESERVED)) {
amdvi_log_illegaldevtab_error(s, devid,
s->devtab +
devid * AMDVI_DEVTAB_ENTRY_SIZE, 0);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 4c708f8d74..8d5d882a06 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -25,6 +25,8 @@
#include "hw/i386/x86-iommu.h"
#include "qom/object.h"
+#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l))
+
/* Capability registers */
#define AMDVI_CAPAB_BAR_LOW 0x04
#define AMDVI_CAPAB_BAR_HIGH 0x08
@@ -162,9 +164,10 @@
#define AMDVI_FEATURE_PC (1ULL << 9) /* Perf counters */
/* reserved DTE bits */
-#define AMDVI_DTE_LOWER_QUAD_RESERVED 0x80300000000000fc
-#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100
-#define AMDVI_DTE_UPPER_QUAD_RESERVED 0x08f0000000000000
+#define AMDVI_DTE_QUAD0_RESERVED (GENMASK64(6, 2) | GENMASK64(63, 63))
+#define AMDVI_DTE_QUAD1_RESERVED 0
+#define AMDVI_DTE_QUAD2_RESERVED GENMASK64(53, 52)
+#define AMDVI_DTE_QUAD3_RESERVED (GENMASK64(14, 0) | GENMASK64(53, 48))
/* AMDVI paging mode */
#define AMDVI_GATS_MODE (2ULL << 12)
--
2.43.5Hi Alejandro,
On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
> The DTE validation method verifies that all bits in reserved DTE fields are
> unset. Update them according to the latest definition available in AMD I/O
> Virtualization Technology (IOMMU) Specification - Section 2.2.2.1 Device
> Table Entry Format. Remove the magic numbers and use a macro helper to
> generate bitmasks covering the specified ranges for better legibility.
>
> Note that some reserved fields specify that events are generated when they
> contain non-zero bits, or checks are skipped under certain configurations.
> This change only updates the reserved masks, checks for special conditions
> are not yet implemented.
Thanks! Eventually we should add some check in amdvi_get_dte(). We can do it later.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
> hw/i386/amd_iommu.c | 7 ++++---
> hw/i386/amd_iommu.h | 9 ++++++---
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 068eeb0cae..8b97abe28c 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -848,9 +848,10 @@ static inline uint64_t amdvi_get_perms(uint64_t entry)
> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid,
> uint64_t *dte)
> {
> - if ((dte[0] & AMDVI_DTE_LOWER_QUAD_RESERVED)
> - || (dte[1] & AMDVI_DTE_MIDDLE_QUAD_RESERVED)
> - || (dte[2] & AMDVI_DTE_UPPER_QUAD_RESERVED) || dte[3]) {
> + if ((dte[0] & AMDVI_DTE_QUAD0_RESERVED) ||
> + (dte[1] & AMDVI_DTE_QUAD1_RESERVED) ||
> + (dte[2] & AMDVI_DTE_QUAD2_RESERVED) ||
> + (dte[3] & AMDVI_DTE_QUAD3_RESERVED)) {
> amdvi_log_illegaldevtab_error(s, devid,
> s->devtab +
> devid * AMDVI_DEVTAB_ENTRY_SIZE, 0);
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 4c708f8d74..8d5d882a06 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -25,6 +25,8 @@
> #include "hw/i386/x86-iommu.h"
> #include "qom/object.h"
>
> +#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l))
> +
> /* Capability registers */
> #define AMDVI_CAPAB_BAR_LOW 0x04
> #define AMDVI_CAPAB_BAR_HIGH 0x08
> @@ -162,9 +164,10 @@
> #define AMDVI_FEATURE_PC (1ULL << 9) /* Perf counters */
>
> /* reserved DTE bits */
> -#define AMDVI_DTE_LOWER_QUAD_RESERVED 0x80300000000000fc
> -#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100
> -#define AMDVI_DTE_UPPER_QUAD_RESERVED 0x08f0000000000000
> +#define AMDVI_DTE_QUAD0_RESERVED (GENMASK64(6, 2) | GENMASK64(63, 63))
> +#define AMDVI_DTE_QUAD1_RESERVED 0
> +#define AMDVI_DTE_QUAD2_RESERVED GENMASK64(53, 52)
> +#define AMDVI_DTE_QUAD3_RESERVED (GENMASK64(14, 0) | GENMASK64(53, 48))
In vIOMMU case guest is not expected to set any value in DTE[3]. So I am
wondering whether to match the spec -OR- what is expected in vIOMMU context. If
we want to go with later then it complicates as there are many other fields like
GV, etc is not expected to be used.
So since this matches the spec I think we are good for now.
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
-Vasant
Hi Alejandro,
On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
> The DTE validation method verifies that all bits in reserved DTE fields are
> unset. Update them according to the latest definition available in AMD I/O
> Virtualization Technology (IOMMU) Specification - Section 2.2.2.1 Device
> Table Entry Format. Remove the magic numbers and use a macro helper to
> generate bitmasks covering the specified ranges for better legibility.
>
> Note that some reserved fields specify that events are generated when they
> contain non-zero bits, or checks are skipped under certain configurations.
> This change only updates the reserved masks, checks for special conditions
> are not yet implemented.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
> hw/i386/amd_iommu.c | 7 ++++---
> hw/i386/amd_iommu.h | 9 ++++++---
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 068eeb0cae..8b97abe28c 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -848,9 +848,10 @@ static inline uint64_t amdvi_get_perms(uint64_t entry)
> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid,
> uint64_t *dte)
> {
> - if ((dte[0] & AMDVI_DTE_LOWER_QUAD_RESERVED)
> - || (dte[1] & AMDVI_DTE_MIDDLE_QUAD_RESERVED)
> - || (dte[2] & AMDVI_DTE_UPPER_QUAD_RESERVED) || dte[3]) {
> + if ((dte[0] & AMDVI_DTE_QUAD0_RESERVED) ||
> + (dte[1] & AMDVI_DTE_QUAD1_RESERVED) ||
> + (dte[2] & AMDVI_DTE_QUAD2_RESERVED) ||
> + (dte[3] & AMDVI_DTE_QUAD3_RESERVED)) {
> amdvi_log_illegaldevtab_error(s, devid,
> s->devtab +
> devid * AMDVI_DEVTAB_ENTRY_SIZE, 0);
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 4c708f8d74..8d5d882a06 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -25,6 +25,8 @@
> #include "hw/i386/x86-iommu.h"
> #include "qom/object.h"
>
> +#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l))
> +
qemu provides the similar macro 'MAKE_64BIT_MASK' in file
'include/qemu/bitops.h', you can use this existing macro
instead of redefining.
> /* Capability registers */
> #define AMDVI_CAPAB_BAR_LOW 0x04
> #define AMDVI_CAPAB_BAR_HIGH 0x08
> @@ -162,9 +164,10 @@
> #define AMDVI_FEATURE_PC (1ULL << 9) /* Perf counters */
>
> /* reserved DTE bits */
> -#define AMDVI_DTE_LOWER_QUAD_RESERVED 0x80300000000000fc
> -#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100
> -#define AMDVI_DTE_UPPER_QUAD_RESERVED 0x08f0000000000000
> +#define AMDVI_DTE_QUAD0_RESERVED (GENMASK64(6, 2) | GENMASK64(63, 63))
> +#define AMDVI_DTE_QUAD1_RESERVED 0
> +#define AMDVI_DTE_QUAD2_RESERVED GENMASK64(53, 52)
> +#define AMDVI_DTE_QUAD3_RESERVED (GENMASK64(14, 0) | GENMASK64(53, 48))
>
> /* AMDVI paging mode */
> #define AMDVI_GATS_MODE (2ULL << 12)
Regards
Sairaj Kodilkar
On 3/12/25 12:12 AM, Arun Kodilkar, Sairaj wrote: > Hi Alejandro, > > On 3/11/2025 8:54 PM, Alejandro Jimenez wrote: [...] >> --- a/hw/i386/amd_iommu.h >> +++ b/hw/i386/amd_iommu.h >> @@ -25,6 +25,8 @@ >> #include "hw/i386/x86-iommu.h" >> #include "qom/object.h" >> +#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l)) >> + > > qemu provides the similar macro 'MAKE_64BIT_MASK' in file > 'include/qemu/bitops.h', you can use this existing macro > instead of redefining. Hi Sairaj, I became aware of MAKE_64BIT_MASK() because you used it in your recent patch, but as you mentioned they are similar but not the same. I personally find that using bit indexes is less prone to errors since that is the same format the spec uses to define the fields. So translating a RSVD[6:2] field from the spec becomes: GENMASK64(6, 2); vs MAKE_64BIT_MASK(6, 5); The latter is more prone to off-by-one errors in my opinion, specially when you are defining lots of masks. Perhaps more importantly, I'd like to progressively convert the amd_iommu definitions to use GENMASK() and the code that retrieves bit fields to use FIELD_GET(). I am planning on later porting the generic GENMASK* definitions (from the kernel into "qemu/bitops.h", since the RISC-V IOMMU is also a consumer of GENMASK, but I am trying to keep the focus on the AMD vIOMMU for this series. Thank you, Alejandro > >> /* Capability registers */ >> #define AMDVI_CAPAB_BAR_LOW 0x04 >> #define AMDVI_CAPAB_BAR_HIGH 0x08 >> @@ -162,9 +164,10 @@ >> #define AMDVI_FEATURE_PC (1ULL << 9) /* Perf >> counters */ >> /* reserved DTE bits */ >> -#define AMDVI_DTE_LOWER_QUAD_RESERVED 0x80300000000000fc >> -#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100 >> -#define AMDVI_DTE_UPPER_QUAD_RESERVED 0x08f0000000000000 >> +#define AMDVI_DTE_QUAD0_RESERVED (GENMASK64(6, 2) | >> GENMASK64(63, 63)) >> +#define AMDVI_DTE_QUAD1_RESERVED 0 >> +#define AMDVI_DTE_QUAD2_RESERVED GENMASK64(53, 52) >> +#define AMDVI_DTE_QUAD3_RESERVED (GENMASK64(14, 0) | >> GENMASK64(53, 48)) >> /* AMDVI paging mode */ >> #define AMDVI_GATS_MODE (2ULL << 12) > > Regards > Sairaj Kodilkar
Hi , On 3/13/2025 7:53 PM, Alejandro Jimenez wrote: > > > On 3/12/25 12:12 AM, Arun Kodilkar, Sairaj wrote: >> Hi Alejandro, >> >> On 3/11/2025 8:54 PM, Alejandro Jimenez wrote: > > [...] > >>> --- a/hw/i386/amd_iommu.h >>> +++ b/hw/i386/amd_iommu.h >>> @@ -25,6 +25,8 @@ >>> #include "hw/i386/x86-iommu.h" >>> #include "qom/object.h" >>> +#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l)) >>> + >> >> qemu provides the similar macro 'MAKE_64BIT_MASK' in file >> 'include/qemu/bitops.h', you can use this existing macro >> instead of redefining. > > Hi Sairaj, > > I became aware of MAKE_64BIT_MASK() because you used it in your recent patch, > but as you mentioned they are similar but not the same. I personally find that > using bit indexes is less prone to errors since that is the same format the spec > uses to define the fields. > So translating a RSVD[6:2] field from the spec becomes: > > GENMASK64(6, 2); > vs > MAKE_64BIT_MASK(6, 5); > > The latter is more prone to off-by-one errors in my opinion, specially when you > are defining lots of masks. Perhaps more importantly, I'd like to progressively > convert the amd_iommu definitions to use GENMASK() and the code that retrieves > bit fields to use FIELD_GET(). > > I am planning on later porting the generic GENMASK* definitions (from the kernel > into "qemu/bitops.h", since the RISC-V IOMMU is also a consumer of GENMASK, but > I am trying to keep the focus on the AMD vIOMMU for this series. I like GENMASK. Its easy to read. RISCV has GENMASK_ULL. As you said, we can consider consolidating them later. -Vasant
On 3/13/2025 7:53 PM, Alejandro Jimenez wrote: > > > On 3/12/25 12:12 AM, Arun Kodilkar, Sairaj wrote: >> Hi Alejandro, >> >> On 3/11/2025 8:54 PM, Alejandro Jimenez wrote: > > [...] > >>> --- a/hw/i386/amd_iommu.h >>> +++ b/hw/i386/amd_iommu.h >>> @@ -25,6 +25,8 @@ >>> #include "hw/i386/x86-iommu.h" >>> #include "qom/object.h" >>> +#define GENMASK64(h, l) (((~0ULL) >> (63 - (h) + (l))) << (l)) >>> + >> >> qemu provides the similar macro 'MAKE_64BIT_MASK' in file >> 'include/qemu/bitops.h', you can use this existing macro >> instead of redefining. > > Hi Sairaj, > > I became aware of MAKE_64BIT_MASK() because you used it in your recent > patch, but as you mentioned they are similar but not the same. I > personally find that using bit indexes is less prone to errors since > that is the same format the spec uses to define the fields. > So translating a RSVD[6:2] field from the spec becomes: > > GENMASK64(6, 2); > vs > MAKE_64BIT_MASK(6, 5); > > The latter is more prone to off-by-one errors in my opinion, specially > when you are defining lots of masks. Perhaps more importantly, I'd like > to progressively convert the amd_iommu definitions to use GENMASK() and > the code that retrieves bit fields to use FIELD_GET(). > > I am planning on later porting the generic GENMASK* definitions (from > the kernel into "qemu/bitops.h", since the RISC-V IOMMU is also a > consumer of GENMASK, but I am trying to keep the focus on the AMD vIOMMU > for this series. > > Thank you, > Alejandro > Hi Alejandro, Yes indeed, GENMASK64() is more readable than the MAKE_64BIT_MASK(). and Since you are going to port it to generic layer, its better to use GENMASK64(). Feel free to replace existing MAKE_64BIT_MASK() in my recent patches. otherwise, I can modify it during next patch series. Regards Sairaj Kodilkar >> >>> /* Capability registers */ >>> #define AMDVI_CAPAB_BAR_LOW 0x04 >>> #define AMDVI_CAPAB_BAR_HIGH 0x08 >>> @@ -162,9 +164,10 @@ >>> #define AMDVI_FEATURE_PC (1ULL << 9) /* Perf >>> counters */ >>> /* reserved DTE bits */ >>> -#define AMDVI_DTE_LOWER_QUAD_RESERVED 0x80300000000000fc >>> -#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100 >>> -#define AMDVI_DTE_UPPER_QUAD_RESERVED 0x08f0000000000000 >>> +#define AMDVI_DTE_QUAD0_RESERVED (GENMASK64(6, 2) | >>> GENMASK64(63, 63)) >>> +#define AMDVI_DTE_QUAD1_RESERVED 0 >>> +#define AMDVI_DTE_QUAD2_RESERVED GENMASK64(53, 52) >>> +#define AMDVI_DTE_QUAD3_RESERVED (GENMASK64(14, 0) | >>> GENMASK64(53, 48)) >>> /* AMDVI paging mode */ >>> #define AMDVI_GATS_MODE (2ULL << 12) >> >> Regards >> Sairaj Kodilkar >
© 2016 - 2025 Red Hat, Inc.