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.
Thanks! Eventually we should add some check in amdvi_get_dte(). We can do it
later.
>
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Alejandro Jimenez <alejandro.j.jime...@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.he...@amd.com>
-Vasant
