* [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one
@ 2016-08-10 3:18 Cao jin
2016-08-15 4:14 ` Cao jin
2016-08-18 10:54 ` Marcel Apfelbaum
0 siblings, 2 replies; 4+ messages in thread
From: Cao jin @ 2016-08-10 3:18 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, alex.williamson
The parameter table_offset & pba_offset is kind of confusing, they shouldn't
include bir field.
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
According to the passed arguments, I guess all the callers of msix_init()
has the same feeling with me, but I am not quite sure about this, so, RFC.
hw/pci/msix.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..3a16d83 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -264,8 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
if ((table_bar_nr == pba_bar_nr &&
ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
table_offset + table_size > memory_region_size(table_bar) ||
- pba_offset + pba_size > memory_region_size(pba_bar) ||
- (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
+ pba_offset + pba_size > memory_region_size(pba_bar)) {
return -EINVAL;
}
@@ -282,8 +281,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
dev->msix_entries_nr = nentries;
dev->msix_function_masked = true;
- pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar_nr);
- pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar_nr);
+ pci_set_long(config + PCI_MSIX_TABLE, (table_offset << 3) | table_bar_nr);
+ pci_set_long(config + PCI_MSIX_PBA, (pba_offset << 3) | pba_bar_nr);
/* Make flags bit writable. */
dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
--
2.1.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one
2016-08-10 3:18 [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one Cao jin
@ 2016-08-15 4:14 ` Cao jin
2016-08-18 10:54 ` Marcel Apfelbaum
1 sibling, 0 replies; 4+ messages in thread
From: Cao jin @ 2016-08-15 4:14 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, alex.williamson, mst
Sorry for the noise.
On 08/10/2016 11:18 AM, Cao jin wrote:
> The parameter table_offset & pba_offset is kind of confusing, they shouldn't
> include bir field.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> According to the passed arguments, I guess all the callers of msix_init()
> has the same feeling with me, but I am not quite sure about this, so, RFC.
>
> hw/pci/msix.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..3a16d83 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -264,8 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> if ((table_bar_nr == pba_bar_nr &&
> ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> table_offset + table_size > memory_region_size(table_bar) ||
> - pba_offset + pba_size > memory_region_size(pba_bar) ||
> - (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> + pba_offset + pba_size > memory_region_size(pba_bar)) {
> return -EINVAL;
> }
>
> @@ -282,8 +281,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> dev->msix_entries_nr = nentries;
> dev->msix_function_masked = true;
>
> - pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar_nr);
> - pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar_nr);
> + pci_set_long(config + PCI_MSIX_TABLE, (table_offset << 3) | table_bar_nr);
> + pci_set_long(config + PCI_MSIX_PBA, (pba_offset << 3) | pba_bar_nr);
>
> /* Make flags bit writable. */
> dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one
2016-08-10 3:18 [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one Cao jin
2016-08-15 4:14 ` Cao jin
@ 2016-08-18 10:54 ` Marcel Apfelbaum
2016-08-19 2:35 ` Cao jin
1 sibling, 1 reply; 4+ messages in thread
From: Marcel Apfelbaum @ 2016-08-18 10:54 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: mst, alex.williamson
On 08/10/2016 06:18 AM, Cao jin wrote:
> The parameter table_offset & pba_offset is kind of confusing, they shouldn't
> include bir field.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
Hi,
> According to the passed arguments, I guess all the callers of msix_init()
> has the same feeling with me, but I am not quite sure about this, so, RFC.
>
> hw/pci/msix.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..3a16d83 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -264,8 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> if ((table_bar_nr == pba_bar_nr &&
> ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> table_offset + table_size > memory_region_size(table_bar) ||
> - pba_offset + pba_size > memory_region_size(pba_bar) ||
> - (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> + pba_offset + pba_size > memory_region_size(pba_bar)) {
> return -EINVAL;
I think we should keep the '(table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK)'
test since it is required by spec, please see: PCI Spec "6.8.2.4. Table Offset/Table BIR for MSI-X"
Table offset: ...The lower 3 Table BIR bits are masked off (set to zero) by software
to form a 32-bit QWORD -aligned offset.
This function gets the offset parameters as the whole 32-BIT QWORD and checks it does not collide
with the BIR offset.
> }
>
> @@ -282,8 +281,8 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> dev->msix_entries_nr = nentries;
> dev->msix_function_masked = true;
>
> - pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar_nr);
> - pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar_nr);
> + pci_set_long(config + PCI_MSIX_TABLE, (table_offset << 3) | table_bar_nr);
> + pci_set_long(config + PCI_MSIX_PBA, (pba_offset << 3) | pba_bar_nr);
>
Here is a similar issue. Your interpretation suggests we need to shift left the offset
to make room for BIR, but I think current implementation looks at it differently already
receiving the offset as a 32-bit QWORD and simply does not "look" to the lower bits
implying them 0.
Thanks,
Marcel
> /* Make flags bit writable. */
> dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one
2016-08-18 10:54 ` Marcel Apfelbaum
@ 2016-08-19 2:35 ` Cao jin
0 siblings, 0 replies; 4+ messages in thread
From: Cao jin @ 2016-08-19 2:35 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: mst, alex.williamson
On 08/18/2016 06:54 PM, Marcel Apfelbaum wrote:
> On 08/10/2016 06:18 AM, Cao jin wrote:
>> The parameter table_offset & pba_offset is kind of confusing, they
>> shouldn't
>> include bir field.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>
>
> Hi,
>
>> According to the passed arguments, I guess all the callers of msix_init()
>> has the same feeling with me, but I am not quite sure about this, so,
>> RFC.
>>
>> hw/pci/msix.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index 0ec1cb1..3a16d83 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -264,8 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned
>> short nentries,
>> if ((table_bar_nr == pba_bar_nr &&
>> ranges_overlap(table_offset, table_size, pba_offset,
>> pba_size)) ||
>> table_offset + table_size > memory_region_size(table_bar) ||
>> - pba_offset + pba_size > memory_region_size(pba_bar) ||
>> - (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
>> + pba_offset + pba_size > memory_region_size(pba_bar)) {
>> return -EINVAL;
>
> I think we should keep the '(table_offset | pba_offset) &
> PCI_MSIX_FLAGS_BIRMASK)'
> test since it is required by spec, please see: PCI Spec "6.8.2.4. Table
> Offset/Table BIR for MSI-X"
>
> Table offset: ...The lower 3 Table BIR bits are masked off (set to
> zero) by software
> to form a 32-bit QWORD -aligned offset.
>
> This function gets the offset parameters as the whole 32-BIT QWORD and
> checks it does not collide
> with the BIR offset.
>
Hi Marcel,
Thanks very much for pointing the accurate reference out.
I also checked how kernel code handle this field, it is just like
you said.
Sorry for this noise.
Cao jin
>
>> }
>>
>> @@ -282,8 +281,8 @@ int msix_init(struct PCIDevice *dev, unsigned
>> short nentries,
>> dev->msix_entries_nr = nentries;
>> dev->msix_function_masked = true;
>>
>> - pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar_nr);
>> - pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar_nr);
>> + pci_set_long(config + PCI_MSIX_TABLE, (table_offset << 3) |
>> table_bar_nr);
>> + pci_set_long(config + PCI_MSIX_PBA, (pba_offset << 3) | pba_bar_nr);
>>
>
> Here is a similar issue. Your interpretation suggests we need to shift
> left the offset
> to make room for BIR, but I think current implementation looks at it
> differently already
> receiving the offset as a 32-bit QWORD and simply does not "look" to the
> lower bits
> implying them 0.
>
> Thanks,
> Marcel
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-19 2:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-10 3:18 [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one Cao jin
2016-08-15 4:14 ` Cao jin
2016-08-18 10:54 ` Marcel Apfelbaum
2016-08-19 2:35 ` Cao jin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.