Re: [dpdk-dev] [PATCH 2/2] eal: fix IOVA mode selection as VA for pci drivers

[Jerin Jacob Kollanukkaran <jerinj@marvell.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, July 15, 2019 9:36 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; David Marchand
> <david.marchand@redhat.com>; dev@dpdk.org; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>; Igor Russkikh
> <igor.russkikh@aquantia.com>; Pavel Belous <pavel.belous@aquantia.com>;
> Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> John Daley <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>;
> Qi Zhang <qi.z.zhang@intel.com>; Xiao Wang <xiao.w.wang@intel.com>;
> Beilei Xing <beilei.xing@intel.com>; Jingjing Wu <jingjing.wu@intel.com>;
> Qiming Yang <qiming.yang@intel.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Matan Azrad <matan@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; Yongseok Koh
> <yskoh@mellanox.com>; Viacheslav Ovsiienko
> <viacheslavo@mellanox.com>; Alejandro Lucero
> <alejandro.lucero@netronome.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Rasesh Mody <rmody@marvell.com>; Shahed
> Shaikh <shshaikh@marvell.com>; Bruce Richardson
> <bruce.richardson@intel.com>; alialnu@mellanox.com;
> aconole@redhat.com
> Subject: Re: [PATCH 2/2] eal: fix IOVA mode selection as VA for pci drivers
> 
> 15/07/2019 17:35, Jerin Jacob Kollanukkaran:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 15/07/2019 16:26, Jerin Jacob Kollanukkaran:
> > > > > > Is there any specific reason why we always prefer PA if
> > > > > > physical addresses are available? Since we're already assuming
> > > > > > that all devices support PA and VA anyway, what's the harm in
> > > > > > enabling VA by
> > > default?
> > > > >
> > > > > If PA is available, it means we are running as root.
> > > > > We can assume that using root is a choice, probably related to a
> > > > > preference for PA.
> > > >
> > > > # Even if we are running as root, Why to choose PA in case of DC?
> > > > ie. Following logic is not need
> > > >                 if (iova_mode == RTE_IOVA_DC) {
> > > >                         iova_mode = phys_addrs ? RTE_IOVA_PA : RTE_IOVA_VA;
> > > >                         RTE_LOG(DEBUG, EAL,
> > > >                                 "Buses did not request a specific IOVA mode, using '%s'
> > > based on physical addresses availability.\n",
> > > >                                 phys_addrs ? "PA" : "VA");
> > > >                 }
> > >
> > > Why running as root if using VA anyway?
> > > We can assume the user knows what he is doing, so it is a user choice.
> > > We want to allow the user choosing, right?
> >
> > The user can override iova=pa/va as eal argument if user needs to run a
> specific mode.
> > Running as root for various other reason(just be lazy) etc. it is not
> > or it should not be connected to set the mode as PA.
> 
> Good point.
> I tend to prefer avoiding the use of EAL arguments because they may be
> unavailable, depending on the application.

Yes. The default case suffice the requirement here.ie when it DC chosen from
bus layer select the VA. I don't see any point in overriding that.
It is a good default. Do you think any case where it need to be "changed"?
If not, let stick with VA i.e until unless if there no HARD requirement for PA.
i.e Stayaway from PA WHEN possible.

> 
> > > > # When DPDK running on guest, Anyway it can not access the real
> > > > PA, It will
> > > be IPA.
> > >
> > > What is IPA? Isn't it a beer?
> >
> > There may a beer with that name. In this context, it is "Intermediate
> physical address"
> >
> > > > So I don't understand logic behind choose PA when DC.
> > > > To me, it make sense to choose PA when DC.
> > >
> > > You probably mean "choose VA".
> >
> > Yup.
> >
> > > > # To align with RTE_PCI_DRV_NEED_MAPPING flag and reflect it "need"
> > > > rather than support, I think, flag can be changed to
> > > > RTE_PCI_DRV_NEED_IOVA_AS_VA
> > >
> > > I think the most important is to have a good documentation of this
> > > flag (it was not done properly when Cavium introduced it initially).
> > > If you want to rename the flag, you can do it in a separate patch.
> > > If renaming, I really would like to get an answer to an old question:
> > > Why IO adress is called IOVA? The name "IOVA_AS_VA" looks strange.
> >
> > IOVA = IO virtual address
> > Since IOVA can be PA or VA, the name IOVA_AS_VA as chosen
> 
> We could also call it "bus address" or "device address".
> I think the word "IOVA" was enforced by Linux.
> Anyway, my real issue when using "virtual" is that we don't really know what
> we are talking about: is it an IOMMU translated address on the device side or
> an MMU translated address on the application side?

Actually in linux kernel, it creates the same mapping for device and CPU so 
user both can access the very same address. In this context, IOVA means,
Virtual address for device. The OS can do same mapping in CPU MMU tables as well.

> 
> I think we should better explain things.
> One diagram which can help:
> https://en.wikipedia.org/wiki/Input%E2%80%93output_memory_managem
> ent_unit#/media/File:MMU_and_IOMMU.svg
> 
> > > For reference, one description of addressing:
> > > https://lists.linuxfoundation.org/pipermail/iommu/2018-May/027686.ht
> > > ml
> > >
> > > About the naming, do you remember how I insisted to have a correct
> > > naming of all related stuff in DPDK? It was hard to get it accepted,
> > > the discussion was not nice and I stopped insisting to get all details fine
> because I just got bored.
> > > It was a really bad experience.
> >
> > I agree.
> > To me that bad experience was due to mostly not having enough
> > technical comments On the proposal. Though I am not the author/owner of
> it.
> >
> > > You can ask why I remind this now? Because we must take care of all
> > > details, make sure our messages are well understood, and be
> cooperative.
> >
> > No disagreement.
> > If we see the history the meaning got changed/updated in this commit
> > By adding intel drivers to it. I would nt say  it is big ideal, It
> > just C code, It can be changed based on the need. I think, what really
> > import is, maintain the the feature and commitment towards fixing any
> issue.
> >
> > commit f37dfab21c988d2d0ecb3c82be4ba9738c7e51c7
> > Author: Jianfeng Tan <jianfeng.tan@intel.com>
> > Date:   Wed Oct 11 10:33:48 2017 +0000
> >
> >     drivers/net: enable IOVA mode for Intel PMDs
> >
> >     If we want to enable IOVA mode, introduced by
> >     commit 93878cf0255e ("eal: introduce helper API for IOVA mode"),
> >     we need PMDs (for PCI devices) to expose this flag.
> >
> >     Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >     Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >     Reviewed-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> 
> The doxygen meaning did not change from day one:
> 	/** Device driver supports IOVA as VA */ But the commit log
> meaning was:
> 	"Flag used when driver needs to operate in iova=va mode."
> And the Intel commit log had a different understanding:
> 	"If we want to enable IOVA mode, [..] we need PMDs [..] to expose
> this flag."
> 
> Anyway we agree on the new meaning to be the original one the author had
> in mind (i.e. "driver needs").
> 
> > > > Other than above points,
> > > > Reviewed this patch and tested on octeontx2, It looks good to me.
> 
>