[dpdk-dev,v2] vfio: noiommu check error handling
Checks
Commit Message
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(-)
Comments
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.
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)
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?
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?
>
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?
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>
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.
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
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)
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?
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.
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
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.
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
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
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
>
>
>
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
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
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
@@ -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