* [PATCH V2] intel_iommu: TM field should not be in reserved bits
@ 2019-09-27 4:58 qi1.zhang
2019-09-27 6:10 ` Peter Xu
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: qi1.zhang @ 2019-09-27 4:58 UTC (permalink / raw)
To: qemu-devel; +Cc: qi1.zhang, ehabkost, mst, pbonzini, rth
From: "Zhang, Qi" <qi1.zhang@intel.com>
When dt is supported, TM field should not be Reserved(0).
Refer to VT-d Spec 9.8
Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
---
hw/i386/intel_iommu.c | 12 ++++++------
hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
2 files changed, 25 insertions(+), 12 deletions(-)
---
Changelog V2:
move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and VTD_SPTE_LPAGE_LX_RSVD_MASK
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f1de8fdb75..35222cf55c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3548,13 +3548,13 @@ static void vtd_init(IntelIOMMUState *s)
* Rsvd field masks for spte
*/
vtd_paging_entry_rsvd_field[0] = ~0ULL;
- vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits);
- vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits);
- vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits);
+ vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
+ vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
+ vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
vtd_paging_entry_rsvd_field[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s->aw_bits);
- vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits);
- vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits);
- vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits);
+ vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
+ vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
+ vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
vtd_paging_entry_rsvd_field[8] = VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
if (x86_iommu_ir_supported(x86_iommu)) {
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index c1235a7063..01f1aa6c86 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc;
#define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
/* Rsvd field masks for spte */
-#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
+#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
-#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw) \
+#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
-#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw) \
+#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
#define VTD_SPTE_PAGE_L4_RSVD_MASK(aw) \
(0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
-#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw) \
+#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
-#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw) \
+#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
-#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw) \
+#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, dt_supported) \
+ dt_supported? \
+ (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
(0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
#define VTD_SPTE_LPAGE_L4_RSVD_MASK(aw) \
(0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
@@ -506,5 +518,6 @@ typedef struct VTDRootEntry VTDRootEntry;
#define VTD_SL_W (1ULL << 1)
#define VTD_SL_PT_BASE_ADDR_MASK(aw) (~(VTD_PAGE_SIZE - 1) & VTD_HAW_MASK(aw))
#define VTD_SL_IGN_COM 0xbff0000000000000ULL
+#define VTD_SL_TM (1ULL << 62)
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 4:58 [PATCH V2] intel_iommu: TM field should not be in reserved bits qi1.zhang
@ 2019-09-27 6:10 ` Peter Xu
2019-09-27 8:03 ` Zhang, Qi1
2019-09-27 9:33 ` Michael S. Tsirkin
2019-09-27 20:44 ` no-reply
2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-09-27 6:10 UTC (permalink / raw)
To: qi1.zhang; +Cc: pbonzini, rth, mst, qemu-devel, ehabkost
On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> From: "Zhang, Qi" <qi1.zhang@intel.com>
>
> When dt is supported, TM field should not be Reserved(0).
>
> Refer to VT-d Spec 9.8
>
> Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> ---
> hw/i386/intel_iommu.c | 12 ++++++------
> hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> 2 files changed, 25 insertions(+), 12 deletions(-)
> ---
> Changelog V2:
> move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and VTD_SPTE_LPAGE_LX_RSVD_MASK
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f1de8fdb75..35222cf55c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3548,13 +3548,13 @@ static void vtd_init(IntelIOMMUState *s)
> * Rsvd field masks for spte
> */
> vtd_paging_entry_rsvd_field[0] = ~0ULL;
> - vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits);
> + vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> vtd_paging_entry_rsvd_field[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits);
> + vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> vtd_paging_entry_rsvd_field[8] = VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6
(1G). While this reminded me that I'm totally confused on why we have
had entry 7, 8 after all... Are they really used?
>
> if (x86_iommu_ir_supported(x86_iommu)) {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index c1235a7063..01f1aa6c86 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc;
> #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
>
> /* Rsvd field masks for spte */
> -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
This seems strange too in that ~VTD_HAW_MASK(aw) probably covered bits
63-48 for aw==48 case so it should already cover VTD_SL_TM?
Meanwhile when I'm reading the spec I see at least bits 61-52 ignored
rather than reserved.
Thanks,
> -#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> #define VTD_SPTE_PAGE_L4_RSVD_MASK(aw) \
> (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> #define VTD_SPTE_LPAGE_L4_RSVD_MASK(aw) \
> (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> @@ -506,5 +518,6 @@ typedef struct VTDRootEntry VTDRootEntry;
> #define VTD_SL_W (1ULL << 1)
> #define VTD_SL_PT_BASE_ADDR_MASK(aw) (~(VTD_PAGE_SIZE - 1) & VTD_HAW_MASK(aw))
> #define VTD_SL_IGN_COM 0xbff0000000000000ULL
> +#define VTD_SL_TM (1ULL << 62)
>
> #endif
> --
> 2.20.1
>
>
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 6:10 ` Peter Xu
@ 2019-09-27 8:03 ` Zhang, Qi1
2019-09-27 9:32 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Zhang, Qi1 @ 2019-09-27 8:03 UTC (permalink / raw)
To: Peter Xu; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
> -----Original Message-----
> From: Peter Xu <peterx@redhat.com>
> Sent: Friday, September 27, 2019 2:10 PM
> To: Zhang, Qi1 <qi1.zhang@intel.com>
> Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> pbonzini@redhat.com; rth@twiddle.net
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > From: "Zhang, Qi" <qi1.zhang@intel.com>
> >
> > When dt is supported, TM field should not be Reserved(0).
> >
> > Refer to VT-d Spec 9.8
> >
> > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > ---
> > hw/i386/intel_iommu.c | 12 ++++++------
> > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > 2 files changed, 25 insertions(+), 12 deletions(-)
> > ---
> > Changelog V2:
> > move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and
> > VTD_SPTE_LPAGE_LX_RSVD_MASK
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > f1de8fdb75..35222cf55c 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3548,13 +3548,13 @@ static void vtd_init(IntelIOMMUState *s)
> > * Rsvd field masks for spte
> > */
> > vtd_paging_entry_rsvd_field[0] = ~0ULL;
> > - vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s-
> >aw_bits);
> > - vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> >aw_bits);
> > - vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s-
> >aw_bits);
> > + vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s-
> >aw_bits, x86_iommu->dt_supported);
> > + vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> >aw_bits, x86_iommu->dt_supported);
> > + vtd_paging_entry_rsvd_field[3] =
> > + VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> >dt_supported);
> > vtd_paging_entry_rsvd_field[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s-
> >aw_bits);
> > - vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s-
> >aw_bits);
> > - vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s-
> >aw_bits);
> > - vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s-
> >aw_bits);
> > + vtd_paging_entry_rsvd_field[5] =
> VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> >dt_supported);
> > + vtd_paging_entry_rsvd_field[6] =
> VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> >dt_supported);
> > + vtd_paging_entry_rsvd_field[7] =
> > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> >dt_supported);
> > vtd_paging_entry_rsvd_field[8] =
> > VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
>
> Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 (1G).
> While this reminded me that I'm totally confused on why we have had entry
> 7, 8 after all... Are they really used?
Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will update a new patchset for it.
>
> >
> > if (x86_iommu_ir_supported(x86_iommu)) { diff --git
> > a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index c1235a7063..01f1aa6c86 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc; #define
> > VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
> >
> > /* Rsvd field masks for spte */
> > -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> > +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> VTD_SL_TM))
> > +: \
> > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
>
> This seems strange too in that ~VTD_HAW_MASK(aw) probably covered bits
> 63-48 for aw==48 case so it should already cover VTD_SL_TM?
VTD_SL_IGN_COM 0xbff0000000000000ULL, TM field is cleared by ~ VTD_SL_IGN_COM
>
> Meanwhile when I'm reading the spec I see at least bits 61-52 ignored rather
> than reserved.
Yes. Bit 61~52 is ignored. Such as the index 5 of this array is 0xfff8000000800.
>
> Thanks,
>
> > -#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw) \
> > +#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> VTD_SL_TM))
> > +: \
> > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) -#define
> > VTD_SPTE_PAGE_L3_RSVD_MASK(aw) \
> > +#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> VTD_SL_TM))
> > +: \
> > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) #define
> > VTD_SPTE_PAGE_L4_RSVD_MASK(aw) \
> > (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) -#define
> > VTD_SPTE_LPAGE_L1_RSVD_MASK(aw) \
> > +#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> VTD_SL_TM))
> > +: \
> > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) -#define
> > VTD_SPTE_LPAGE_L2_RSVD_MASK(aw) \
> > +#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> > +VTD_SL_TM)) : \
> > (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) -
> #define
> > VTD_SPTE_LPAGE_L3_RSVD_MASK(aw) \
> > +#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, dt_supported) \
> > + dt_supported? \
> > + (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> > +VTD_SL_TM)) : \
> > (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> > #define VTD_SPTE_LPAGE_L4_RSVD_MASK(aw) \
> > (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) @@ -
> 506,5
> > +518,6 @@ typedef struct VTDRootEntry VTDRootEntry;
> > #define VTD_SL_W (1ULL << 1)
> > #define VTD_SL_PT_BASE_ADDR_MASK(aw) (~(VTD_PAGE_SIZE - 1) &
> VTD_HAW_MASK(aw))
> > #define VTD_SL_IGN_COM 0xbff0000000000000ULL
> > +#define VTD_SL_TM (1ULL << 62)
> >
> > #endif
> > --
> > 2.20.1
> >
> >
>
> --
> Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 8:03 ` Zhang, Qi1
@ 2019-09-27 9:32 ` Peter Xu
2019-09-29 1:11 ` Zhang, Qi1
0 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-09-27 9:32 UTC (permalink / raw)
To: Zhang, Qi1; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
On Fri, Sep 27, 2019 at 08:03:21AM +0000, Zhang, Qi1 wrote:
>
>
> > -----Original Message-----
> > From: Peter Xu <peterx@redhat.com>
> > Sent: Friday, September 27, 2019 2:10 PM
> > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > pbonzini@redhat.com; rth@twiddle.net
> > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> >
> > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > > From: "Zhang, Qi" <qi1.zhang@intel.com>
> > >
> > > When dt is supported, TM field should not be Reserved(0).
> > >
> > > Refer to VT-d Spec 9.8
> > >
> > > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > > ---
> > > hw/i386/intel_iommu.c | 12 ++++++------
> > > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > ---
> > > Changelog V2:
> > > move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and
> > > VTD_SPTE_LPAGE_LX_RSVD_MASK
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > > f1de8fdb75..35222cf55c 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3548,13 +3548,13 @@ static void vtd_init(IntelIOMMUState *s)
> > > * Rsvd field masks for spte
> > > */
> > > vtd_paging_entry_rsvd_field[0] = ~0ULL;
> > > - vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s-
> > >aw_bits);
> > > - vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > >aw_bits);
> > > - vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s-
> > >aw_bits);
> > > + vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s-
> > >aw_bits, x86_iommu->dt_supported);
> > > + vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > >aw_bits, x86_iommu->dt_supported);
> > > + vtd_paging_entry_rsvd_field[3] =
> > > + VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > >dt_supported);
> > > vtd_paging_entry_rsvd_field[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s-
> > >aw_bits);
> > > - vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s-
> > >aw_bits);
> > > - vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s-
> > >aw_bits);
> > > - vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s-
> > >aw_bits);
> > > + vtd_paging_entry_rsvd_field[5] =
> > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > >dt_supported);
> > > + vtd_paging_entry_rsvd_field[6] =
> > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > >dt_supported);
> > > + vtd_paging_entry_rsvd_field[7] =
> > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > >dt_supported);
> > > vtd_paging_entry_rsvd_field[8] =
> > > VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> >
> > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 (1G).
> > While this reminded me that I'm totally confused on why we have had entry
> > 7, 8 after all... Are they really used?
> Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will update a new patchset for it.
Could I ask why index 7 may be leaf?
> >
> > >
> > > if (x86_iommu_ir_supported(x86_iommu)) { diff --git
> > > a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > > index c1235a7063..01f1aa6c86 100644
> > > --- a/hw/i386/intel_iommu_internal.h
> > > +++ b/hw/i386/intel_iommu_internal.h
> > > @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc; #define
> > > VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
> > >
> > > /* Rsvd field masks for spte */
> > > -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> > > +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> > > + dt_supported? \
> > > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> > VTD_SL_TM))
> > > +: \
> > > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> >
> > This seems strange too in that ~VTD_HAW_MASK(aw) probably covered bits
> > 63-48 for aw==48 case so it should already cover VTD_SL_TM?
> VTD_SL_IGN_COM 0xbff0000000000000ULL, TM field is cleared by ~ VTD_SL_IGN_COM
> >
> > Meanwhile when I'm reading the spec I see at least bits 61-52 ignored rather
> > than reserved.
> Yes. Bit 61~52 is ignored. Such as the index 5 of this array is 0xfff8000000800.
Oops, my poor eye obviously didn't see that the "~" operator is
applied over the whole (VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)... :)
Btw, you should only touch up the macros that are leaves here.
Non-leaves should still keep that bit as reserved.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 4:58 [PATCH V2] intel_iommu: TM field should not be in reserved bits qi1.zhang
2019-09-27 6:10 ` Peter Xu
@ 2019-09-27 9:33 ` Michael S. Tsirkin
2019-09-27 20:44 ` no-reply
2 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-09-27 9:33 UTC (permalink / raw)
To: qi1.zhang; +Cc: pbonzini, ehabkost, qemu-devel, rth
On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> From: "Zhang, Qi" <qi1.zhang@intel.com>
>
> When dt is supported, TM field should not be Reserved(0).
>
> Refer to VT-d Spec 9.8
>
> Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> ---
> hw/i386/intel_iommu.c | 12 ++++++------
> hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> 2 files changed, 25 insertions(+), 12 deletions(-)
> ---
> Changelog V2:
> move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and VTD_SPTE_LPAGE_LX_RSVD_MASK
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f1de8fdb75..35222cf55c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3548,13 +3548,13 @@ static void vtd_init(IntelIOMMUState *s)
> * Rsvd field masks for spte
> */
> vtd_paging_entry_rsvd_field[0] = ~0ULL;
> - vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits);
> + vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> vtd_paging_entry_rsvd_field[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits);
> - vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits);
> + vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> + vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
> vtd_paging_entry_rsvd_field[8] = VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
>
> if (x86_iommu_ir_supported(x86_iommu)) {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index c1235a7063..01f1aa6c86 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc;
> #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
>
> /* Rsvd field masks for spte */
> -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
space before ? please.
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
cleaner:
0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
(dt_supported ? VTD_SL_TM : 0x0))
> -#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L2_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw) \
> +#define VTD_SPTE_PAGE_L3_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> #define VTD_SPTE_PAGE_L4_RSVD_MASK(aw) \
> (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L1_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L2_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x1ff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> -#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw) \
> +#define VTD_SPTE_LPAGE_L3_RSVD_MASK(aw, dt_supported) \
> + dt_supported? \
> + (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM | VTD_SL_TM)) : \
> (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> #define VTD_SPTE_LPAGE_L4_RSVD_MASK(aw) \
> (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> @@ -506,5 +518,6 @@ typedef struct VTDRootEntry VTDRootEntry;
> #define VTD_SL_W (1ULL << 1)
> #define VTD_SL_PT_BASE_ADDR_MASK(aw) (~(VTD_PAGE_SIZE - 1) & VTD_HAW_MASK(aw))
> #define VTD_SL_IGN_COM 0xbff0000000000000ULL
> +#define VTD_SL_TM (1ULL << 62)
>
> #endif
> --
> 2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 4:58 [PATCH V2] intel_iommu: TM field should not be in reserved bits qi1.zhang
2019-09-27 6:10 ` Peter Xu
2019-09-27 9:33 ` Michael S. Tsirkin
@ 2019-09-27 20:44 ` no-reply
2 siblings, 0 replies; 10+ messages in thread
From: no-reply @ 2019-09-27 20:44 UTC (permalink / raw)
To: qi1.zhang; +Cc: qi1.zhang, ehabkost, mst, qemu-devel, pbonzini, rth
Patchew URL: https://patchew.org/QEMU/20190927045838.2968-1-qi1.zhang@intel.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20190927045838.2968-1-qi1.zhang@intel.com
Subject: [PATCH V2] intel_iommu: TM field should not be in reserved bits
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
4cd505b intel_iommu: TM field should not be in reserved bits
=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#26: FILE: hw/i386/intel_iommu.c:3551:
+ vtd_paging_entry_rsvd_field[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: line over 90 characters
#27: FILE: hw/i386/intel_iommu.c:3552:
+ vtd_paging_entry_rsvd_field[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: line over 90 characters
#28: FILE: hw/i386/intel_iommu.c:3553:
+ vtd_paging_entry_rsvd_field[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: line over 90 characters
#33: FILE: hw/i386/intel_iommu.c:3555:
+ vtd_paging_entry_rsvd_field[5] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: line over 90 characters
#34: FILE: hw/i386/intel_iommu.c:3556:
+ vtd_paging_entry_rsvd_field[6] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: line over 90 characters
#35: FILE: hw/i386/intel_iommu.c:3557:
+ vtd_paging_entry_rsvd_field[7] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu->dt_supported);
ERROR: spaces required around that '?' (ctx:VxE)
#49: FILE: hw/i386/intel_iommu_internal.h:391:
+ dt_supported? \
^
ERROR: spaces required around that '?' (ctx:VxE)
#54: FILE: hw/i386/intel_iommu_internal.h:395:
+ dt_supported? \
^
ERROR: spaces required around that '?' (ctx:VxE)
#59: FILE: hw/i386/intel_iommu_internal.h:399:
+ dt_supported? \
^
ERROR: spaces required around that '?' (ctx:VxE)
#66: FILE: hw/i386/intel_iommu_internal.h:405:
+ dt_supported? \
^
ERROR: spaces required around that '?' (ctx:VxE)
#71: FILE: hw/i386/intel_iommu_internal.h:409:
+ dt_supported? \
^
ERROR: spaces required around that '?' (ctx:VxE)
#76: FILE: hw/i386/intel_iommu_internal.h:413:
+ dt_supported? \
^
total: 12 errors, 0 warnings, 62 lines checked
Commit 4cd505b83cac (intel_iommu: TM field should not be in reserved bits) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190927045838.2968-1-qi1.zhang@intel.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-27 9:32 ` Peter Xu
@ 2019-09-29 1:11 ` Zhang, Qi1
2019-09-29 2:02 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Zhang, Qi1 @ 2019-09-29 1:11 UTC (permalink / raw)
To: Peter Xu; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
> -----Original Message-----
> From: Peter Xu <peterx@redhat.com>
> Sent: Friday, September 27, 2019 5:32 PM
> To: Zhang, Qi1 <qi1.zhang@intel.com>
> Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> pbonzini@redhat.com; rth@twiddle.net; Qi, Yadong <yadong.qi@intel.com>
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Fri, Sep 27, 2019 at 08:03:21AM +0000, Zhang, Qi1 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peter Xu <peterx@redhat.com>
> > > Sent: Friday, September 27, 2019 2:10 PM
> > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > > pbonzini@redhat.com; rth@twiddle.net
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > > > From: "Zhang, Qi" <qi1.zhang@intel.com>
> > > >
> > > > When dt is supported, TM field should not be Reserved(0).
> > > >
> > > > Refer to VT-d Spec 9.8
> > > >
> > > > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > > > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > > > ---
> > > > hw/i386/intel_iommu.c | 12 ++++++------
> > > > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > ---
> VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > >aw_bits);
> > > > - vtd_paging_entry_rsvd_field[3] =
> > > >aw_bits);
> > > > + vtd_paging_entry_rsvd_field[5] =
> > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > + vtd_paging_entry_rsvd_field[6] =
> > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > + vtd_paging_entry_rsvd_field[7] =
> > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > >dt_supported);
> > > > vtd_paging_entry_rsvd_field[8] =
> > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > >
> > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 (1G).
> > > While this reminded me that I'm totally confused on why we have had
> > > entry 7, 8 after all... Are they really used?
> > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will update
> a new patchset for it.
>
> Could I ask why index 7 may be leaf?
Index 7 is PDPE 1G GB leaf.
>
> > >
> > > >
> > > > if (x86_iommu_ir_supported(x86_iommu)) { diff --git
> > > > a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > > > index c1235a7063..01f1aa6c86 100644
> > > > --- a/hw/i386/intel_iommu_internal.h
> > > > +++ b/hw/i386/intel_iommu_internal.h
> > > > @@ -387,19 +387,31 @@ typedef union VTDInvDesc VTDInvDesc;
> > > > #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
> > > >
> > > > /* Rsvd field masks for spte */
> > > > -#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw) \
> > > > +#define VTD_SPTE_PAGE_L1_RSVD_MASK(aw, dt_supported) \
> > > > + dt_supported? \
> > > > + (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
> > > VTD_SL_TM))
> > > > +: \
> > > > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
> > >
> > > This seems strange too in that ~VTD_HAW_MASK(aw) probably covered
> > > bits
> > > 63-48 for aw==48 case so it should already cover VTD_SL_TM?
> > VTD_SL_IGN_COM 0xbff0000000000000ULL, TM field is cleared by ~
> > VTD_SL_IGN_COM
> > >
> > > Meanwhile when I'm reading the spec I see at least bits 61-52
> > > ignored rather than reserved.
> > Yes. Bit 61~52 is ignored. Such as the index 5 of this array is 0xfff8000000800.
>
> Oops, my poor eye obviously didn't see that the "~" operator is applied over
> the whole (VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)... :)
>
> Btw, you should only touch up the macros that are leaves here.
> Non-leaves should still keep that bit as reserved.
Yes. I will.
>
> Thanks,
>
> --
> Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-29 1:11 ` Zhang, Qi1
@ 2019-09-29 2:02 ` Peter Xu
2019-09-29 2:10 ` Zhang, Qi1
2019-09-29 2:19 ` Peter Xu
0 siblings, 2 replies; 10+ messages in thread
From: Peter Xu @ 2019-09-29 2:02 UTC (permalink / raw)
To: Zhang, Qi1; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
On Sun, Sep 29, 2019 at 01:11:12AM +0000, Zhang, Qi1 wrote:
>
>
> > -----Original Message-----
> > From: Peter Xu <peterx@redhat.com>
> > Sent: Friday, September 27, 2019 5:32 PM
> > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > pbonzini@redhat.com; rth@twiddle.net; Qi, Yadong <yadong.qi@intel.com>
> > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> >
> > On Fri, Sep 27, 2019 at 08:03:21AM +0000, Zhang, Qi1 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Peter Xu <peterx@redhat.com>
> > > > Sent: Friday, September 27, 2019 2:10 PM
> > > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > > > pbonzini@redhat.com; rth@twiddle.net
> > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > > reserved bits
> > > >
> > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > > > > From: "Zhang, Qi" <qi1.zhang@intel.com>
> > > > >
> > > > > When dt is supported, TM field should not be Reserved(0).
> > > > >
> > > > > Refer to VT-d Spec 9.8
> > > > >
> > > > > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > > > > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > > > > ---
> > > > > hw/i386/intel_iommu.c | 12 ++++++------
> > > > > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > > ---
> > VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > > >aw_bits);
> > > > > - vtd_paging_entry_rsvd_field[3] =
> > > > >aw_bits);
> > > > > + vtd_paging_entry_rsvd_field[5] =
> > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > >dt_supported);
> > > > > + vtd_paging_entry_rsvd_field[6] =
> > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > >dt_supported);
> > > > > + vtd_paging_entry_rsvd_field[7] =
> > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > >dt_supported);
> > > > > vtd_paging_entry_rsvd_field[8] =
> > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > > >
> > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 (1G).
[1]
> > > > While this reminded me that I'm totally confused on why we have had
> > > > entry 7, 8 after all... Are they really used?
> > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will update
> > a new patchset for it.
> >
> > Could I ask why index 7 may be leaf?
> Index 7 is PDPE 1G GB leaf.
I thought 1G was index 6. I've listed my understanding above [1].
Would you please double confirm? Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-29 2:02 ` Peter Xu
@ 2019-09-29 2:10 ` Zhang, Qi1
2019-09-29 2:19 ` Peter Xu
1 sibling, 0 replies; 10+ messages in thread
From: Zhang, Qi1 @ 2019-09-29 2:10 UTC (permalink / raw)
To: Peter Xu; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
> -----Original Message-----
> From: Peter Xu <peterx@redhat.com>
> Sent: Sunday, September 29, 2019 10:02 AM
> To: Zhang, Qi1 <qi1.zhang@intel.com>
> Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> pbonzini@redhat.com; rth@twiddle.net; Qi, Yadong <yadong.qi@intel.com>
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Sun, Sep 29, 2019 at 01:11:12AM +0000, Zhang, Qi1 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peter Xu <peterx@redhat.com>
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > > pbonzini@redhat.com; rth@twiddle.net; Qi, Yadong
> > > <yadong.qi@intel.com>
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 08:03:21AM +0000, Zhang, Qi1 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Peter Xu <peterx@redhat.com>
> > > > > Sent: Friday, September 27, 2019 2:10 PM
> > > > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com;
> mst@redhat.com;
> > > > > pbonzini@redhat.com; rth@twiddle.net
> > > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > > > reserved bits
> > > > >
> > > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > > > > > From: "Zhang, Qi" <qi1.zhang@intel.com>
> > > > > >
> > > > > > When dt is supported, TM field should not be Reserved(0).
> > > > > >
> > > > > > Refer to VT-d Spec 9.8
> > > > > >
> > > > > > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > > > > > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > > > > > ---
> > > > > > hw/i386/intel_iommu.c | 12 ++++++------
> > > > > > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > > > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > > > ---
> > > VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > > > >aw_bits);
> > > > > > - vtd_paging_entry_rsvd_field[3] =
> > > > > >aw_bits);
> > > > > > + vtd_paging_entry_rsvd_field[5] =
> > > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > + vtd_paging_entry_rsvd_field[6] =
> > > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > + vtd_paging_entry_rsvd_field[7] =
> > > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > vtd_paging_entry_rsvd_field[8] =
> > > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > > > >
> > > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6
> (1G).
>
> [1]
>
> > > > > While this reminded me that I'm totally confused on why we have
> > > > > had entry 7, 8 after all... Are they really used?
> > > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be
> > > > leaf. Will update
> > > a new patchset for it.
> > >
> > > Could I ask why index 7 may be leaf?
> > Index 7 is PDPE 1G GB leaf.
>
> I thought 1G was index 6. I've listed my understanding above [1].
> Would you please double confirm? Thanks,
Check the existing function. When level is 3 VTD_SL_PDP_LEVEL and the entry is leaf, it is PDPE 1G leaf and the corresponding index of this array 7.
static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
{
if (slpte & VTD_SL_PT_PAGE_SIZE_MASK) {
/* Maybe large page */
return slpte & vtd_paging_entry_rsvd_field[level + 4];
} else {
return slpte & vtd_paging_entry_rsvd_field[level];
}
}
>
> --
> Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
2019-09-29 2:02 ` Peter Xu
2019-09-29 2:10 ` Zhang, Qi1
@ 2019-09-29 2:19 ` Peter Xu
1 sibling, 0 replies; 10+ messages in thread
From: Peter Xu @ 2019-09-29 2:19 UTC (permalink / raw)
To: Zhang, Qi1; +Cc: ehabkost, mst, qemu-devel, pbonzini, Qi, Yadong, rth
On Sun, Sep 29, 2019 at 10:02:20AM +0800, Peter Xu wrote:
> On Sun, Sep 29, 2019 at 01:11:12AM +0000, Zhang, Qi1 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peter Xu <peterx@redhat.com>
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > > pbonzini@redhat.com; rth@twiddle.net; Qi, Yadong <yadong.qi@intel.com>
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 08:03:21AM +0000, Zhang, Qi1 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Peter Xu <peterx@redhat.com>
> > > > > Sent: Friday, September 27, 2019 2:10 PM
> > > > > To: Zhang, Qi1 <qi1.zhang@intel.com>
> > > > > Cc: qemu-devel@nongnu.org; ehabkost@redhat.com; mst@redhat.com;
> > > > > pbonzini@redhat.com; rth@twiddle.net
> > > > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > > > reserved bits
> > > > >
> > > > > On Fri, Sep 27, 2019 at 12:58:38PM +0800, qi1.zhang@intel.com wrote:
> > > > > > From: "Zhang, Qi" <qi1.zhang@intel.com>
> > > > > >
> > > > > > When dt is supported, TM field should not be Reserved(0).
> > > > > >
> > > > > > Refer to VT-d Spec 9.8
> > > > > >
> > > > > > Signed-off-by: Zhang, Qi <qi1.zhang@intel.com>
> > > > > > Signed-off-by: Qi, Yadong <yadong.qi@intel.com>
> > > > > > ---
> > > > > > hw/i386/intel_iommu.c | 12 ++++++------
> > > > > > hw/i386/intel_iommu_internal.h | 25 +++++++++++++++++++------
> > > > > > 2 files changed, 25 insertions(+), 12 deletions(-)
> > > > > > ---
> > > VTD_SPTE_PAGE_L2_RSVD_MASK(s-
> > > > > >aw_bits);
> > > > > > - vtd_paging_entry_rsvd_field[3] =
> > > > > >aw_bits);
> > > > > > + vtd_paging_entry_rsvd_field[5] =
> > > > > VTD_SPTE_LPAGE_L1_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > + vtd_paging_entry_rsvd_field[6] =
> > > > > VTD_SPTE_LPAGE_L2_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > + vtd_paging_entry_rsvd_field[7] =
> > > > > > + VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits, x86_iommu-
> > > > > >dt_supported);
> > > > > > vtd_paging_entry_rsvd_field[8] =
> > > > > >VTD_SPTE_LPAGE_L4_RSVD_MASK(s->aw_bits);
> > > > >
> > > > > Should this TM bit only affects leaves? Say, entry 1 (4K), 5 (2M), 6 (1G).
>
> [1]
>
> > > > > While this reminded me that I'm totally confused on why we have had
> > > > > entry 7, 8 after all... Are they really used?
> > > > Yes. TM bit only affects. To this array, index 1, 5,6,7 may be leaf. Will update
> > > a new patchset for it.
> > >
> > > Could I ask why index 7 may be leaf?
> > Index 7 is PDPE 1G GB leaf.
>
> I thought 1G was index 6. I've listed my understanding above [1].
> Would you please double confirm? Thanks,
Oh wait, You are right... Index 6 should be for 1G because index 5
seems to be unused as well. However then again we should drop 5
instead of 7?
I think we can do this in two patches:
Patch 1 to clean these up by only let vtd_paging_rsvd (we can rename
it to shorter one like this if going to touch it) to keep reserved
bits for non-huge pages. Then we define a new struct
(e.g. vtd_paging_rsvd_huge) to only keep the huge page entries. The
thing is that I see no point in keeping huge && non-huge in a single
array (and I believe that's why it caused confusion so far...). That
new one should be a size of 2 array. Meanwhile we need to fix
vtd_slpte_nonzero_rsvd() too using the new arrays.
Then in patch 2 we do the DT bit change.
Does that look ok?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-29 2:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-27 4:58 [PATCH V2] intel_iommu: TM field should not be in reserved bits qi1.zhang
2019-09-27 6:10 ` Peter Xu
2019-09-27 8:03 ` Zhang, Qi1
2019-09-27 9:32 ` Peter Xu
2019-09-29 1:11 ` Zhang, Qi1
2019-09-29 2:02 ` Peter Xu
2019-09-29 2:10 ` Zhang, Qi1
2019-09-29 2:19 ` Peter Xu
2019-09-27 9:33 ` Michael S. Tsirkin
2019-09-27 20:44 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.