Re: [Qemu-devel] [PATCH v1 3/3] intel-iommu: search iotlb for levels supported by the address width.

[Yu Zhang]

On Mon, Nov 12, 2018 at 04:51:22PM +0800, Peter Xu wrote:
> On Fri, Nov 09, 2018 at 07:49:47PM +0800, Yu Zhang wrote:
> > This patch updates vtd_lookup_iotlb() to search cached mappings only
> > for all page levels supported by address width of current vIOMMU. Also,
> > to cover 57-bit width, the shift of source id(VTD_IOTLB_SID_SHIFT) and
> > of page level(VTD_IOTLB_LVL_SHIFT) are enlarged by 9 - the stride of
> > one paging structure level.
> > 
> > Signed-off-by: Yu Zhang <address@hidden>
> > ---
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Cc: Paolo Bonzini <address@hidden> 
> > Cc: Richard Henderson <address@hidden> 
> > Cc: Eduardo Habkost <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c          | 5 +++--
> >  hw/i386/intel_iommu_internal.h | 7 ++-----
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 9cdf755..ce7e17e 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -254,11 +254,12 @@ static uint64_t vtd_get_iotlb_gfn(hwaddr addr, 
> > uint32_t level)
> >  static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t 
> > source_id,
> >                                         hwaddr addr)
> >  {
> > -    VTDIOTLBEntry *entry;
> > +    VTDIOTLBEntry *entry = NULL;
> >      uint64_t key;
> >      int level;
> > +    int max_level = (s->aw_bits - VTD_PAGE_SHIFT_4K) / VTD_SL_LEVEL_BITS;
> >  
> > -    for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
> > +    for (level = VTD_SL_PT_LEVEL; level < max_level; level++) {
> 
> My understanding of current IOTLB is that it only caches the last
> level of mapping, say:
> 
>   - level 1: 4K page
>   - level 2: 2M page
>   - level 3: 1G page
> 
> So we don't check against level=4 even if x-aw-bits=48 is specified.
> 
> Here does it mean that we're going to have... 512G iommu huge pages?
> 

No. My bad, I misunderstood this routine. And now I believe we do not
need this patch. :-)

Thanks, Peter.

> >          key = vtd_get_iotlb_key(vtd_get_iotlb_gfn(addr, level),
> >                                  source_id, level);
> >          entry = g_hash_table_lookup(s->iotlb, &key);
> > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index a7ef24b..bdf2b7c 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -114,8 +114,8 @@
> >                                       VTD_INTERRUPT_ADDR_FIRST + 1)
> >  
> >  /* The shift of source_id in the key of IOTLB hash table */
> > -#define VTD_IOTLB_SID_SHIFT         36
> > -#define VTD_IOTLB_LVL_SHIFT         52
> > +#define VTD_IOTLB_SID_SHIFT         45
> > +#define VTD_IOTLB_LVL_SHIFT         61
> >  #define VTD_IOTLB_MAX_SIZE          1024    /* Max size of the hash table 
> > */
> >  
> >  /* IOTLB_REG */
> > @@ -450,9 +450,6 @@ typedef struct VTDRootEntry VTDRootEntry;
> >  #define VTD_SL_LEVEL_BITS           9
> >  
> >  /* Second Level Paging Structure */
> > -#define VTD_SL_PML4_LEVEL           4
> > -#define VTD_SL_PDP_LEVEL            3
> > -#define VTD_SL_PD_LEVEL             2
> >  #define VTD_SL_PT_LEVEL             1
> >  #define VTD_SL_PT_ENTRY_NR          512
> >  
> > -- 
> > 1.9.1
> > 
> 
> Regards,
> 
> -- 
> Peter Xu
> 

B.R.
Yu