Re: [PATCH v6 01/14] iommu: introduce the concept of BFN...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 06 September 2018 14:13
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien Grall
> <julien.grall@arm.com>; Kevin Tian <kevin.tian@intel.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [Xen-devel] [PATCH v6 01/14] iommu: introduce the concept of
> BFN...
> 
> >>> On 06.09.18 at 12:36, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 05 September 2018 10:39
> >>
> >> >>> On 05.09.18 at 11:13, <Paul.Durrant@citrix.com> wrote:
> >> > Personally I think 'bus address' is commonly enough used term for
> >> addresses
> >> > used by devices for DMA. Indeed we have already 'dev_bus_addr' in
> the
> >> grant
> >> > map and unmap hypercalls. So baddr and bfn seem like ok terms to me.
> It's
> >> > also not impossible to rename these later if they prove problematic.
> >>
> >> But that's the point - the names are problematic (to me): I permanently
> >> have to remind myself that they do _not_ refer to the addresses as
> >> seen when accessing memory, but the ones going _into_ the IOMMU.
> >
> > Ok. Could we agree on 'IOFN' then? I think 'iova' and 'io address' are also
> > reasonably widely used terms to refer to address from a device's PoV. I'd
> > really like to unblock these early patches.
> 
> Hmm, earlier I had indicated I'd prefer DFN (as this make clear whose
> view we are talking about). Kevin seemed to prefer DFN too, just with
> a different association for D (which, as said, I consider unhelpful). So
> is there a particular reason you're now suggesting IOFN nevertheless?

It was the ambiguity and lack of agreement over the 'D' that made me think that the other alternative would be better.
Kevin, would you be ok with 'IOFN'?

> 
> >> The confusion (on my part) arises every time I see a mixture of gfn, bfn,
> >> and mfn in the same patch, perhaps including some 1:1-ness assumptions
> >> between pairs of them.
> >>
> >> Take these two hunks as example (mixing in some pfn as well):
> >>
> >> @@ -436,7 +436,7 @@ static int iommu_merge_pages(struct domain *d,
> >> unsigned long pt_mfn,
> >>   * {Re, un}mapping super page frames causes re-allocation of io
> >>   * page tables.
> >>   */
> >> -static int iommu_pde_from_gfn(struct domain *d, unsigned long pfn,
> >> +static int iommu_pde_from_bfn(struct domain *d, unsigned long pfn,
> >>                                unsigned long pt_mfn[])
> >>  {
> >>      u64 *pde, *next_table_vaddr;
> >> @@ -477,11 +477,11 @@ static int iommu_pde_from_gfn(struct domain
> *d,
> >> unsigned long pfn,
> >>               next_table_mfn != 0 )
> >>          {
> >>              int i;
> >> -            unsigned long mfn, gfn;
> >> +            unsigned long mfn, bfn;
> >>              unsigned int page_sz;
> >>
> >>              page_sz = 1 << (PTE_PER_TABLE_SHIFT * (next_level - 1));
> >> -            gfn =  pfn & ~((1 << (PTE_PER_TABLE_SHIFT * next_level)) - 1);
> >> +            bfn =  pfn & ~((1 << (PTE_PER_TABLE_SHIFT * next_level)) - 1);
> >
> > This is not wonderful code, agreed. In this particular case it looks like I
> > may be able to just rename the pfn argument to iofn (assuming we go with
> that
> > name) and lose the stack variable, if that helps.
> 
> Renaming the parameter will likely help, I agree. Getting rid of the
> local variable, otoh, I'm not sure is going to work (you need to retain
> the function parameter's original value for the next iteration of the
> outer loop).

Oh, yes I missed that. I'll see what I can do once we have agreement on a name.

  Paul

> 


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