[v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver
Checks
Commit Message
With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the start and end
of the MSI table to the PAGE_SIZE, the end offset of the MSI
table is out of the PCI BAR boundary.
This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size, and skip the mapping
if it is out of Bar scope.
The patch fixes the debug log as below:
EAL: Skipping BAR0
Signed-off-by: tone.zhang <tone.zhang@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Steve Capper <Steve.Capper@arm.com>
Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
---
drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
Comments
On 09-Nov-18 5:57 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very
> close to the end of the BAR s.t. when we align the start and end
> of the MSI table to the PAGE_SIZE, the end offset of the MSI
> table is out of the PCI BAR boundary.
>
> This patch addresses the issue by comparing both the start and the
> end offset of the MSI table with the BAR size, and skip the mapping
> if it is out of Bar scope.
>
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
>
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
In the future, please don't include my Reviewed tag unless i actually
sent one :)
> ---
> drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 305cc06..9a0affe 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
> struct pci_msix_table *msix_table = &vfio_res->msix_table;
> struct pci_map *bar = &vfio_res->maps[bar_index];
>
> - if (bar->size == 0)
> + if (bar->size == 0) {
> /* Skip this BAR */
> + RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> return 0;
> + }
>
> if (msix_table->bar_index == bar_index) {
> /*
> @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
> */
> uint32_t table_start = msix_table->offset;
> uint32_t table_end = table_start + msix_table->size;
> - table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> - table_start &= PAGE_MASK;
> + table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> + table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> + /* after rounding to PAGE_SIZE, it is over the bar->size,
> + * fall back to the MSI-X table offset in the bar and
> + * align with PAGE_SIZE.
> + */
Minor nitpick - wording of comment could be better, for example:
if page-aligned start of MSI-X table is beyond BAR size, shrink the
mapping size to MSI-X table start address.
Also, probably needs newline before comment.
> + if (table_start >= bar->size) {
> + table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> + PAGE_SIZE);
> + /* after aligning with PAGE_SIZE, if it is less than
> + * the MSI-X table offset, continue falling back to
> + * the actual MSI-X table offset in the bar.
> + */
Same here, wording could probably be improved. Suggested rewording:
If MSI-X table address, floor-aligned by page size, is lower than actual
MSI-X table offset, fall back to using MSI-X table offset as table start.
Now that i think of it, this could really be expressed like this:
uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
table_start = RTE_MAX(aligned, msix_table_offset);
I believe this would be much clearer.
> + if (table_start < msix_table->offset)
> + table_start = msix_table->offset;
> + }
>
> if (table_start == 0 && table_end >= bar->size) {
> /* Cannot map this BAR */
> @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>
> memreg[0].offset = bar->offset;
> memreg[0].size = table_start;
> - memreg[1].offset = bar->offset + table_end;
> - memreg[1].size = bar->size - table_end;
> + if (bar->size < table_end) {
> + /*
> + * after rounding to PAGE_SIZE we don't have any space
> + * left after the MSI table, so don't try and map it.
> + */
Suggested rewording:
If MSI-X table end is beyond BAR end, don't attempt to perform second
mapping.
> + memreg[1].offset = 0;
> + memreg[1].size = 0;
> + }
> + else {
> + memreg[1].offset = bar->offset + table_end;
> + memreg[1].size = bar->size - table_end;
> + }
>
> RTE_LOG(DEBUG, EAL,
> "Trying to map BAR%d that contains the MSI-X "
>
However, the patch can go in as is if needed, so
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Hi Anatoly,
Sorry for the late response.
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday, November 9, 2018 8:15 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
> driver
>
> On 09-Nov-18 5:57 AM, tone.zhang wrote:
> > With a larger PAGE_SIZE it is possible for the MSI table to very close
> > to the end of the BAR s.t. when we align the start and end of the MSI
> > table to the PAGE_SIZE, the end offset of the MSI table is out of the
> > PCI BAR boundary.
> >
> > This patch addresses the issue by comparing both the start and the end
> > offset of the MSI table with the BAR size, and skip the mapping if it
> > is out of Bar scope.
> >
> > The patch fixes the debug log as below:
> > EAL: Skipping BAR0
> >
> > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
>
> In the future, please don't include my Reviewed tag unless i actually sent one :)
Thanks a lot! Will keep in mind. 😊
>
> > ---
> > drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++---
> --
> > 1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index 305cc06..9a0affe 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> > struct pci_msix_table *msix_table = &vfio_res->msix_table;
> > struct pci_map *bar = &vfio_res->maps[bar_index];
> >
> > - if (bar->size == 0)
> > + if (bar->size == 0) {
> > /* Skip this BAR */
> > + RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> > return 0;
> > + }
> >
> > if (msix_table->bar_index == bar_index) {
> > /*
> > @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> > */
> > uint32_t table_start = msix_table->offset;
> > uint32_t table_end = table_start + msix_table->size;
> > - table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> > - table_start &= PAGE_MASK;
> > + table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> > + table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> > + /* after rounding to PAGE_SIZE, it is over the bar->size,
> > + * fall back to the MSI-X table offset in the bar and
> > + * align with PAGE_SIZE.
> > + */
>
> Minor nitpick - wording of comment could be better, for example:
>
> if page-aligned start of MSI-X table is beyond BAR size, shrink the mapping size
> to MSI-X table start address.
>
> Also, probably needs newline before comment.
>
Will update the code in next version. Thanks!
> > + if (table_start >= bar->size) {
> > + table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> > + PAGE_SIZE);
> > + /* after aligning with PAGE_SIZE, if it is less than
> > + * the MSI-X table offset, continue falling back to
> > + * the actual MSI-X table offset in the bar.
> > + */
>
> Same here, wording could probably be improved. Suggested rewording:
>
> If MSI-X table address, floor-aligned by page size, is lower than actual MSI-X
> table offset, fall back to using MSI-X table offset as table start.
>
> Now that i think of it, this could really be expressed like this:
>
> uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
> table_start = RTE_MAX(aligned, msix_table_offset);
>
> I believe this would be much clearer.
>
Will update the patch.
> > + if (table_start < msix_table->offset)
> > + table_start = msix_table->offset;
> > + }
> >
> > if (table_start == 0 && table_end >= bar->size) {
> > /* Cannot map this BAR */
> > @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> >
> > memreg[0].offset = bar->offset;
> > memreg[0].size = table_start;
> > - memreg[1].offset = bar->offset + table_end;
> > - memreg[1].size = bar->size - table_end;
> > + if (bar->size < table_end) {
> > + /*
> > + * after rounding to PAGE_SIZE we don't have any space
> > + * left after the MSI table, so don't try and map it.
> > + */
>
> Suggested rewording:
>
> If MSI-X table end is beyond BAR end, don't attempt to perform second mapping.
>
Thanks a lot. Will update.
> > + memreg[1].offset = 0;
> > + memreg[1].size = 0;
> > + }
> > + else {
> > + memreg[1].offset = bar->offset + table_end;
> > + memreg[1].size = bar->size - table_end;
> > + }
> >
> > RTE_LOG(DEBUG, EAL,
> > "Trying to map BAR%d that contains the MSI-X "
> >
>
> However, the patch can go in as is if needed, so
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
Thanks! 😉
> --
> Thanks,
> Anatoly
Br,
Tone
Hi Anatoly,
I have some comments.
> -----Original Message-----
> From: Tone Zhang (Arm Technology China)
> Sent: Thursday, November 15, 2018 8:49 AM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
> driver
>
> Hi Anatoly,
>
> Sorry for the late response.
>
> > -----Original Message-----
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > Sent: Friday, November 9, 2018 8:15 PM
> > To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> > dev@dpdk.org
> > Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> > <Steve.Capper@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with
> > vfio-pci driver
> >
> > On 09-Nov-18 5:57 AM, tone.zhang wrote:
> > > With a larger PAGE_SIZE it is possible for the MSI table to very
> > > close to the end of the BAR s.t. when we align the start and end of
> > > the MSI table to the PAGE_SIZE, the end offset of the MSI table is
> > > out of the PCI BAR boundary.
> > >
> > > This patch addresses the issue by comparing both the start and the
> > > end offset of the MSI table with the BAR size, and skip the mapping
> > > if it is out of Bar scope.
> > >
> > > The patch fixes the debug log as below:
> > > EAL: Skipping BAR0
> > >
> > > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > > Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
> >
> > In the future, please don't include my Reviewed tag unless i actually
> > sent one :)
>
> Thanks a lot! Will keep in mind. 😊
>
> >
> > > ---
> > > drivers/bus/pci/linux/pci_vfio.c | 36
> > > +++++++++++++++++++++++++++++++---
> > --
> > > 1 file changed, 31 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > b/drivers/bus/pci/linux/pci_vfio.c
> > > index 305cc06..9a0affe 100644
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> > > struct pci_msix_table *msix_table = &vfio_res->msix_table;
> > > struct pci_map *bar = &vfio_res->maps[bar_index];
> > >
> > > - if (bar->size == 0)
> > > + if (bar->size == 0) {
> > > /* Skip this BAR */
> > > + RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> > > return 0;
> > > + }
> > >
> > > if (msix_table->bar_index == bar_index) {
> > > /*
> > > @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> > > */
> > > uint32_t table_start = msix_table->offset;
> > > uint32_t table_end = table_start + msix_table->size;
> > > - table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> > > - table_start &= PAGE_MASK;
> > > + table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> > > + table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> > > + /* after rounding to PAGE_SIZE, it is over the bar->size,
> > > + * fall back to the MSI-X table offset in the bar and
> > > + * align with PAGE_SIZE.
> > > + */
> >
> > Minor nitpick - wording of comment could be better, for example:
> >
> > if page-aligned start of MSI-X table is beyond BAR size, shrink the
> > mapping size to MSI-X table start address.
> >
> > Also, probably needs newline before comment.
> >
>
> Will update the code in next version. Thanks!
>
> > > + if (table_start >= bar->size) {
> > > + table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> > > + PAGE_SIZE);
> > > + /* after aligning with PAGE_SIZE, if it is less than
> > > + * the MSI-X table offset, continue falling back to
> > > + * the actual MSI-X table offset in the bar.
> > > + */
> >
> > Same here, wording could probably be improved. Suggested rewording:
> >
> > If MSI-X table address, floor-aligned by page size, is lower than
> > actual MSI-X table offset, fall back to using MSI-X table offset as table start.
> >
> > Now that i think of it, this could really be expressed like this:
> >
> > uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
> > table_start = RTE_MAX(aligned, msix_table_offset);
> >
> > I believe this would be much clearer.
> >
>
> Will update the patch.
>
When enter the judgement, it implies the "msix_table->offset" is NOT page size aligned, I think we can replace the code in the judgement with one line: table_start = msix_table->offset;
It looks more simple. What's your opinion? Thanks!
> > > + if (table_start < msix_table->offset)
> > > + table_start = msix_table->offset;
> > > + }
> > >
> > > if (table_start == 0 && table_end >= bar->size) {
> > > /* Cannot map this BAR */
> > > @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > > mapped_pci_resource *vfio_res,
> > >
> > > memreg[0].offset = bar->offset;
> > > memreg[0].size = table_start;
> > > - memreg[1].offset = bar->offset + table_end;
> > > - memreg[1].size = bar->size - table_end;
> > > + if (bar->size < table_end) {
> > > + /*
> > > + * after rounding to PAGE_SIZE we don't have any space
> > > + * left after the MSI table, so don't try and map it.
> > > + */
> >
> > Suggested rewording:
> >
> > If MSI-X table end is beyond BAR end, don't attempt to perform second
> mapping.
> >
>
> Thanks a lot. Will update.
>
> > > + memreg[1].offset = 0;
> > > + memreg[1].size = 0;
> > > + }
> > > + else {
> > > + memreg[1].offset = bar->offset + table_end;
> > > + memreg[1].size = bar->size - table_end;
> > > + }
> > >
> > > RTE_LOG(DEBUG, EAL,
> > > "Trying to map BAR%d that contains the MSI-X "
> > >
> >
> > However, the patch can go in as is if needed, so
> >
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
>
> Thanks! 😉
>
> > --
> > Thanks,
> > Anatoly
>
> Br,
> Tone
Br,
Tone
On 16-Nov-18 2:34 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Anatoly,
>
> I have some comments.
>
>> -----Original Message-----
>> From: Tone Zhang (Arm Technology China)
>> Sent: Thursday, November 15, 2018 8:49 AM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
>> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
>> <Steve.Capper@arm.com>; nd <nd@arm.com>
>> Subject: RE: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
>> driver
>>
>> Hi Anatoly,
>>
>> Sorry for the late response.
>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Friday, November 9, 2018 8:15 PM
>>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
>>> dev@dpdk.org
>>> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
>>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
>>> <Steve.Capper@arm.com>; nd <nd@arm.com>
>>> Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with
>>> vfio-pci driver
>>>
>>> On 09-Nov-18 5:57 AM, tone.zhang wrote:
>>>> With a larger PAGE_SIZE it is possible for the MSI table to very
>>>> close to the end of the BAR s.t. when we align the start and end of
>>>> the MSI table to the PAGE_SIZE, the end offset of the MSI table is
>>>> out of the PCI BAR boundary.
>>>>
>>>> This patch addresses the issue by comparing both the start and the
>>>> end offset of the MSI table with the BAR size, and skip the mapping
>>>> if it is out of Bar scope.
>>>>
>>>> The patch fixes the debug log as below:
>>>> EAL: Skipping BAR0
>>>>
>>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>>> Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
>>>
>>> In the future, please don't include my Reviewed tag unless i actually
>>> sent one :)
>>
>> Thanks a lot! Will keep in mind. 😊
>>
>>>
>>>> ---
>>>> drivers/bus/pci/linux/pci_vfio.c | 36
>>>> +++++++++++++++++++++++++++++++---
>>> --
>>>> 1 file changed, 31 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>>> b/drivers/bus/pci/linux/pci_vfio.c
>>>> index 305cc06..9a0affe 100644
>>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>> mapped_pci_resource *vfio_res,
>>>> struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>> struct pci_map *bar = &vfio_res->maps[bar_index];
>>>>
>>>> - if (bar->size == 0)
>>>> + if (bar->size == 0) {
>>>> /* Skip this BAR */
>>>> + RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
>>>> return 0;
>>>> + }
>>>>
>>>> if (msix_table->bar_index == bar_index) {
>>>> /*
>>>> @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>> mapped_pci_resource *vfio_res,
>>>> */
>>>> uint32_t table_start = msix_table->offset;
>>>> uint32_t table_end = table_start + msix_table->size;
>>>> - table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>>> - table_start &= PAGE_MASK;
>>>> + table_end = RTE_ALIGN(table_end, PAGE_SIZE);
>>>> + table_start = RTE_ALIGN(table_start, PAGE_SIZE);
>>>> + /* after rounding to PAGE_SIZE, it is over the bar->size,
>>>> + * fall back to the MSI-X table offset in the bar and
>>>> + * align with PAGE_SIZE.
>>>> + */
>>>
>>> Minor nitpick - wording of comment could be better, for example:
>>>
>>> if page-aligned start of MSI-X table is beyond BAR size, shrink the
>>> mapping size to MSI-X table start address.
>>>
>>> Also, probably needs newline before comment.
>>>
>>
>> Will update the code in next version. Thanks!
>>
>>>> + if (table_start >= bar->size) {
>>>> + table_start = RTE_ALIGN_FLOOR(msix_table->offset,
>>>> + PAGE_SIZE);
>>>> + /* after aligning with PAGE_SIZE, if it is less than
>>>> + * the MSI-X table offset, continue falling back to
>>>> + * the actual MSI-X table offset in the bar.
>>>> + */
>>>
>>> Same here, wording could probably be improved. Suggested rewording:
>>>
>>> If MSI-X table address, floor-aligned by page size, is lower than
>>> actual MSI-X table offset, fall back to using MSI-X table offset as table start.
>>>
>>> Now that i think of it, this could really be expressed like this:
>>>
>>> uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
>>> table_start = RTE_MAX(aligned, msix_table_offset);
>>>
>>> I believe this would be much clearer.
>>>
>>
>> Will update the patch.
>>
>
> When enter the judgement, it implies the "msix_table->offset" is NOT page size aligned, I think we can replace the code in the judgement with one line: table_start = msix_table->offset;
> It looks more simple. What's your opinion? Thanks!
Agree, what was i thinking :D
>
>>>> + if (table_start < msix_table->offset)
>>>> + table_start = msix_table->offset;
>>>> + }
>>>>
>>>> if (table_start == 0 && table_end >= bar->size) {
>>>> /* Cannot map this BAR */
>>>> @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>>> mapped_pci_resource *vfio_res,
>>>>
>>>> memreg[0].offset = bar->offset;
>>>> memreg[0].size = table_start;
>>>> - memreg[1].offset = bar->offset + table_end;
>>>> - memreg[1].size = bar->size - table_end;
>>>> + if (bar->size < table_end) {
>>>> + /*
>>>> + * after rounding to PAGE_SIZE we don't have any space
>>>> + * left after the MSI table, so don't try and map it.
>>>> + */
>>>
>>> Suggested rewording:
>>>
>>> If MSI-X table end is beyond BAR end, don't attempt to perform second
>> mapping.
>>>
>>
>> Thanks a lot. Will update.
>>
>>>> + memreg[1].offset = 0;
>>>> + memreg[1].size = 0;
>>>> + }
>>>> + else {
>>>> + memreg[1].offset = bar->offset + table_end;
>>>> + memreg[1].size = bar->size - table_end;
>>>> + }
>>>>
>>>> RTE_LOG(DEBUG, EAL,
>>>> "Trying to map BAR%d that contains the MSI-X "
>>>>
>>>
>>> However, the patch can go in as is if needed, so
>>>
>>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>
>> Thanks! 😉
>>
>>> --
>>> Thanks,
>>> Anatoly
>>
>> Br,
>> Tone
>
> Br,
> Tone
>
@@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
struct pci_msix_table *msix_table = &vfio_res->msix_table;
struct pci_map *bar = &vfio_res->maps[bar_index];
- if (bar->size == 0)
+ if (bar->size == 0) {
/* Skip this BAR */
+ RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
return 0;
+ }
if (msix_table->bar_index == bar_index) {
/*
@@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
*/
uint32_t table_start = msix_table->offset;
uint32_t table_end = table_start + msix_table->size;
- table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
- table_start &= PAGE_MASK;
+ table_end = RTE_ALIGN(table_end, PAGE_SIZE);
+ table_start = RTE_ALIGN(table_start, PAGE_SIZE);
+ /* after rounding to PAGE_SIZE, it is over the bar->size,
+ * fall back to the MSI-X table offset in the bar and
+ * align with PAGE_SIZE.
+ */
+ if (table_start >= bar->size) {
+ table_start = RTE_ALIGN_FLOOR(msix_table->offset,
+ PAGE_SIZE);
+ /* after aligning with PAGE_SIZE, if it is less than
+ * the MSI-X table offset, continue falling back to
+ * the actual MSI-X table offset in the bar.
+ */
+ if (table_start < msix_table->offset)
+ table_start = msix_table->offset;
+ }
if (table_start == 0 && table_end >= bar->size) {
/* Cannot map this BAR */
@@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
memreg[0].offset = bar->offset;
memreg[0].size = table_start;
- memreg[1].offset = bar->offset + table_end;
- memreg[1].size = bar->size - table_end;
+ if (bar->size < table_end) {
+ /*
+ * after rounding to PAGE_SIZE we don't have any space
+ * left after the MSI table, so don't try and map it.
+ */
+ memreg[1].offset = 0;
+ memreg[1].size = 0;
+ }
+ else {
+ memreg[1].offset = bar->offset + table_end;
+ memreg[1].size = bar->size - table_end;
+ }
RTE_LOG(DEBUG, EAL,
"Trying to map BAR%d that contains the MSI-X "