RE: [PATCH v1 0/2] TM field check failed
Hello, everyone May I know you have comments on this patch set? Thank you! BRs Qi Zhang > -Original Message- > From: Zhang, Qi1 > Sent: Tuesday, October 8, 2019 10:35 AM > To: qemu-devel@nongnu.org > Cc: m...@redhat.com; marcel.apfelb...@gmail.com; pbonz...@redhat.com; > r...@twiddle.net; ehabk...@redhat.com; Zhang, Qi1 ; > Qi, Yadong > Subject: [PATCH v1 0/2] TM field check failed > > From: "Zhang, Qi" > > spilt the reserved fields arrays and remove TM field from reserved bits > > Changelog V1: > add descriptons > > Zhang, Qi (2): > intel_iommu: split the resevred fields arrays into two ones > intel_iommu: TM field should not be in reserved bits > > hw/i386/intel_iommu.c | 35 -- > hw/i386/intel_iommu_internal.h | 17 + > 2 files changed, 34 insertions(+), 18 deletions(-) > > -- > 2.20.1
RE: [PATCH v4] intel_iommu: TM field should not be in reserved bits
Thank you! I formatted the patch with cover-letter and submitted them again.
> -Original Message-
> From: Michael S. Tsirkin
> Sent: Sunday, October 6, 2019 5:50 AM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; marcel.apfelb...@gmail.com;
> pbonz...@redhat.com; r...@twiddle.net; ehabk...@redhat.com; Qi, Yadong
>
> Subject: Re: [PATCH v4] intel_iommu: TM field should not be in reserved bits
>
> On Mon, Sep 30, 2019 at 01:04:51PM +0800, qi1.zh...@intel.com wrote:
> > From: "Zhang, Qi"
> >
> > When dt is supported, TM field should not be Reserved(0).
> >
> > Refer to VT-d Spec 9.8
> >
> > Signed-off-by: Zhang, Qi
> > Signed-off-by: Qi, Yadong
>
> I am guessing this is really a 2 patch series right?
> So you need to format it as such with
> git format-patch --cover-letter --thread=shallow
>
> > ---
> > hw/i386/intel_iommu.c | 12
> > hw/i386/intel_iommu_internal.h | 17 +
> > 2 files changed, 21 insertions(+), 8 deletions(-)
> > ---
> > Changelog V2:
> > move dt_supported flag to VTD_SPTE_PAGE_LX_RSVD_MASK and
> > VTD_SPTE_LPAGE_LX_RSVD_MASK Changelog V3:
> > based on the change to split the arrays into two ones Changelog V4:
> > style error check
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > a118efaeaf..d62604ece3 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3549,15 +3549,19 @@ static void vtd_init(IntelIOMMUState *s)
> > * Rsvd field masks for spte
> > */
> > vtd_spte_rsvd[0] = ~0ULL;
> > -vtd_spte_rsvd[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits);
> > +vtd_spte_rsvd[1] = VTD_SPTE_PAGE_L1_RSVD_MASK(s->aw_bits,
> > +
> > + x86_iommu->dt_supported);
> > vtd_spte_rsvd[2] = VTD_SPTE_PAGE_L2_RSVD_MASK(s->aw_bits);
> > vtd_spte_rsvd[3] = VTD_SPTE_PAGE_L3_RSVD_MASK(s->aw_bits);
> > vtd_spte_rsvd[4] = VTD_SPTE_PAGE_L4_RSVD_MASK(s->aw_bits);
> >
> > vtd_spte_rsvd_large[0] = ~0ULL;
> > -vtd_spte_rsvd_large[1] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s-
> >aw_bits);
> > -vtd_spte_rsvd_large[2] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s-
> >aw_bits);
> > -vtd_spte_rsvd_large[3] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s-
> >aw_bits);
> > +vtd_spte_rsvd_large[1] = VTD_SPTE_LPAGE_L1_RSVD_MASK(s-
> >aw_bits,
> > +
> > x86_iommu->dt_supported);
> > +vtd_spte_rsvd_large[2] = VTD_SPTE_LPAGE_L2_RSVD_MASK(s-
> >aw_bits,
> > +
> > x86_iommu->dt_supported);
> > +vtd_spte_rsvd_large[3] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s-
> >aw_bits,
> > +
> > + x86_iommu->dt_supported);
> > vtd_spte_rsvd_large[4] = 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..3a839a8925 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -387,7 +387,9 @@ typedef union VTDInvDesc VTDInvDesc; #define
> > VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffe0fff8
> >
> > /* 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) \
> > (0x800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) @@ -
> 395,11
> > +397,17 @@ typedef union VTDInvDesc VTDInvDesc;
> > (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)
RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> -Original Message-
> From: Peter Xu
> Sent: Sunday, September 29, 2019 10:02 AM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Sun, Sep 29, 2019 at 01:11:12AM +, Zhang, Qi1 wrote:
> >
> >
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > To: Zhang, Qi1
> > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> > > pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> > >
> > > Subject: Re: [PATCH V2] intel_iommu: TM field should not be in
> > > reserved bits
> > >
> > > On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote:
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Peter Xu
> > > > > Sent: Friday, September 27, 2019 2:10 PM
> > > > > To: Zhang, Qi1
> > > > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com;
> m...@redhat.com;
> > > > > pbonz...@redhat.com; r...@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.zh...@intel.com wrote:
> > > > > > From: "Zhang, Qi"
> > > > > >
> > > > > > When dt is supported, TM field should not be Reserved(0).
> > > > > >
> > > > > > Refer to VT-d Spec 9.8
> > > > > >
> > > > > > Signed-off-by: Zhang, Qi
> > > > > > Signed-off-by: Qi, Yadong
> > > > > > ---
> > > > > > 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
RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> -Original Message-
> From: Peter Xu
> Sent: Friday, September 27, 2019 5:32 PM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> pbonz...@redhat.com; r...@twiddle.net; Qi, Yadong
> Subject: Re: [PATCH V2] intel_iommu: TM field should not be in reserved bits
>
> On Fri, Sep 27, 2019 at 08:03:21AM +, Zhang, Qi1 wrote:
> >
> >
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Friday, September 27, 2019 2:10 PM
> > > To: Zhang, Qi1
> > > Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> > > pbonz...@redhat.com; r...@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.zh...@intel.com wrote:
> > > > From: "Zhang, Qi"
> > > >
> > > > When dt is supported, TM field should not be Reserved(0).
> > > >
> > > > Refer to VT-d Spec 9.8
> > > >
> > > > Signed-off-by: Zhang, Qi
> > > > Signed-off-by: Qi, Yadong
> > > > ---
> > > > 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 0xffe0fff8
> > > >
> > > > /* 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 0xbff0ULL, 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
> > 0xfff800800.
>
> 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
RE: [PATCH V2] intel_iommu: TM field should not be in reserved bits
> -Original Message-
> From: Peter Xu
> Sent: Friday, September 27, 2019 2:10 PM
> To: Zhang, Qi1
> Cc: qemu-devel@nongnu.org; ehabk...@redhat.com; m...@redhat.com;
> pbonz...@redhat.com; r...@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.zh...@intel.com wrote:
> > From: "Zhang, Qi"
> >
> > When dt is supported, TM field should not be Reserved(0).
> >
> > Refer to VT-d Spec 9.8
> >
> > Signed-off-by: Zhang, Qi
> > Signed-off-by: Qi, Yadong
> > ---
> > 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 0xffe0fff8
> >
> > /* 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 0xbff0ULL, 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 0xfff800800.
>
> 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_suppo
