Re: [PATCH v2 06/20] amd_iommu: Return an error when unable to read PTE from guest memory

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

On 6/12/25 6:37 AM, Vasant Hegde wrote:
> Alejandro,
> 
> 
> On 5/2/2025 7:45 AM, Alejandro Jimenez wrote:
>> Make amdvi_get_pte_entry() return an error value (-1) in cases where the
>> memory read fails, versus the current return of 0 to indicate failure.
>> The reason is that 0 is also a valid PTE value, and it is useful to know
> 
> 
> If PTE is valid then at least PR bit will be set. So it will not be zero right?
> 

I can change the wording in the last sentence to something like:

"The reason is that 0 is also a valid value to have stored in the PTE in 
guest memory i.e. the guest does not have a mapping"

What I am trying to convey is that amdvi_get_pte_entry() should return 
three different states:

-1: Error attempting to read the guest memory that contains the PTE i.e. 
dma_memory_read() returned an error. This has likely nothing to do with 
the guest and signals a problem with the emulation (e.g. problem with 
QEMU memory backend)

0: The guest memory containing the PTE was successfully read, but there 
is currently no mapping in that PTE i.e. *pte==0

 >0: The guest memory containing the PTE was successfully read, and 
there is currently a mapping i.e. *pte== <hopefully a valid IO page 
table entry>

Before the change, amdvi_get_pte_entry() returned 0 for both an error 
and for empty PTEs, but for the page walker implementation later in the 
patchset we need to differentiate between those two conditions.

Thank you,
Alejandro

> -Vasant
> 
> 
>> when a PTE points to memory that is zero i.e. the guest unmapped the
>> page.
>>
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 5322a614f5d6..698967cc1a88 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -496,7 +496,7 @@ static inline uint64_t amdvi_get_pte_entry(AMDVIState *s, uint64_t pte_addr,
>>                           &pte, sizeof(pte), MEMTXATTRS_UNSPECIFIED)) {
>>           trace_amdvi_get_pte_hwerror(pte_addr);
>>           amdvi_log_pagetab_error(s, devid, pte_addr, 0);
>> -        pte = 0;
>> +        pte = (uint64_t)-1;
>>           return pte;
>>       }
>>   
>> @@ -1024,7 +1024,7 @@ static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
>>               /* add offset and load pte */
>>               pte_addr += ((addr >> (3 + 9 * level)) & 0x1FF) << 3;
>>               pte = amdvi_get_pte_entry(as->iommu_state, pte_addr, as->devfn);
>> -            if (!pte) {
>> +            if (!pte || (pte == (uint64_t)-1)) {
>>                   return;
>>               }
>>               oldlevel = level;
>