diff mbox series

[v2,08/17] intel_iommu: Set accessed and dirty bits during first stage translation

Message ID 20240805062727.2307552-9-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Aug. 5, 2024, 6:27 a.m. UTC 
From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu_internal.h |  3 +++
 hw/i386/intel_iommu.c          | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Yi Liu Aug. 14, 2024, 11:45 a.m. UTC | #1 
On 2024/8/5 14:27, Zhenzhong Duan wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h |  3 +++
>   hw/i386/intel_iommu.c          | 24 ++++++++++++++++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 668583aeca..7786ef7624 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -324,6 +324,7 @@ typedef enum VTDFaultReason {
>   
>       /* Output address in the interrupt address range for scalable mode */
>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
> +    VTD_FR_FS_BIT_UPDATE_FAILED = 0x91, /* SFS.10 */
>       VTD_FR_MAX,                 /* Guard */
>   } VTDFaultReason;
>   
> @@ -549,6 +550,8 @@ typedef struct VTDRootEntry VTDRootEntry;
>   /* Masks for First Level Paging Entry */
>   #define VTD_FL_P                    1ULL
>   #define VTD_FL_RW_MASK              (1ULL << 1)
> +#define VTD_FL_A                    0x20
> +#define VTD_FL_D                    0x40
>   
>   /* Second Level Page Translation Pointer*/
>   #define VTD_SM_PASID_ENTRY_SLPTPTR     (~0xfffULL)
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 6121cca4cd..3c2ceed284 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1822,6 +1822,7 @@ static const bool vtd_qualified_faults[] = {
>       [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>       [VTD_FR_FS_NON_CANONICAL] = true,
> +    [VTD_FR_FS_BIT_UPDATE_FAILED] = true,
>       [VTD_FR_MAX] = false,
>   };
>   
> @@ -1939,6 +1940,20 @@ static bool vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t iova,
>               );
>   }
>   
> +static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr, uint32_t index,
> +                                       uint64_t pte, uint64_t flag)
> +{
> +    if (pte & flag) {
> +        return MEMTX_OK;
> +    }
> +    pte |= flag;
> +    pte = cpu_to_le64(pte);
> +    return dma_memory_write(&address_space_memory,
> +                            base_addr + index * sizeof(pte),
> +                            &pte, sizeof(pte),
> +                            MEMTXATTRS_UNSPECIFIED);

Can we ensure this write is atomic? A/D bit setting should be atomic from
guest p.o.v.

> +}
> +
>   /*
>    * Given the @iova, get relevant @flptep. @flpte_level will be the last level
>    * of the translation, can be used for deciding the size of large page.
> @@ -1990,7 +2005,16 @@ static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
>               return -VTD_FR_PAGING_ENTRY_RSVD;
>           }
>   
> +        if (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_A) != MEMTX_OK) {
> +            return -VTD_FR_FS_BIT_UPDATE_FAILED;
> +        }
> +
>           if (vtd_is_last_pte(flpte, level)) {
> +            if (is_write &&
> +                (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_D) !=
> +                                                                    MEMTX_OK)) {
> +                    return -VTD_FR_FS_BIT_UPDATE_FAILED;
> +            }
>               *flptep = flpte;
>               *flpte_level = level;
>               return 0;
Duan, Zhenzhong Aug. 16, 2024, 2:37 a.m. UTC | #2 
>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v2 08/17] intel_iommu: Set accessed and dirty bits
>during first stage translation
>
>On 2024/8/5 14:27, Zhenzhong Duan wrote:
>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  3 +++
>>   hw/i386/intel_iommu.c          | 24 ++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>> index 668583aeca..7786ef7624 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -324,6 +324,7 @@ typedef enum VTDFaultReason {
>>
>>       /* Output address in the interrupt address range for scalable mode */
>>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
>> +    VTD_FR_FS_BIT_UPDATE_FAILED = 0x91, /* SFS.10 */
>>       VTD_FR_MAX,                 /* Guard */
>>   } VTDFaultReason;
>>
>> @@ -549,6 +550,8 @@ typedef struct VTDRootEntry VTDRootEntry;
>>   /* Masks for First Level Paging Entry */
>>   #define VTD_FL_P                    1ULL
>>   #define VTD_FL_RW_MASK              (1ULL << 1)
>> +#define VTD_FL_A                    0x20
>> +#define VTD_FL_D                    0x40
>>
>>   /* Second Level Page Translation Pointer*/
>>   #define VTD_SM_PASID_ENTRY_SLPTPTR     (~0xfffULL)
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 6121cca4cd..3c2ceed284 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1822,6 +1822,7 @@ static const bool vtd_qualified_faults[] = {
>>       [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>>       [VTD_FR_FS_NON_CANONICAL] = true,
>> +    [VTD_FR_FS_BIT_UPDATE_FAILED] = true,
>>       [VTD_FR_MAX] = false,
>>   };
>>
>> @@ -1939,6 +1940,20 @@ static bool
>vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t iova,
>>               );
>>   }
>>
>> +static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr,
>uint32_t index,
>> +                                       uint64_t pte, uint64_t flag)
>> +{
>> +    if (pte & flag) {
>> +        return MEMTX_OK;
>> +    }
>> +    pte |= flag;
>> +    pte = cpu_to_le64(pte);
>> +    return dma_memory_write(&address_space_memory,
>> +                            base_addr + index * sizeof(pte),
>> +                            &pte, sizeof(pte),
>> +                            MEMTXATTRS_UNSPECIFIED);
>
>Can we ensure this write is atomic? A/D bit setting should be atomic from
>guest p.o.v.

Yes, what about below:

@@ -2096,7 +2096,7 @@ static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
     dma_addr_t addr = vtd_get_iova_pgtbl_base(s, ce, pasid);
     uint32_t level = vtd_get_iova_level(s, ce, pasid);
     uint32_t offset;
-    uint64_t flpte;
+    uint64_t flpte, flag_ad = VTD_FL_A;

     if (!vtd_iova_fl_check_canonical(s, iova, ce, pasid)) {
         error_report_once("%s: detected non canonical IOVA (iova=0x%" PRIx64 ","
@@ -2134,16 +2134,15 @@ static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
             return -VTD_FR_PAGING_ENTRY_RSVD;
         }

-        if (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_A) != MEMTX_OK) {
+        if (vtd_is_last_pte(flpte, level) && is_write) {
+            flag_ad |= VTD_FL_D;
+        }
+
+        if (vtd_set_flag_in_pte(addr, offset, flpte, flag_ad) != MEMTX_OK) {
             return -VTD_FR_FS_BIT_UPDATE_FAILED;
         }

         if (vtd_is_last_pte(flpte, level)) {
-            if (is_write &&
-                (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_D) !=
-                                                                    MEMTX_OK)) {
-                    return -VTD_FR_FS_BIT_UPDATE_FAILED;
-            }
             *flptep = flpte;
             *flpte_level = level;
             return 0;

Thanks
Zhenzhong
CLEMENT MATHIEU--DRIF Aug. 16, 2024, 4:22 a.m. UTC | #3 
On 14/08/2024 13:45, Yi Liu 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.
>
>
> On 2024/8/5 14:27, Zhenzhong Duan wrote:
>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  3 +++
>>   hw/i386/intel_iommu.c          | 24 ++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h 
>> b/hw/i386/intel_iommu_internal.h
>> index 668583aeca..7786ef7624 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -324,6 +324,7 @@ typedef enum VTDFaultReason {
>>
>>       /* Output address in the interrupt address range for scalable 
>> mode */
>>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
>> +    VTD_FR_FS_BIT_UPDATE_FAILED = 0x91, /* SFS.10 */
>>       VTD_FR_MAX,                 /* Guard */
>>   } VTDFaultReason;
>>
>> @@ -549,6 +550,8 @@ typedef struct VTDRootEntry VTDRootEntry;
>>   /* Masks for First Level Paging Entry */
>>   #define VTD_FL_P                    1ULL
>>   #define VTD_FL_RW_MASK              (1ULL << 1)
>> +#define VTD_FL_A                    0x20
>> +#define VTD_FL_D                    0x40
>>
>>   /* Second Level Page Translation Pointer*/
>>   #define VTD_SM_PASID_ENTRY_SLPTPTR     (~0xfffULL)
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 6121cca4cd..3c2ceed284 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1822,6 +1822,7 @@ static const bool vtd_qualified_faults[] = {
>>       [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>>       [VTD_FR_FS_NON_CANONICAL] = true,
>> +    [VTD_FR_FS_BIT_UPDATE_FAILED] = true,
>>       [VTD_FR_MAX] = false,
>>   };
>>
>> @@ -1939,6 +1940,20 @@ static bool 
>> vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t iova,
>>               );
>>   }
>>
>> +static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr, 
>> uint32_t index,
>> +                                       uint64_t pte, uint64_t flag)
>> +{
>> +    if (pte & flag) {
>> +        return MEMTX_OK;
>> +    }
>> +    pte |= flag;
>> +    pte = cpu_to_le64(pte);
>> +    return dma_memory_write(&address_space_memory,
>> +                            base_addr + index * sizeof(pte),
>> +                            &pte, sizeof(pte),
>> +                            MEMTXATTRS_UNSPECIFIED);
>
> Can we ensure this write is atomic? A/D bit setting should be atomic from
> guest p.o.v.
As we only set one bit at a time, I don't think we can face atomicity issues
>
>> +}
>> +
>>   /*
>>    * Given the @iova, get relevant @flptep. @flpte_level will be the 
>> last level
>>    * of the translation, can be used for deciding the size of large 
>> page.
>> @@ -1990,7 +2005,16 @@ static int vtd_iova_to_flpte(IntelIOMMUState 
>> *s, VTDContextEntry *ce,
>>               return -VTD_FR_PAGING_ENTRY_RSVD;
>>           }
>>
>> +        if (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_A) != 
>> MEMTX_OK) {
>> +            return -VTD_FR_FS_BIT_UPDATE_FAILED;
>> +        }
>> +
>>           if (vtd_is_last_pte(flpte, level)) {
>> +            if (is_write &&
>> +                (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_D) !=
>> + MEMTX_OK)) {
>> +                    return -VTD_FR_FS_BIT_UPDATE_FAILED;
>> +            }
>>               *flptep = flpte;
>>               *flpte_level = level;
>>               return 0;
>
> -- 
> Regards,
> Yi Liu
CLEMENT MATHIEU--DRIF Aug. 16, 2024, 4:29 a.m. UTC | #4 
On 16/08/2024 04:37, Duan, Zhenzhong 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.
>
>
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Subject: Re: [PATCH v2 08/17] intel_iommu: Set accessed and dirty bits
>> during first stage translation
>>
>> On 2024/8/5 14:27, Zhenzhong Duan wrote:
>>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>>
>>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu_internal.h |  3 +++
>>>    hw/i386/intel_iommu.c          | 24 ++++++++++++++++++++++++
>>>    2 files changed, 27 insertions(+)
>>>
>>> diff --git a/hw/i386/intel_iommu_internal.h
>> b/hw/i386/intel_iommu_internal.h
>>> index 668583aeca..7786ef7624 100644
>>> --- a/hw/i386/intel_iommu_internal.h
>>> +++ b/hw/i386/intel_iommu_internal.h
>>> @@ -324,6 +324,7 @@ typedef enum VTDFaultReason {
>>>
>>>        /* Output address in the interrupt address range for scalable mode */
>>>        VTD_FR_SM_INTERRUPT_ADDR = 0x87,
>>> +    VTD_FR_FS_BIT_UPDATE_FAILED = 0x91, /* SFS.10 */
>>>        VTD_FR_MAX,                 /* Guard */
>>>    } VTDFaultReason;
>>>
>>> @@ -549,6 +550,8 @@ typedef struct VTDRootEntry VTDRootEntry;
>>>    /* Masks for First Level Paging Entry */
>>>    #define VTD_FL_P                    1ULL
>>>    #define VTD_FL_RW_MASK              (1ULL << 1)
>>> +#define VTD_FL_A                    0x20
>>> +#define VTD_FL_D                    0x40
>>>
>>>    /* Second Level Page Translation Pointer*/
>>>    #define VTD_SM_PASID_ENTRY_SLPTPTR     (~0xfffULL)
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 6121cca4cd..3c2ceed284 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -1822,6 +1822,7 @@ static const bool vtd_qualified_faults[] = {
>>>        [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>>>        [VTD_FR_SM_INTERRUPT_ADDR] = true,
>>>        [VTD_FR_FS_NON_CANONICAL] = true,
>>> +    [VTD_FR_FS_BIT_UPDATE_FAILED] = true,
>>>        [VTD_FR_MAX] = false,
>>>    };
>>>
>>> @@ -1939,6 +1940,20 @@ static bool
>> vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t iova,
>>>                );
>>>    }
>>>
>>> +static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr,
>> uint32_t index,
>>> +                                       uint64_t pte, uint64_t flag)
>>> +{
>>> +    if (pte & flag) {
>>> +        return MEMTX_OK;
>>> +    }
>>> +    pte |= flag;
>>> +    pte = cpu_to_le64(pte);
>>> +    return dma_memory_write(&address_space_memory,
>>> +                            base_addr + index * sizeof(pte),
>>> +                            &pte, sizeof(pte),
>>> +                            MEMTXATTRS_UNSPECIFIED);
>> Can we ensure this write is atomic? A/D bit setting should be atomic from
>> guest p.o.v.
> Yes, what about below:
>
> @@ -2096,7 +2096,7 @@ static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
>       dma_addr_t addr = vtd_get_iova_pgtbl_base(s, ce, pasid);
>       uint32_t level = vtd_get_iova_level(s, ce, pasid);
>       uint32_t offset;
> -    uint64_t flpte;
> +    uint64_t flpte, flag_ad = VTD_FL_A;
>
>       if (!vtd_iova_fl_check_canonical(s, iova, ce, pasid)) {
>           error_report_once("%s: detected non canonical IOVA (iova=0x%" PRIx64 ","
> @@ -2134,16 +2134,15 @@ static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
>               return -VTD_FR_PAGING_ENTRY_RSVD;
>           }
>
> -        if (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_A) != MEMTX_OK) {
> +        if (vtd_is_last_pte(flpte, level) && is_write) {
> +            flag_ad |= VTD_FL_D;
> +        }
> +
> +        if (vtd_set_flag_in_pte(addr, offset, flpte, flag_ad) != MEMTX_OK) {
>               return -VTD_FR_FS_BIT_UPDATE_FAILED;
>           }
>
>           if (vtd_is_last_pte(flpte, level)) {
> -            if (is_write &&
> -                (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_D) !=
> -                                                                    MEMTX_OK)) {
> -                    return -VTD_FR_FS_BIT_UPDATE_FAILED;
> -            }
>               *flptep = flpte;
>               *flpte_level = level;
>               return 0;
lgtm

Thanks
 >cmd
>
> Thanks
> Zhenzhong
>
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 668583aeca..7786ef7624 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -324,6 +324,7 @@  typedef enum VTDFaultReason {
 
     /* Output address in the interrupt address range for scalable mode */
     VTD_FR_SM_INTERRUPT_ADDR = 0x87,
+    VTD_FR_FS_BIT_UPDATE_FAILED = 0x91, /* SFS.10 */
     VTD_FR_MAX,                 /* Guard */
 } VTDFaultReason;
 
@@ -549,6 +550,8 @@  typedef struct VTDRootEntry VTDRootEntry;
 /* Masks for First Level Paging Entry */
 #define VTD_FL_P                    1ULL
 #define VTD_FL_RW_MASK              (1ULL << 1)
+#define VTD_FL_A                    0x20
+#define VTD_FL_D                    0x40
 
 /* Second Level Page Translation Pointer*/
 #define VTD_SM_PASID_ENTRY_SLPTPTR     (~0xfffULL)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 6121cca4cd..3c2ceed284 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1822,6 +1822,7 @@  static const bool vtd_qualified_faults[] = {
     [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
     [VTD_FR_SM_INTERRUPT_ADDR] = true,
     [VTD_FR_FS_NON_CANONICAL] = true,
+    [VTD_FR_FS_BIT_UPDATE_FAILED] = true,
     [VTD_FR_MAX] = false,
 };
 
@@ -1939,6 +1940,20 @@  static bool vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t iova,
             );
 }
 
+static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr, uint32_t index,
+                                       uint64_t pte, uint64_t flag)
+{
+    if (pte & flag) {
+        return MEMTX_OK;
+    }
+    pte |= flag;
+    pte = cpu_to_le64(pte);
+    return dma_memory_write(&address_space_memory,
+                            base_addr + index * sizeof(pte),
+                            &pte, sizeof(pte),
+                            MEMTXATTRS_UNSPECIFIED);
+}
+
 /*
  * Given the @iova, get relevant @flptep. @flpte_level will be the last level
  * of the translation, can be used for deciding the size of large page.
@@ -1990,7 +2005,16 @@  static int vtd_iova_to_flpte(IntelIOMMUState *s, VTDContextEntry *ce,
             return -VTD_FR_PAGING_ENTRY_RSVD;
         }
 
+        if (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_A) != MEMTX_OK) {
+            return -VTD_FR_FS_BIT_UPDATE_FAILED;
+        }
+
         if (vtd_is_last_pte(flpte, level)) {
+            if (is_write &&
+                (vtd_set_flag_in_pte(addr, offset, flpte, VTD_FL_D) !=
+                                                                    MEMTX_OK)) {
+                    return -VTD_FR_FS_BIT_UPDATE_FAILED;
+            }
             *flptep = flpte;
             *flpte_level = level;
             return 0;