Hi Ethan,
On 5/20/25 6:18 AM, Ethan MILON wrote:
Hi,
On 5/2/25 4:15 AM, Alejandro Jimenez wrote:
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.
+/*
+ * 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;
+
+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.
I am afraid I don't understand this argument. How does it make it
harder? It returns 0 on success, and a negative value in case of error,
and the error type can be checked and handled as needed by the caller.
The current implementation checks for 3 basic possible failures and maps
them to errors:
AMDVI_FR_DTE_RTR_ERR --> This generally means something is very wrong
AMDVI_FR_DTE_V and/or AMDVI_FR_DTE_TV i.e. V=1, TV=1 --> This is a basic
condition that multiple DTE fields require to be considered valid.
Every time we need to retrieve a DTE (for current and future features)
we need to minimally check for those conditions.
Is there a reason this couldn't be handled inline?
Discounting future uses, just by the end of the series you have 5
different callers of this function, that is a lot of code duplication...
Thank you,
Alejandro
