Re: [Qemu-devel] [PATCH RFC v3 03/14] intel_iommu: renaming gpa to iova where proper

[Peter Xu]

On Fri, Jan 20, 2017 at 08:27:31AM +0000, Tian, Kevin wrote:
> > From: Peter Xu [mailto:address@hidden
> > Sent: Friday, January 13, 2017 11:06 AM
> > 
> > There are lots of places in current intel_iommu.c codes that named
> > "iova" as "gpa". It is really confusing to use a name "gpa" in these
> > places (which is very easily to be understood as "Guest Physical
> > Address", while it's not). To make the codes (much) easier to be read, I
> > decided to do this once and for all.
> > 
> > No functional change is made. Only literal ones.
> 
> If looking at VT-d spec (3.2 Domains and Address Translation)
> 
>       Remapping hardware treats the address in inbound requests as DMA 
>       Address. Depending on the software usage model, the DMA address 
>       space may be the Guest-Physical Address (GPA) space of the virtual 
>       machine to which the device is assigned, or application Virtual Address 
>       (VA) space defined by the PASID assigned to an application, or some 
>       abstract I/O virtual address (IOVA) space defined by software.
> 
>       For simplicity, this document refers to address in requests-without-
>       PASID as GPA, and address in requests-with-PASID as Virtual Address 
>       (VA) (or Guest Virtual Address (GVA), if such request is from a device 
>       assigned to a virtual machine). The translated address is referred to 
> as 
>       HPA.
> 
> It will add more readability if similar comment is added in this file - you
> can say choosing iova to represent address in requests-without-PASID.

I agree that the code will be more readable if we can explain all the
bits in detail.

However this patch is not adding comments, but to "fix" an existing
literal problem, which is very misleading to people reading the codes
for the first time. The places touched in this patch was doing the
namings incorrectly, so I just corrected them. So even if we want to
have more comments on explaining the bits, IMHO it'll be nicer to use
a separate patch, not squashing all the things into a single one.

If you won't disagree, I'd like to keep this single patch as-it-is, to
make sure this series can converge soon (I will be sorry since I'll
extend this series a bit, I hope that won't extend the review process
too long for it). After that, we can add more documentations if
needed.

Thanks,

-- peterx