Hi,
On 5/21/25 4:49 PM, 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.
>
>
> 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.
>
You're right, I initially thought the amdvi_as_to_dte() helper might
make proper fault reporting harder, but on second look, it shouldn't be
a problem.
> 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
>
For AMDVI_FR_DTE_RTR_ERR, would it make sense to be more specific? Right
now, it could mean either a hardware issue or a malformed DTE. Or should
it be split up in a future patch with proper fault reporting implemented?
> 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...
>
Ah, my mistake, I didn’t notice all the callers.
Thank,
Ethan
> Thank you,
> Alejandro
>
>
>
>
