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

[Peter Xu]

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 <address@hidden>
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > To: Zhang, Qi1 <address@hidden>
> > > Cc: address@hidden; address@hidden; address@hidden;
> > > address@hidden; address@hidden; Qi, Yadong <address@hidden>
> > > 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 <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(-)
> > > > > > ---
> > > 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