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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.18 at 12:36, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 05 September 2018 10:39
>> 
>> >>> On 05.09.18 at 11:13, <Paul.Durrant@xxxxxxxxxx> 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?

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



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