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: 05 September 2018 10:39
> 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 05.09.18 at 11:13, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 05 September 2018 08:12
> >> To: Kevin Tian <kevin.tian@intel.com>
> >> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien Grall
> >> <julien.grall@arm.com>; Paul Durrant <Paul.Durrant@citrix.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 05.09.18 at 08:56, <kevin.tian@intel.com> wrote:
> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Wednesday, September 5, 2018 2:49 PM
> >> >>
> >> >> >>> On 05.09.18 at 02:42, <kevin.tian@intel.com> wrote:
> >> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> >> Sent: Tuesday, September 4, 2018 5:08 PM
> >> >> >>
> >> >> >> >>> On 04.09.18 at 10:49, <Paul.Durrant@citrix.com> wrote:
> >> >> >> >>  -----Original Message-----
> >> >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> >> >> Sent: 04 September 2018 09:47
> >> >> >> >> To: Kevin Tian <kevin.tian@intel.com>
> >> >> >> >> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>;
> >> Julien
> >> >> >> Grall
> >> >> >> >> <julien.grall@arm.com>; Paul Durrant
> <Paul.Durrant@citrix.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 04.09.18 at 10:37, <kevin.tian@intel.com> wrote:
> >> >> >> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> >> >> >> Sent: Tuesday, September 4, 2018 4:33 PM
> >> >> >> >> >> >
> >> >> >> >> >> > bus address is commonly used along with physical/virtual
> >> >> address,
> >> >> >> to
> >> >> >> >> >> > represent different views between devices and CPU. From
> >> that
> >> >> >> angle
> >> >> >> >> >> > I think BFN is a clear term in this context. btw it is not
> >> necessary
> >> >> to
> >> >> >> >> >> > differentiate GBFN and MBFN since there is only one BFN
> view
> >> >> per
> >> >> >> >> >> > device.
> >> >> >> >> >>
> >> >> >> >> >> Sure, but you neglect the presence of one or more IOMMUs
> >> when
> >> >> >> >> >> you say "between devices and CPU". There addresses prior
> to
> >> and
> >> >> >> >> >> after IOMMU translation are distinct, and while the one
> before
> >> the
> >> >> >> >> >> translation matches the device view, the one after translation
> >> does
> >> >> >> >> >> not necessarily match the CPU view. Hence there are two
> "bus"
> >> >> >> >> >> frame numbers here - one representing the device view, and
> >> the
> >> >> >> >> >> other representing the IOMMU (output) view.
> >> >> >> >> >>
> >> >> >> >> >
> >> >> >> >> > I didn't get. the output address from IOMMU is the one sent
> to
> >> >> >> >> > memory controller, same as the one sent from CPU.
> >> >> >> >>
> >> >> >> >> That's on present x86 systems, but aiui not in the general case.
> The
> >> >> >> >> terminology to be used in Xen should fit the general case
> though.
> >> >> >> >
> >> >> >> > So your concern is cascaded IOMMUs?
> >> >> >>
> >> >> >> Not primarily. My concern are systems with an I/O address space
> >> >> >> (behind the IOMMU) distinct from the CPU address space. Iirc at
> >> >> >> least Alpha is/was that way.
> >> >> >>
> >> >> >
> >> >> > Then Paul please documents clearly that this bus address refers to
> >> >> > the input side of IOMMU. :-)
> >> >>
> >> >> But when reading code you can't always go back to look at the one
> >> >> place where its meaning is documented. Hence my desire for a name
> >> >> which properly conveys the meaning.
> >> >>
> >> >
> >> > Then possibly go back to DFN, but take 'D' as DMA instead of device?
> >>
> >> How would "DMA" be any better than "bus"? Whose view it is then still
> >> is unclear.
> >>
> >
> > 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.

> 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.

  Paul

>              mfn = next_table_mfn;
> 
>              /* allocate lower level page table */
> 
> Jan


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