* [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
@ 2017-10-31 15:59 Jonas Pfefferle
2017-11-06 20:25 ` Thomas Monjalon
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Jonas Pfefferle @ 2017-10-31 15:59 UTC (permalink / raw)
To: dev; +Cc: anatoly.burakov, Jonas Pfefferle
Check and report errors on open/read in noiommu check.
Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
---
lib/librte_eal/linuxapp/eal/eal_vfio.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
index 5bbcdf9..80afdb3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
@@ -843,20 +843,33 @@ vfio_noiommu_dma_map(int __rte_unused vfio_container_fd)
int
vfio_noiommu_is_enabled(void)
{
- int fd, ret, cnt __rte_unused;
+ int fd;
+ ssize_t cnt;
char c;
- ret = -1;
fd = open(VFIO_NOIOMMU_MODE, O_RDONLY);
- if (fd < 0)
- return -1;
+ if (fd < 0) {
+ if (errno != ENOENT) {
+ RTE_LOG(ERR, EAL, " cannot open vfio noiommu file %i (%s)\n",
+ errno, strerror(errno));
+ return -1;
+ }
+ /*
+ * else the file does not exists
+ * i.e. noiommu is not enabled
+ */
+ return 0;
+ }
cnt = read(fd, &c, 1);
- if (c == 'Y')
- ret = 1;
-
close(fd);
- return ret;
+ if (cnt != 1) {
+ RTE_LOG(ERR, EAL, " unable to read from vfio noiommu "
+ "file %i (%s)\n", errno, strerror(errno));
+ return -1;
+ }
+
+ return c == 'Y';
}
#endif
--
2.7.4
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-10-31 15:59 [dpdk-dev] [PATCH v2] vfio: noiommu check error handling Jonas Pfefferle
@ 2017-11-06 20:25 ` Thomas Monjalon
2017-11-07 9:05 ` Jonas Pfefferle1
2018-01-13 12:05 ` Burakov, Anatoly
2018-01-19 17:37 ` Maxime Coquelin
2 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2017-11-06 20:25 UTC (permalink / raw)
To: Jonas Pfefferle, anatoly.burakov; +Cc: dev
31/10/2017 16:59, Jonas Pfefferle:
> Check and report errors on open/read in noiommu check.
>
> Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
I cannot decide to apply this patch as it does not explain what
it is fixing, and as it is not reviewed.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-11-06 20:25 ` Thomas Monjalon
@ 2017-11-07 9:05 ` Jonas Pfefferle1
2017-11-07 9:40 ` Thomas Monjalon
0 siblings, 1 reply; 20+ messages in thread
From: Jonas Pfefferle1 @ 2017-11-07 9:05 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: anatoly.burakov, dev
Thomas Monjalon <thomas@monjalon.net> wrote on 11/06/2017 09:25:15 PM:
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jonas Pfefferle <jpf@zurich.ibm.com>, anatoly.burakov@intel.com
> Cc: dev@dpdk.org
> Date: 11/06/2017 09:55 PM
> Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
>
> 31/10/2017 16:59, Jonas Pfefferle:
> > Check and report errors on open/read in noiommu check.
> >
> > Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
>
> I cannot decide to apply this patch as it does not explain what
> it is fixing, and as it is not reviewed.
>
This patch adds error handling and logging to the noiommu check.
Also, on older kernels when the noiommu_enable file does not exist it
assumes noiommu is not enabled instead of returning -1.
Note that in rte_pci_get_iommu_class (drivers/bus/pci/linux/pci.c)
is the only usage of the function and it assumes return == 1:
noiommu is enabled any other return value noiommu disabled, i.e.
my code change does not change the behavior of this function.
We might want to check for errors in rte_pci_get_iommu_class
as well since assuming it is not enabled when we cannot open
and read it might lead to iova == VA being used even if noiommu is
enabled.
All this comes back to what I proposed before: instead of
the noiommu and PPC64 check we should decide which iova mode
to use depending on the iommu types available.
The type should be already available at the point where we
decide on the iova type.
(iommu types supported is checked by vfio_get_container_fd)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-11-07 9:05 ` Jonas Pfefferle1
@ 2017-11-07 9:40 ` Thomas Monjalon
2017-11-07 9:50 ` Jonas Pfefferle1
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2017-11-07 9:40 UTC (permalink / raw)
To: Jonas Pfefferle1, anatoly.burakov; +Cc: dev
07/11/2017 10:05, Jonas Pfefferle1:
> Thomas Monjalon <thomas@monjalon.net> wrote on 11/06/2017 09:25:15 PM:
>
> > From: Thomas Monjalon <thomas@monjalon.net>
> > To: Jonas Pfefferle <jpf@zurich.ibm.com>, anatoly.burakov@intel.com
> > Cc: dev@dpdk.org
> > Date: 11/06/2017 09:55 PM
> > Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
> >
> > 31/10/2017 16:59, Jonas Pfefferle:
> > > Check and report errors on open/read in noiommu check.
> > >
> > > Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> >
> > I cannot decide to apply this patch as it does not explain what
> > it is fixing, and as it is not reviewed.
> >
>
> This patch adds error handling and logging to the noiommu check.
> Also, on older kernels when the noiommu_enable file does not exist it
> assumes noiommu is not enabled instead of returning -1.
> Note that in rte_pci_get_iommu_class (drivers/bus/pci/linux/pci.c)
> is the only usage of the function and it assumes return == 1:
> noiommu is enabled any other return value noiommu disabled, i.e.
> my code change does not change the behavior of this function.
> We might want to check for errors in rte_pci_get_iommu_class
> as well since assuming it is not enabled when we cannot open
> and read it might lead to iova == VA being used even if noiommu is
> enabled.
>
> All this comes back to what I proposed before: instead of
> the noiommu and PPC64 check we should decide which iova mode
> to use depending on the iommu types available.
> The type should be already available at the point where we
> decide on the iova type.
> (iommu types supported is checked by vfio_get_container_fd)
Is there something urgent for 17.11?
Or can it be refined in 18.02?
Anatoly, any thought?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-11-07 9:40 ` Thomas Monjalon
@ 2017-11-07 9:50 ` Jonas Pfefferle1
2018-01-11 23:45 ` Thomas Monjalon
0 siblings, 1 reply; 20+ messages in thread
From: Jonas Pfefferle1 @ 2017-11-07 9:50 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: anatoly.burakov, dev
Thomas Monjalon <thomas@monjalon.net> wrote on 11/07/2017 10:40:15 AM:
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jonas Pfefferle1 <JPF@zurich.ibm.com>, anatoly.burakov@intel.com
> Cc: dev@dpdk.org
> Date: 11/07/2017 10:40 AM
> Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
>
> 07/11/2017 10:05, Jonas Pfefferle1:
> > Thomas Monjalon <thomas@monjalon.net> wrote on 11/06/2017 09:25:15 PM:
> >
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > To: Jonas Pfefferle <jpf@zurich.ibm.com>, anatoly.burakov@intel.com
> > > Cc: dev@dpdk.org
> > > Date: 11/06/2017 09:55 PM
> > > Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
> > >
> > > 31/10/2017 16:59, Jonas Pfefferle:
> > > > Check and report errors on open/read in noiommu check.
> > > >
> > > > Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> > >
> > > I cannot decide to apply this patch as it does not explain what
> > > it is fixing, and as it is not reviewed.
> > >
> >
> > This patch adds error handling and logging to the noiommu check.
> > Also, on older kernels when the noiommu_enable file does not exist it
> > assumes noiommu is not enabled instead of returning -1.
> > Note that in rte_pci_get_iommu_class (drivers/bus/pci/linux/pci.c)
> > is the only usage of the function and it assumes return == 1:
> > noiommu is enabled any other return value noiommu disabled, i.e.
> > my code change does not change the behavior of this function.
> > We might want to check for errors in rte_pci_get_iommu_class
> > as well since assuming it is not enabled when we cannot open
> > and read it might lead to iova == VA being used even if noiommu is
> > enabled.
> >
> > All this comes back to what I proposed before: instead of
> > the noiommu and PPC64 check we should decide which iova mode
> > to use depending on the iommu types available.
> > The type should be already available at the point where we
> > decide on the iova type.
> > (iommu types supported is checked by vfio_get_container_fd)
>
> Is there something urgent for 17.11?
> Or can it be refined in 18.02?
Nothing urgent. We can refine this for 18.02.
>
> Anatoly, any thought?
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-11-07 9:50 ` Jonas Pfefferle1
@ 2018-01-11 23:45 ` Thomas Monjalon
2018-01-13 12:15 ` Burakov, Anatoly
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2018-01-11 23:45 UTC (permalink / raw)
To: Jonas Pfefferle1, anatoly.burakov; +Cc: dev
07/11/2017 10:50, Jonas Pfefferle1:
> Thomas Monjalon <thomas@monjalon.net> wrote on 11/07/2017 10:40:15 AM:
>
> > From: Thomas Monjalon <thomas@monjalon.net>
> > To: Jonas Pfefferle1 <JPF@zurich.ibm.com>, anatoly.burakov@intel.com
> > Cc: dev@dpdk.org
> > Date: 11/07/2017 10:40 AM
> > Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
> >
> > 07/11/2017 10:05, Jonas Pfefferle1:
> > > Thomas Monjalon <thomas@monjalon.net> wrote on 11/06/2017 09:25:15 PM:
> > >
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > To: Jonas Pfefferle <jpf@zurich.ibm.com>, anatoly.burakov@intel.com
> > > > Cc: dev@dpdk.org
> > > > Date: 11/06/2017 09:55 PM
> > > > Subject: Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
> > > >
> > > > 31/10/2017 16:59, Jonas Pfefferle:
> > > > > Check and report errors on open/read in noiommu check.
> > > > >
> > > > > Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> > > >
> > > > I cannot decide to apply this patch as it does not explain what
> > > > it is fixing, and as it is not reviewed.
> > > >
> > >
> > > This patch adds error handling and logging to the noiommu check.
> > > Also, on older kernels when the noiommu_enable file does not exist it
> > > assumes noiommu is not enabled instead of returning -1.
> > > Note that in rte_pci_get_iommu_class (drivers/bus/pci/linux/pci.c)
> > > is the only usage of the function and it assumes return == 1:
> > > noiommu is enabled any other return value noiommu disabled, i.e.
> > > my code change does not change the behavior of this function.
> > > We might want to check for errors in rte_pci_get_iommu_class
> > > as well since assuming it is not enabled when we cannot open
> > > and read it might lead to iova == VA being used even if noiommu is
> > > enabled.
> > >
> > > All this comes back to what I proposed before: instead of
> > > the noiommu and PPC64 check we should decide which iova mode
> > > to use depending on the iommu types available.
> > > The type should be already available at the point where we
> > > decide on the iova type.
> > > (iommu types supported is checked by vfio_get_container_fd)
> >
> > Is there something urgent for 17.11?
> > Or can it be refined in 18.02?
>
> Nothing urgent. We can refine this for 18.02.
>
> > Anatoly, any thought?
Anatoly, Jonas, how do you want to proceed with this patch?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-11 23:45 ` Thomas Monjalon
@ 2018-01-13 12:15 ` Burakov, Anatoly
2018-01-13 22:49 ` Thomas Monjalon
0 siblings, 1 reply; 20+ messages in thread
From: Burakov, Anatoly @ 2018-01-13 12:15 UTC (permalink / raw)
To: Thomas Monjalon, Jonas Pfefferle1; +Cc: dev
On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
> 07/11/2017 10:50, Jonas Pfefferle1:
>>> Is there something urgent for 17.11?
>>> Or can it be refined in 18.02?
>>
>> Nothing urgent. We can refine this for 18.02.
>>
>>> Anatoly, any thought?
>
> Anatoly, Jonas, how do you want to proceed with this patch?
>
I don't see anything to be refined here, it's a simple bug fix - code
assumes noiommu mode support is always available, when it might not be
the case on older kernels.
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-13 12:15 ` Burakov, Anatoly
@ 2018-01-13 22:49 ` Thomas Monjalon
2018-01-15 12:22 ` Jonas Pfefferle
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2018-01-13 22:49 UTC (permalink / raw)
To: Burakov, Anatoly, Jonas Pfefferle1; +Cc: dev
13/01/2018 13:15, Burakov, Anatoly:
> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
> > 07/11/2017 10:50, Jonas Pfefferle1:
> >>> Is there something urgent for 17.11?
> >>> Or can it be refined in 18.02?
> >>
> >> Nothing urgent. We can refine this for 18.02.
> >>
> >>> Anatoly, any thought?
> >
> > Anatoly, Jonas, how do you want to proceed with this patch?
> >
>
> I don't see anything to be refined here, it's a simple bug fix - code
> assumes noiommu mode support is always available, when it might not be
> the case on older kernels.
As a bug fix, the title must start with "fix" and a tag "Fixes:"
must be added to help with backport.
At the same time, the explanation of the bug must be added in
the commit log please.
Thanks
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-13 22:49 ` Thomas Monjalon
@ 2018-01-15 12:22 ` Jonas Pfefferle
2018-01-15 16:11 ` Thomas Monjalon
2018-01-16 10:07 ` Burakov, Anatoly
0 siblings, 2 replies; 20+ messages in thread
From: Jonas Pfefferle @ 2018-01-15 12:22 UTC (permalink / raw)
To: Thomas Monjalon, Burakov, Anatoly; +Cc: dev
On Sat, 13 Jan 2018 23:49:30 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:
> 13/01/2018 13:15, Burakov, Anatoly:
>> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>> > 07/11/2017 10:50, Jonas Pfefferle1:
>> >>> Is there something urgent for 17.11?
>> >>> Or can it be refined in 18.02?
>> >>
>> >> Nothing urgent. We can refine this for 18.02.
>> >>
>> >>> Anatoly, any thought?
>> >
>> > Anatoly, Jonas, how do you want to proceed with this patch?
>> >
>>
>> I don't see anything to be refined here, it's a simple bug fix -
>>code
>> assumes noiommu mode support is always available, when it might not
>>be
>> the case on older kernels.
>
> As a bug fix, the title must start with "fix" and a tag "Fixes:"
> must be added to help with backport.
> At the same time, the explanation of the bug must be added in
> the commit log please.
>
> Thanks
It's not really a bug fix since it does not change the semantic of the
function but just adds nicer error handling.
Regarding redefining the code: What I don't like is the special cases
we have to check for when using the sPAPR iommu because it does not
support VA mappings yet. I think we should decide which iova mode to
use based on the iommu types available, i.e. each iommu type should
report which iova type it supports. Thoughts?
Jonas (new email address)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-15 12:22 ` Jonas Pfefferle
@ 2018-01-15 16:11 ` Thomas Monjalon
2018-01-16 16:08 ` Jonas Pfefferle
2018-01-16 10:07 ` Burakov, Anatoly
1 sibling, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2018-01-15 16:11 UTC (permalink / raw)
To: Jonas Pfefferle; +Cc: Burakov, Anatoly, dev, maxime.coquelin
15/01/2018 13:22, Jonas Pfefferle:
>
> On Sat, 13 Jan 2018 23:49:30 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 13/01/2018 13:15, Burakov, Anatoly:
> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
> >> > 07/11/2017 10:50, Jonas Pfefferle1:
> >> >>> Is there something urgent for 17.11?
> >> >>> Or can it be refined in 18.02?
> >> >>
> >> >> Nothing urgent. We can refine this for 18.02.
> >> >>
> >> >>> Anatoly, any thought?
> >> >
> >> > Anatoly, Jonas, how do you want to proceed with this patch?
> >> >
> >>
> >> I don't see anything to be refined here, it's a simple bug fix -
> >>code
> >> assumes noiommu mode support is always available, when it might not
> >>be
> >> the case on older kernels.
> >
> > As a bug fix, the title must start with "fix" and a tag "Fixes:"
> > must be added to help with backport.
> > At the same time, the explanation of the bug must be added in
> > the commit log please.
> >
> > Thanks
>
> It's not really a bug fix since it does not change the semantic of the
> function but just adds nicer error handling.
> Regarding redefining the code: What I don't like is the special cases
> we have to check for when using the sPAPR iommu because it does not
> support VA mappings yet. I think we should decide which iova mode to
> use based on the iommu types available, i.e. each iommu type should
> report which iova type it supports. Thoughts?
Have you looked at what Maxime did?
https://dpdk.org/dev/patchwork/patch/33650/
How does it affect this patch?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-15 16:11 ` Thomas Monjalon
@ 2018-01-16 16:08 ` Jonas Pfefferle
2018-01-16 17:01 ` Maxime Coquelin
0 siblings, 1 reply; 20+ messages in thread
From: Jonas Pfefferle @ 2018-01-16 16:08 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Burakov, Anatoly, dev, maxime.coquelin
On Mon, 15 Jan 2018 17:11:58 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:
> 15/01/2018 13:22, Jonas Pfefferle:
>>
>> On Sat, 13 Jan 2018 23:49:30 +0100
>> Thomas Monjalon <thomas@monjalon.net> wrote:
>> > 13/01/2018 13:15, Burakov, Anatoly:
>> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>> >> > 07/11/2017 10:50, Jonas Pfefferle1:
>> >> >>> Is there something urgent for 17.11?
>> >> >>> Or can it be refined in 18.02?
>> >> >>
>> >> >> Nothing urgent. We can refine this for 18.02.
>> >> >>
>> >> >>> Anatoly, any thought?
>> >> >
>> >> > Anatoly, Jonas, how do you want to proceed with this patch?
>> >> >
>> >>
>> >> I don't see anything to be refined here, it's a simple bug fix -
>> >>code
>> >> assumes noiommu mode support is always available, when it might
>>not
>> >>be
>> >> the case on older kernels.
>> >
>> > As a bug fix, the title must start with "fix" and a tag "Fixes:"
>> > must be added to help with backport.
>> > At the same time, the explanation of the bug must be added in
>> > the commit log please.
>> >
>> > Thanks
>>
>> It's not really a bug fix since it does not change the semantic of
>>the
>> function but just adds nicer error handling.
>> Regarding redefining the code: What I don't like is the special
>>cases
>> we have to check for when using the sPAPR iommu because it does not
>> support VA mappings yet. I think we should decide which iova mode to
>> use based on the iommu types available, i.e. each iommu type should
>> report which iova type it supports. Thoughts?
>
> Have you looked at what Maxime did?
> https://dpdk.org/dev/patchwork/patch/33650/
>
> How does it affect this patch?
>
>
IMO it has the same problem. We shouldn't add more exception cases in
drivers/bus/pci/linux/pci.c but instead keep all the information about
what an IOMMU can do in lib/librte_eal/linuxapp/eal/eal_vfio.c
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-16 16:08 ` Jonas Pfefferle
@ 2018-01-16 17:01 ` Maxime Coquelin
2018-01-17 8:48 ` Jonas Pfefferle
0 siblings, 1 reply; 20+ messages in thread
From: Maxime Coquelin @ 2018-01-16 17:01 UTC (permalink / raw)
To: Jonas Pfefferle, Thomas Monjalon; +Cc: Burakov, Anatoly, dev
Hi Jonas,
On 01/16/2018 05:08 PM, Jonas Pfefferle wrote:
>
> On Mon, 15 Jan 2018 17:11:58 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
>> 15/01/2018 13:22, Jonas Pfefferle:
>>>
>>> On Sat, 13 Jan 2018 23:49:30 +0100
>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>> > 13/01/2018 13:15, Burakov, Anatoly:
>>> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>> >> > 07/11/2017 10:50, Jonas Pfefferle1:
>>> >> >>> Is there something urgent for 17.11?
>>> >> >>> Or can it be refined in 18.02?
>>> >> >>
>>> >> >> Nothing urgent. We can refine this for 18.02.
>>> >> >>
>>> >> >>> Anatoly, any thought?
>>> >> > >> > Anatoly, Jonas, how do you want to proceed with this patch?
>>> >> > >> >> I don't see anything to be refined here, it's a simple bug
>>> fix - >>code >> assumes noiommu mode support is always available,
>>> when it might not >>be >> the case on older kernels.
>>> > > As a bug fix, the title must start with "fix" and a tag "Fixes:"
>>> > must be added to help with backport.
>>> > At the same time, the explanation of the bug must be added in
>>> > the commit log please.
>>> > > Thanks
>>>
>>> It's not really a bug fix since it does not change the semantic of
>>> the function but just adds nicer error handling.
>>> Regarding redefining the code: What I don't like is the special cases
>>> we have to check for when using the sPAPR iommu because it does not
>>> support VA mappings yet. I think we should decide which iova mode to
>>> use based on the iommu types available, i.e. each iommu type should
>>> report which iova type it supports. Thoughts?
>>
>> Have you looked at what Maxime did?
>> https://dpdk.org/dev/patchwork/patch/33650/
>>
>> How does it affect this patch?
>>
>>
>
> IMO it has the same problem. We shouldn't add more exception cases in
> drivers/bus/pci/linux/pci.c but instead keep all the information about
> what an IOMMU can do in lib/librte_eal/linuxapp/eal/eal_vfio.c
I agree adding an exception in drivers/bus/pci/linux/pci.c isn't great,
but we need first to fix a regression introduced in v17.11 LTS, and
IMHO, we cannot do a big rework as the fix is to be backported.
Once fixed, I agree we should work on a refactoring. I don't know if
eal_vfio is the right place though, as in my case for example I cannot
get the information needed through vfio ioctl().
Out of curiosity, what prevents sPAPR to use VA mode for now?
Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-16 17:01 ` Maxime Coquelin
@ 2018-01-17 8:48 ` Jonas Pfefferle
2018-01-17 8:55 ` Maxime Coquelin
0 siblings, 1 reply; 20+ messages in thread
From: Jonas Pfefferle @ 2018-01-17 8:48 UTC (permalink / raw)
To: Maxime Coquelin, Thomas Monjalon; +Cc: Burakov, Anatoly, dev
On Tue, 16 Jan 2018 18:01:32 +0100
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> Hi Jonas,
>
> On 01/16/2018 05:08 PM, Jonas Pfefferle wrote:
>>
>> On Mon, 15 Jan 2018 17:11:58 +0100
>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>> 15/01/2018 13:22, Jonas Pfefferle:
>>>>
>>>> On Sat, 13 Jan 2018 23:49:30 +0100
>>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>> > 13/01/2018 13:15, Burakov, Anatoly:
>>>> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>>> >> > 07/11/2017 10:50, Jonas Pfefferle1:
>>>> >> >>> Is there something urgent for 17.11?
>>>> >> >>> Or can it be refined in 18.02?
>>>> >> >>
>>>> >> >> Nothing urgent. We can refine this for 18.02.
>>>> >> >>
>>>> >> >>> Anatoly, any thought?
>>>> >> > >> > Anatoly, Jonas, how do you want to proceed with this
>>>>patch?
>>>> >> > >> >> I don't see anything to be refined here, it's a simple
>>>>bug fix - >>code >> assumes noiommu mode support is always available,
>>>>when it might not >>be >> the case on older kernels.
>>>> > > As a bug fix, the title must start with "fix" and a tag "Fixes:"
>>>> > must be added to help with backport.
>>>> > At the same time, the explanation of the bug must be added in
>>>> > the commit log please.
>>>> > > Thanks
>>>>
>>>> It's not really a bug fix since it does not change the semantic of
>>>>the function but just adds nicer error handling.
>>>> Regarding redefining the code: What I don't like is the special
>>>>cases we have to check for when using the sPAPR iommu because it does
>>>>not support VA mappings yet. I think we should decide which iova mode
>>>>to use based on the iommu types available, i.e. each iommu type
>>>>should report which iova type it supports. Thoughts?
>>>
>>> Have you looked at what Maxime did?
>>> https://dpdk.org/dev/patchwork/patch/33650/
>>>
>>> How does it affect this patch?
>>>
>>>
>>
>> IMO it has the same problem. We shouldn't add more exception cases
>>in drivers/bus/pci/linux/pci.c but instead keep all the information
>>about what an IOMMU can do in lib/librte_eal/linuxapp/eal/eal_vfio.c
>
> I agree adding an exception in drivers/bus/pci/linux/pci.c isn't
>great,
> but we need first to fix a regression introduced in v17.11 LTS, and
> IMHO, we cannot do a big rework as the fix is to be backported.
>
> Once fixed, I agree we should work on a refactoring. I don't know if
> eal_vfio is the right place though, as in my case for example I
>cannot
> get the information needed through vfio ioctl().
>
> Out of curiosity, what prevents sPAPR to use VA mode for now?
>
> Maxime
Sounds good to me.
The current sPAPR Linux driver cannot use virtual addresses because
the DMA window size is restricted to RAM size and always starts at 0.
This is not a hardware restriction and we are working on allowing to
create arbitrary size windows.
Jonas
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-17 8:48 ` Jonas Pfefferle
@ 2018-01-17 8:55 ` Maxime Coquelin
2018-01-17 10:34 ` Jonas Pfefferle
0 siblings, 1 reply; 20+ messages in thread
From: Maxime Coquelin @ 2018-01-17 8:55 UTC (permalink / raw)
To: Jonas Pfefferle, Thomas Monjalon; +Cc: Burakov, Anatoly, dev
On 01/17/2018 09:48 AM, Jonas Pfefferle wrote:
>
> On Tue, 16 Jan 2018 18:01:32 +0100
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>> Hi Jonas,
>>
>> On 01/16/2018 05:08 PM, Jonas Pfefferle wrote:
>>>
>>> On Mon, 15 Jan 2018 17:11:58 +0100
>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>> 15/01/2018 13:22, Jonas Pfefferle:
>>>>>
>>>>> On Sat, 13 Jan 2018 23:49:30 +0100
>>>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>> > 13/01/2018 13:15, Burakov, Anatoly:
>>>>> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>>>> >> > 07/11/2017 10:50, Jonas Pfefferle1:
>>>>> >> >>> Is there something urgent for 17.11?
>>>>> >> >>> Or can it be refined in 18.02?
>>>>> >> >>
>>>>> >> >> Nothing urgent. We can refine this for 18.02.
>>>>> >> >>
>>>>> >> >>> Anatoly, any thought?
>>>>> >> > >> > Anatoly, Jonas, how do you want to proceed with this patch?
>>>>> >> > >> >> I don't see anything to be refined here, it's a simple
>>>>> bug fix - >>code >> assumes noiommu mode support is always
>>>>> available, when it might not >>be >> the case on older kernels.
>>>>> > > As a bug fix, the title must start with "fix" and a tag "Fixes:"
>>>>> > must be added to help with backport.
>>>>> > At the same time, the explanation of the bug must be added in
>>>>> > the commit log please.
>>>>> > > Thanks
>>>>>
>>>>> It's not really a bug fix since it does not change the semantic of
>>>>> the function but just adds nicer error handling.
>>>>> Regarding redefining the code: What I don't like is the special
>>>>> cases we have to check for when using the sPAPR iommu because it
>>>>> does not support VA mappings yet. I think we should decide which
>>>>> iova mode to use based on the iommu types available, i.e. each
>>>>> iommu type should report which iova type it supports. Thoughts?
>>>>
>>>> Have you looked at what Maxime did?
>>>> https://dpdk.org/dev/patchwork/patch/33650/
>>>>
>>>> How does it affect this patch?
>>>>
>>>>
>>>
>>> IMO it has the same problem. We shouldn't add more exception cases in
>>> drivers/bus/pci/linux/pci.c but instead keep all the information
>>> about what an IOMMU can do in lib/librte_eal/linuxapp/eal/eal_vfio.c
>>
>> I agree adding an exception in drivers/bus/pci/linux/pci.c isn't great,
>> but we need first to fix a regression introduced in v17.11 LTS, and
>> IMHO, we cannot do a big rework as the fix is to be backported.
>>
>> Once fixed, I agree we should work on a refactoring. I don't know if
>> eal_vfio is the right place though, as in my case for example I cannot
>> get the information needed through vfio ioctl().
>>
>> Out of curiosity, what prevents sPAPR to use VA mode for now?
>>
>> Maxime
>
> Sounds good to me.
>
> The current sPAPR Linux driver cannot use virtual addresses because the
> DMA window size is restricted to RAM size and always starts at 0. This
> is not a hardware restriction and we are working on allowing to create
> arbitrary size windows.
Thanks for the clarification, is the DMA window size information
accessible from user-space, so that we can enable VA mode or not
depending on its value?
Maxime
> Jonas
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-17 8:55 ` Maxime Coquelin
@ 2018-01-17 10:34 ` Jonas Pfefferle
0 siblings, 0 replies; 20+ messages in thread
From: Jonas Pfefferle @ 2018-01-17 10:34 UTC (permalink / raw)
To: Maxime Coquelin, Thomas Monjalon; +Cc: Burakov, Anatoly, dev
On Wed, 17 Jan 2018 09:55:37 +0100
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>
> On 01/17/2018 09:48 AM, Jonas Pfefferle wrote:
>>
>> On Tue, 16 Jan 2018 18:01:32 +0100
>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>> Hi Jonas,
>>>
>>> On 01/16/2018 05:08 PM, Jonas Pfefferle wrote:
>>>>
>>>> On Mon, 15 Jan 2018 17:11:58 +0100
>>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>> 15/01/2018 13:22, Jonas Pfefferle:
>>>>>>
>>>>>> On Sat, 13 Jan 2018 23:49:30 +0100
>>>>>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>>>>> > 13/01/2018 13:15, Burakov, Anatoly:
>>>>>> >> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>>>>> >> > 07/11/2017 10:50, Jonas Pfefferle1:
>>>>>> >> >>> Is there something urgent for 17.11?
>>>>>> >> >>> Or can it be refined in 18.02?
>>>>>> >> >>
>>>>>> >> >> Nothing urgent. We can refine this for 18.02.
>>>>>> >> >>
>>>>>> >> >>> Anatoly, any thought?
>>>>>> >> > >> > Anatoly, Jonas, how do you want to proceed with this
>>>>>>patch?
>>>>>> >> > >> >> I don't see anything to be refined here, it's a simple
>>>>>>bug fix - >>code >> assumes noiommu mode support is always available,
>>>>>>when it might not >>be >> the case on older kernels.
>>>>>> > > As a bug fix, the title must start with "fix" and a tag "Fixes:"
>>>>>> > must be added to help with backport.
>>>>>> > At the same time, the explanation of the bug must be added in
>>>>>> > the commit log please.
>>>>>> > > Thanks
>>>>>>
>>>>>> It's not really a bug fix since it does not change the semantic of
>>>>>>the function but just adds nicer error handling.
>>>>>> Regarding redefining the code: What I don't like is the special
>>>>>>cases we have to check for when using the sPAPR iommu because it does
>>>>>>not support VA mappings yet. I think we should decide which iova mode
>>>>>>to use based on the iommu types available, i.e. each iommu type
>>>>>>should report which iova type it supports. Thoughts?
>>>>>
>>>>> Have you looked at what Maxime did?
>>>>> https://dpdk.org/dev/patchwork/patch/33650/
>>>>>
>>>>> How does it affect this patch?
>>>>>
>>>>>
>>>>
>>>> IMO it has the same problem. We shouldn't add more exception cases
>>>>in drivers/bus/pci/linux/pci.c but instead keep all the information
>>>>about what an IOMMU can do in lib/librte_eal/linuxapp/eal/eal_vfio.c
>>>
>>> I agree adding an exception in drivers/bus/pci/linux/pci.c isn't
>>>great,
>>> but we need first to fix a regression introduced in v17.11 LTS, and
>>> IMHO, we cannot do a big rework as the fix is to be backported.
>>>
>>> Once fixed, I agree we should work on a refactoring. I don't know if
>>> eal_vfio is the right place though, as in my case for example I
>>>cannot
>>> get the information needed through vfio ioctl().
>>>
>>> Out of curiosity, what prevents sPAPR to use VA mode for now?
>>>
>>> Maxime
>>
>> Sounds good to me.
>>
>> The current sPAPR Linux driver cannot use virtual addresses because
>>the DMA window size is restricted to RAM size and always starts at 0.
>>This is not a hardware restriction and we are working on allowing to
>>create arbitrary size windows.
>
>
> Thanks for the clarification, is the DMA window size information
> accessible from user-space, so that we can enable VA mode or not
> depending on its value?
>
> Maxime
>> Jonas
>>
AFAIK it is not queryable. But we can try to create a window and if it
fails we try again with PA mode.
Jonas
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-15 12:22 ` Jonas Pfefferle
2018-01-15 16:11 ` Thomas Monjalon
@ 2018-01-16 10:07 ` Burakov, Anatoly
2018-01-16 16:09 ` Jonas Pfefferle
1 sibling, 1 reply; 20+ messages in thread
From: Burakov, Anatoly @ 2018-01-16 10:07 UTC (permalink / raw)
To: Jonas Pfefferle, Thomas Monjalon; +Cc: dev
On 15-Jan-18 12:22 PM, Jonas Pfefferle wrote:
>
> On Sat, 13 Jan 2018 23:49:30 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
>> 13/01/2018 13:15, Burakov, Anatoly:
>>> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>> > 07/11/2017 10:50, Jonas Pfefferle1:
>>> >>> Is there something urgent for 17.11?
>>> >>> Or can it be refined in 18.02?
>>> >>
>>> >> Nothing urgent. We can refine this for 18.02.
>>> >>
>>> >>> Anatoly, any thought?
>>> > > Anatoly, Jonas, how do you want to proceed with this patch?
>>> >
>>> I don't see anything to be refined here, it's a simple bug fix - code
>>> assumes noiommu mode support is always available, when it might not
>>> be the case on older kernels.
>>
>> As a bug fix, the title must start with "fix" and a tag "Fixes:"
>> must be added to help with backport.
>> At the same time, the explanation of the bug must be added in
>> the commit log please.
>>
>> Thanks
>
> It's not really a bug fix since it does not change the semantic of the
> function but just adds nicer error handling.
Well, as far as i can tell, it *does* change semantics - previously, if
noiommu mode file was not found, we returned -1, now we return 0.
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-16 10:07 ` Burakov, Anatoly
@ 2018-01-16 16:09 ` Jonas Pfefferle
0 siblings, 0 replies; 20+ messages in thread
From: Jonas Pfefferle @ 2018-01-16 16:09 UTC (permalink / raw)
To: Burakov, Anatoly, Thomas Monjalon; +Cc: dev
On Tue, 16 Jan 2018 10:07:51 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:
> On 15-Jan-18 12:22 PM, Jonas Pfefferle wrote:
>>
>> On Sat, 13 Jan 2018 23:49:30 +0100
>> Thomas Monjalon <thomas@monjalon.net> wrote:
>>> 13/01/2018 13:15, Burakov, Anatoly:
>>>> On 11-Jan-18 11:45 PM, Thomas Monjalon wrote:
>>>> > 07/11/2017 10:50, Jonas Pfefferle1:
>>>> >>> Is there something urgent for 17.11?
>>>> >>> Or can it be refined in 18.02?
>>>> >>
>>>> >> Nothing urgent. We can refine this for 18.02.
>>>> >>
>>>> >>> Anatoly, any thought?
>>>> > > Anatoly, Jonas, how do you want to proceed with this patch?
>>>> >
>>>> I don't see anything to be refined here, it's a simple bug fix -
>>>>code assumes noiommu mode support is always available, when it might
>>>>not be the case on older kernels.
>>>
>>> As a bug fix, the title must start with "fix" and a tag "Fixes:"
>>> must be added to help with backport.
>>> At the same time, the explanation of the bug must be added in
>>> the commit log please.
>>>
>>> Thanks
>>
>> It's not really a bug fix since it does not change the semantic of
>>the function but just adds nicer error handling.
>
> Well, as far as i can tell, it *does* change semantics - previously,
>if noiommu mode file was not found, we returned -1, now we return 0.
>
>
> --
> Thanks,
> Anatoly
True. I can change it to a bug fix.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-10-31 15:59 [dpdk-dev] [PATCH v2] vfio: noiommu check error handling Jonas Pfefferle
2017-11-06 20:25 ` Thomas Monjalon
@ 2018-01-13 12:05 ` Burakov, Anatoly
2018-01-19 17:37 ` Maxime Coquelin
2 siblings, 0 replies; 20+ messages in thread
From: Burakov, Anatoly @ 2018-01-13 12:05 UTC (permalink / raw)
To: Jonas Pfefferle, dev
On 31-Oct-17 3:59 PM, Jonas Pfefferle wrote:
> Check and report errors on open/read in noiommu check.
>
> Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> ---
LGTM
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2017-10-31 15:59 [dpdk-dev] [PATCH v2] vfio: noiommu check error handling Jonas Pfefferle
2017-11-06 20:25 ` Thomas Monjalon
2018-01-13 12:05 ` Burakov, Anatoly
@ 2018-01-19 17:37 ` Maxime Coquelin
2018-01-20 14:48 ` Thomas Monjalon
2 siblings, 1 reply; 20+ messages in thread
From: Maxime Coquelin @ 2018-01-19 17:37 UTC (permalink / raw)
To: Jonas Pfefferle, dev; +Cc: anatoly.burakov
On 10/31/2017 04:59 PM, Jonas Pfefferle wrote:
> Check and report errors on open/read in noiommu check.
>
> Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> ---
> lib/librte_eal/linuxapp/eal/eal_vfio.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
I agree with the fix, Kernels v4.4 and earlier does have vfio, but not
the noiommu mode, so the file does not exist.
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vfio: noiommu check error handling
2018-01-19 17:37 ` Maxime Coquelin
@ 2018-01-20 14:48 ` Thomas Monjalon
0 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-01-20 14:48 UTC (permalink / raw)
To: Maxime Coquelin, Jonas Pfefferle; +Cc: dev, anatoly.burakov
19/01/2018 18:37, Maxime Coquelin:
>
> On 10/31/2017 04:59 PM, Jonas Pfefferle wrote:
> > Check and report errors on open/read in noiommu check.
> >
> > Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> > ---
> > lib/librte_eal/linuxapp/eal/eal_vfio.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
>
> I agree with the fix, Kernels v4.4 and earlier does have vfio, but not
> the noiommu mode, so the file does not exist.
>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Applied, and added this explanation, thanks
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-01-20 14:49 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 15:59 [dpdk-dev] [PATCH v2] vfio: noiommu check error handling Jonas Pfefferle
2017-11-06 20:25 ` Thomas Monjalon
2017-11-07 9:05 ` Jonas Pfefferle1
2017-11-07 9:40 ` Thomas Monjalon
2017-11-07 9:50 ` Jonas Pfefferle1
2018-01-11 23:45 ` Thomas Monjalon
2018-01-13 12:15 ` Burakov, Anatoly
2018-01-13 22:49 ` Thomas Monjalon
2018-01-15 12:22 ` Jonas Pfefferle
2018-01-15 16:11 ` Thomas Monjalon
2018-01-16 16:08 ` Jonas Pfefferle
2018-01-16 17:01 ` Maxime Coquelin
2018-01-17 8:48 ` Jonas Pfefferle
2018-01-17 8:55 ` Maxime Coquelin
2018-01-17 10:34 ` Jonas Pfefferle
2018-01-16 10:07 ` Burakov, Anatoly
2018-01-16 16:09 ` Jonas Pfefferle
2018-01-13 12:05 ` Burakov, Anatoly
2018-01-19 17:37 ` Maxime Coquelin
2018-01-20 14:48 ` Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).