Re: [Xen-devel] [PATCH 1/7] iommu: introduce the concept of BFN...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 15 March 2018 13:40
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/7] iommu: introduce the concept of BFN...
> 
> >>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -367,9 +367,9 @@ void amd_iommu_flush_all_pages(struct domain
> *d)
> >  }
> >
> >  void amd_iommu_flush_pages(struct domain *d,
> > -                           unsigned long gfn, unsigned int order)
> > +                           unsigned long bfn, unsigned int order)
> >  {
> > -    _amd_iommu_flush_pages(d, (uint64_t) gfn << PAGE_SHIFT, order);
> > +    _amd_iommu_flush_pages(d, (uint64_t) bfn << PAGE_SHIFT, order);
> >  }
> 
> I assume you've simply used sed or alike to do the replacements,
> but we prefer to make style corrections at the same time when
> already touching a line: There's a stray space after the cast here,
> and really this wants to be bfn_to_baddr() (which then also
> shouldn't use the MMU's PAGE_SHIFT).
> 

I guess I'll add IOMMU_PAGE_SHIFT/MASK definitions and use those in a new 
bfn_to_baddr()/baddr_to_bfn() pair.

> > @@ -651,34 +651,34 @@ int amd_iommu_map_page(struct domain *d,
> unsigned long gfn, unsigned long mfn,
> >      if ( rc )
> >      {
> >          spin_unlock(&hd->arch.mapping_lock);
> > -        AMD_IOMMU_DEBUG("Root table alloc failed, gfn = %lx\n", gfn);
> > +        AMD_IOMMU_DEBUG("Root table alloc failed, bfn = %lx\n", bfn);
> >          domain_crash(d);
> >          return rc;
> >      }
> >
> >      /* Since HVM domain is initialized with 2 level IO page table,
> > -     * we might need a deeper page table for lager gfn now */
> > +     * we might need a deeper page table for lager bfn now */
> 
> Similarly here: Mind making this say "larger" (or "wider")? There's at
> least one more instance further down.
> 

Sure.

> > @@ -2763,10 +2763,10 @@ static int __must_check
> arm_smmu_map_page(struct domain *d, unsigned long gfn,
> >      * The function guest_physmap_add_entry replaces the current
> mapping
> >      * if there is already one...
> >      */
> > -   return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
> > +   return guest_physmap_add_entry(d, _gfn(bfn), _mfn(mfn), 0, t);
> 
> Hmm, very bad a change, but I presume unavoidable. I'd prefer if
> such could at least be accompanied by a comment clarifying why
> this mix of address spaces is correct in the specific case.
> 

I'll add such a comment stating the 1:1 mapping.

> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -23,11 +23,15 @@
> >  #include <xen/page-defs.h>
> >  #include <xen/spinlock.h>
> >  #include <xen/pci.h>
> > +#include <xen/typesafe.h>
> >  #include <public/hvm/ioreq.h>
> >  #include <public/domctl.h>
> >  #include <asm/device.h>
> >  #include <asm/iommu.h>
> >
> > +TYPE_SAFE(unsigned long, bfn);
> > +#define INVALID_BFN      _bfn(~0UL)
> 
> Please accompany this by a grep fodder (like the others have) and
> perhaps also PRI_bfn. And while the type definition logically belongs
> here, you will also want to add bfn_t with a description of its
> purpose into the comment at the top of xen/mm.h. I guess you'll
> need to replace / amend "host" in the MFN description there at the
> same time.
> 

Should I move the TYPE_SAFE evaluation to xen/mm.h then? If I leave it here 
then I'll presumably need some ifdef hackery in mm.h if you want be to define 
bfn_t there too.

> I ask for this in particular because the description saying "mapped
> in the IOMMU rather than the MMU" is ambiguous: Is it the input
> frame number, or the output one (and things are even more
> complicated when IOMMUs do two stages of translation). That in
> turn affects whether I'd consider correct some of the changes
> done elsewhere in this patch.
> 

Ok.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel