Hi,
On 5/2/25 4:15 AM, Alejandro Jimenez wrote:
> Caution: External email. Do not open attachments or click links, unless this
> email comes from a known sender and you know the content is safe.
>
>
> Extracting the DTE from a given AMDVIAddressSpace pointer structure is a
> common operation required for syncing the shadow page tables. Implement a
> helper to do it and check for common error conditions.
>
> Signed-off-by: Alejandro Jimenez <alejandro.j.jime...@oracle.com>
> ---
> hw/i386/amd_iommu.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index dff6f04c8651..5322a614f5d6 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -77,6 +77,18 @@ typedef struct AMDVIIOTLBEntry {
> uint64_t page_mask; /* physical page size */
> } AMDVIIOTLBEntry;
>
> +/*
> + * These 'fault' reasons have an overloaded meaning since they are not only
> + * intended for describing reasons that generate an IO_PAGE_FAULT as per the
> AMD
> + * IOMMU specification, but are also used to signal internal errors in the
> + * emulation code.
> + */
> +typedef enum AMDVIFaultReason {
> + AMDVI_FR_DTE_RTR_ERR = 1, /* Failure to retrieve DTE */
> + AMDVI_FR_DTE_V, /* DTE[V] = 0 */
> + AMDVI_FR_DTE_TV, /* DTE[TV] = 0 */
> +} AMDVIFaultReason;
> +
> uint64_t amdvi_extended_feature_register(AMDVIState *s)
> {
> uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
> @@ -492,6 +504,28 @@ static inline uint64_t amdvi_get_pte_entry(AMDVIState
> *s, uint64_t pte_addr,
> return pte;
> }
>
> +static int amdvi_as_to_dte(AMDVIAddressSpace *as, uint64_t *dte)
> +{
> + uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> + AMDVIState *s = as->iommu_state;
> +
> + if (!amdvi_get_dte(s, devid, dte)) {
> + /* Unable to retrieve DTE for devid */
> + return -AMDVI_FR_DTE_RTR_ERR;
> + }
> +
> + if (!(dte[0] & AMDVI_DEV_VALID)) {
> + /* DTE[V] not set, address is passed untranslated for devid */
> + return -AMDVI_FR_DTE_V;
> + }
> +
> + if (!(dte[0] & AMDVI_DEV_TRANSLATION_VALID)) {
> + /* DTE[TV] not set, host page table not valid for devid */
> + return -AMDVI_FR_DTE_TV;
> + }
> + return 0;
> +}
> +
I'm not sure the new amdvi_as_to_dte() helper adds much. It just wraps a
few checks and makes it harder to report faults properly in the future.
Is there a reason this couldn't be handled inline?
> /* log error without aborting since linux seems to be using reserved bits */
> static void amdvi_inval_devtab_entry(AMDVIState *s, uint64_t *cmd)
> {
> @@ -1024,6 +1058,7 @@ static void amdvi_do_translate(AMDVIAddressSpace *as,
> hwaddr addr,
> uint16_t devid = PCI_BUILD_BDF(as->bus_num, as->devfn);
> AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, devid);
> uint64_t entry[4];
> + int dte_ret;
>
> if (iotlb_entry) {
> trace_amdvi_iotlb_hit(PCI_BUS_NUM(devid), PCI_SLOT(devid),
> @@ -1035,13 +1070,13 @@ static void amdvi_do_translate(AMDVIAddressSpace *as,
> hwaddr addr,
> return;
> }
>
> - if (!amdvi_get_dte(s, devid, entry)) {
> - return;
> - }
> + dte_ret = amdvi_as_to_dte(as, entry);
>
> - /* devices with V = 0 are not translated */
> - if (!(entry[0] & AMDVI_DEV_VALID)) {
> + if (dte_ret == -AMDVI_FR_DTE_V) {
> + /* DTE[V]=0, address is passed untranslated */
> goto out;
> + } else if (dte_ret == -AMDVI_FR_DTE_TV) {
> + return;
> }
>
> amdvi_page_walk(as, entry, ret,
> --
> 2.43.5
>
