diff mbox

[for-2.9,1/2] intel_iommu: check validity for GAW bits in CE

Message ID 1481089965-3888-2-git-send-email-peterx@redhat.com
State New
Headers show

Commit Message

Peter Xu Dec. 7, 2016, 5:52 a.m. UTC 
Currently vt-d Context Entry (CE) only allows 39/48 bits address width.
If guest software configured more than that, we complain and force
shrink to the maximum supported, which is 48bits.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 12 +++++++++++-
 hw/i386/intel_iommu_internal.h |  2 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Jason Wang Dec. 8, 2016, 2:02 a.m. UTC | #1 
On 2016年12月07日 13:52, Peter Xu wrote:
> Currently vt-d Context Entry (CE) only allows 39/48 bits address width.
> If guest software configured more than that, we complain and force
> shrink to the maximum supported, which is 48bits.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/intel_iommu.c          | 12 +++++++++++-
>   hw/i386/intel_iommu_internal.h |  2 ++
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 5f3e351..98d45ef 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -601,7 +601,17 @@ static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
>   
>   static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
>   {
> -    return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
> +    uint8_t aw = (ce->hi & VTD_CONTEXT_ENTRY_AW);
> +    /*
> +     * According to vt-d spec 10.4.2 bits 12:8, SAGAW only allows
> +     * 39/48 bits.
> +     */
> +    if (aw > VTD_CE_AW_48BIT) {
> +        error_report("Context entry address width not supported (aw=%d), "
> +                     "Shrinking to maximum.", aw);
> +        aw = VTD_CE_AW_48BIT;
> +    }

Is this behavior specified by spec?

> +    return 30 + aw * 9;
>   }
>   
>   static const uint64_t vtd_paging_entry_rsvd_field[] = {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 11abfa2..e808c67 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -406,6 +406,8 @@ typedef struct VTDRootEntry VTDRootEntry;
>   #define VTD_CONTEXT_ENTRY_RSVD_LO   (0xff0ULL | ~VTD_HAW_MASK)
>   /* hi */
>   #define VTD_CONTEXT_ENTRY_AW        7ULL /* Adjusted guest-address-width */
> +#define VTD_CE_AW_39BIT             1
> +#define VTD_CE_AW_48BIT             2
>   #define VTD_CONTEXT_ENTRY_DID(val)  (((val) >> 8) & VTD_DOMAIN_ID_MASK)
>   #define VTD_CONTEXT_ENTRY_RSVD_HI   0xffffffffff000080ULL
>
Peter Xu Dec. 8, 2016, 2:16 a.m. UTC | #2 
On Thu, Dec 08, 2016 at 10:02:15AM +0800, Jason Wang wrote:
> 
> 
> On 2016年12月07日 13:52, Peter Xu wrote:
> >Currently vt-d Context Entry (CE) only allows 39/48 bits address width.
> >If guest software configured more than that, we complain and force
> >shrink to the maximum supported, which is 48bits.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> >  hw/i386/intel_iommu.c          | 12 +++++++++++-
> >  hw/i386/intel_iommu_internal.h |  2 ++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index 5f3e351..98d45ef 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -601,7 +601,17 @@ static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
> >  static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
> >  {
> >-    return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
> >+    uint8_t aw = (ce->hi & VTD_CONTEXT_ENTRY_AW);
> >+    /*
> >+     * According to vt-d spec 10.4.2 bits 12:8, SAGAW only allows
> >+     * 39/48 bits.
> >+     */
> >+    if (aw > VTD_CE_AW_48BIT) {
> >+        error_report("Context entry address width not supported (aw=%d), "
> >+                     "Shrinking to maximum.", aw);
> >+        aw = VTD_CE_AW_48BIT;
> >+    }
> 
> Is this behavior specified by spec?

That's how I understand spec 10.4.2 bits 12:8 (as mentioned in above
comment). Only 39/48 bits AGAW are allowed, and others are reserved.

When writting up this patch, I thought illegal value for this aw bits
(from guest software) might cause trouble, but I was wrong, since we
have a "MIN(ce_agaw, VTD_MGAW)" check later on. So that won't be a
problem, and this patch is not that necessary now.

Thanks,

-- peterx
Jason Wang Dec. 8, 2016, 2:21 a.m. UTC | #3 
On 2016年12月08日 10:16, Peter Xu wrote:
> On Thu, Dec 08, 2016 at 10:02:15AM +0800, Jason Wang wrote:
>>
>> On 2016年12月07日 13:52, Peter Xu wrote:
>>> Currently vt-d Context Entry (CE) only allows 39/48 bits address width.
>>> If guest software configured more than that, we complain and force
>>> shrink to the maximum supported, which is 48bits.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>   hw/i386/intel_iommu.c          | 12 +++++++++++-
>>>   hw/i386/intel_iommu_internal.h |  2 ++
>>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 5f3e351..98d45ef 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -601,7 +601,17 @@ static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
>>>   static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
>>>   {
>>> -    return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
>>> +    uint8_t aw = (ce->hi & VTD_CONTEXT_ENTRY_AW);
>>> +    /*
>>> +     * According to vt-d spec 10.4.2 bits 12:8, SAGAW only allows
>>> +     * 39/48 bits.
>>> +     */
>>> +    if (aw > VTD_CE_AW_48BIT) {
>>> +        error_report("Context entry address width not supported (aw=%d), "
>>> +                     "Shrinking to maximum.", aw);
>>> +        aw = VTD_CE_AW_48BIT;
>>> +    }
>> Is this behavior specified by spec?
> That's how I understand spec 10.4.2 bits 12:8 (as mentioned in above
> comment). Only 39/48 bits AGAW are allowed, and others are reserved.
>
> When writting up this patch, I thought illegal value for this aw bits
> (from guest software) might cause trouble, but I was wrong, since we
> have a "MIN(ce_agaw, VTD_MGAW)" check later on. So that won't be a
> problem, and this patch is not that necessary now.
>
> Thanks,
>
> -- peterx

Yes, and it can even report DMAR fault to guest which is better.

Thanks
Peter Xu Dec. 12, 2016, 1:47 a.m. UTC | #4 
On Thu, Dec 08, 2016 at 10:21:35AM +0800, Jason Wang wrote:
> 
> 
> On 2016年12月08日 10:16, Peter Xu wrote:
> >On Thu, Dec 08, 2016 at 10:02:15AM +0800, Jason Wang wrote:
> >>
> >>On 2016年12月07日 13:52, Peter Xu wrote:
> >>>Currently vt-d Context Entry (CE) only allows 39/48 bits address width.
> >>>If guest software configured more than that, we complain and force
> >>>shrink to the maximum supported, which is 48bits.
> >>>
> >>>Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>---
> >>>  hw/i386/intel_iommu.c          | 12 +++++++++++-
> >>>  hw/i386/intel_iommu_internal.h |  2 ++
> >>>  2 files changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >>>index 5f3e351..98d45ef 100644
> >>>--- a/hw/i386/intel_iommu.c
> >>>+++ b/hw/i386/intel_iommu.c
> >>>@@ -601,7 +601,17 @@ static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
> >>>  static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
> >>>  {
> >>>-    return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
> >>>+    uint8_t aw = (ce->hi & VTD_CONTEXT_ENTRY_AW);
> >>>+    /*
> >>>+     * According to vt-d spec 10.4.2 bits 12:8, SAGAW only allows
> >>>+     * 39/48 bits.
> >>>+     */
> >>>+    if (aw > VTD_CE_AW_48BIT) {
> >>>+        error_report("Context entry address width not supported (aw=%d), "
> >>>+                     "Shrinking to maximum.", aw);
> >>>+        aw = VTD_CE_AW_48BIT;
> >>>+    }
> >>Is this behavior specified by spec?
> >That's how I understand spec 10.4.2 bits 12:8 (as mentioned in above
> >comment). Only 39/48 bits AGAW are allowed, and others are reserved.
> >
> >When writting up this patch, I thought illegal value for this aw bits
> >(from guest software) might cause trouble, but I was wrong, since we
> >have a "MIN(ce_agaw, VTD_MGAW)" check later on. So that won't be a
> >problem, and this patch is not that necessary now.
> >
> >Thanks,
> >
> >-- peterx
> 
> Yes, and it can even report DMAR fault to guest which is better.

Yes. Will respin, thanks. :-)

-- peterx
diff mbox

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5f3e351..98d45ef 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -601,7 +601,17 @@  static inline uint32_t vtd_get_level_from_context_entry(VTDContextEntry *ce)
 
 static inline uint32_t vtd_get_agaw_from_context_entry(VTDContextEntry *ce)
 {
-    return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9;
+    uint8_t aw = (ce->hi & VTD_CONTEXT_ENTRY_AW);
+    /*
+     * According to vt-d spec 10.4.2 bits 12:8, SAGAW only allows
+     * 39/48 bits.
+     */
+    if (aw > VTD_CE_AW_48BIT) {
+        error_report("Context entry address width not supported (aw=%d), "
+                     "Shrinking to maximum.", aw);
+        aw = VTD_CE_AW_48BIT;
+    }
+    return 30 + aw * 9;
 }
 
 static const uint64_t vtd_paging_entry_rsvd_field[] = {
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 11abfa2..e808c67 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -406,6 +406,8 @@  typedef struct VTDRootEntry VTDRootEntry;
 #define VTD_CONTEXT_ENTRY_RSVD_LO   (0xff0ULL | ~VTD_HAW_MASK)
 /* hi */
 #define VTD_CONTEXT_ENTRY_AW        7ULL /* Adjusted guest-address-width */
+#define VTD_CE_AW_39BIT             1
+#define VTD_CE_AW_48BIT             2
 #define VTD_CONTEXT_ENTRY_DID(val)  (((val) >> 8) & VTD_DOMAIN_ID_MASK)
 #define VTD_CONTEXT_ENTRY_RSVD_HI   0xffffffffff000080ULL