[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 01/14] iommu: introduce the concept of BFN...
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 05 September 2018 10:39 > 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 <xen- > devel@xxxxxxxxxxxxxxxxxxxx> > Subject: RE: [Xen-devel] [PATCH v6 01/14] iommu: introduce the concept of > BFN... > > >>> On 05.09.18 at 11:13, <Paul.Durrant@xxxxxxxxxx> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > >> Sent: 05 September 2018 08:12 > >> To: Kevin Tian <kevin.tian@xxxxxxxxx> > >> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Julien Grall > >> <julien.grall@xxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; > Stefano > >> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen- > >> devel@xxxxxxxxxxxxxxxxxxxx> > >> Subject: RE: [Xen-devel] [PATCH v6 01/14] iommu: introduce the concept > of > >> BFN... > >> > >> >>> On 05.09.18 at 08:56, <kevin.tian@xxxxxxxxx> wrote: > >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > >> >> Sent: Wednesday, September 5, 2018 2:49 PM > >> >> > >> >> >>> On 05.09.18 at 02:42, <kevin.tian@xxxxxxxxx> wrote: > >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > >> >> >> Sent: Tuesday, September 4, 2018 5:08 PM > >> >> >> > >> >> >> >>> On 04.09.18 at 10:49, <Paul.Durrant@xxxxxxxxxx> wrote: > >> >> >> >> -----Original Message----- > >> >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > >> >> >> >> Sent: 04 September 2018 09:47 > >> >> >> >> To: Kevin Tian <kevin.tian@xxxxxxxxx> > >> >> >> >> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; > >> Julien > >> >> >> Grall > >> >> >> >> <julien.grall@xxxxxxx>; Paul Durrant > <Paul.Durrant@xxxxxxxxxx>; > >> >> >> Stefano > >> >> >> >> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen- > >> >> >> >> devel@xxxxxxxxxxxxxxxxxxxx> > >> >> >> >> Subject: Re: [Xen-devel] [PATCH v6 01/14] iommu: introduce the > >> >> concept > >> >> >> of > >> >> >> >> BFN... > >> >> >> >> > >> >> >> >> >>> On 04.09.18 at 10:37, <kevin.tian@xxxxxxxxx> wrote: > >> >> >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > >> >> >> >> >> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |