RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits

[Zhang, Qi1]

> -----Original Message-----
> From: Peter Xu <address@hidden>
> Sent: Friday, September 27, 2019 2:10 PM
> To: Zhang, Qi1 <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> 
> On Fri, Sep 27, 2019 at 12:58:38PM +0800, address@hidden wrote:
> > From: "Zhang, Qi" <address@hidden>
> >
> > When dt is supported, TM field should not be Reserved(0).
> >
> > Refer to VT-d Spec 9.8
> >
> > Signed-off-by: Zhang, Qi <address@hidden>
> > Signed-off-by: Qi, Yadong <address@hidden>
> > ---
> >  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