| Message ID | 20101022203151.10161.17562.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote: > To enable common msix support to be used with pass through devices, > don't attempt to change the BAR if the device already has an > MSI-X capability. This also means we want to pay closer attention > to the size when we map the msix table page, as it isn't necessarily > covering the entire end of the BAR. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- > 1 files changed, 38 insertions(+), 29 deletions(-) > > diff --git a/hw/msix.c b/hw/msix.c > index 43efbd2..4122395 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > { > int config_offset; > uint8_t *config; > - uint32_t new_size; > > - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) > - return -EINVAL; > - if (bar_size > 0x80000000) > - return -ENOSPC; > - > - /* Add space for MSI-X structures */ > - if (!bar_size) { > - new_size = MSIX_PAGE_SIZE; > - } else if (bar_size < MSIX_PAGE_SIZE) { > - bar_size = MSIX_PAGE_SIZE; > - new_size = MSIX_PAGE_SIZE * 2; > - } else { > - new_size = bar_size * 2; > - } > - > - pdev->msix_bar_size = new_size; > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > - if (config_offset < 0) > - return config_offset; > - config = pdev->config + config_offset; > - > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > - /* Table on top of BAR */ > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > - /* Pending bits on top of that */ > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > - bar_nr); > + pdev->msix_bar_size = bar_size; > + > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); > + > + if (!config_offset) { > + uint32_t new_size; > + > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) > + return -EINVAL; > + if (bar_size > 0x80000000) > + return -ENOSPC; > + > + /* Add space for MSI-X structures */ > + if (!bar_size) { > + new_size = MSIX_PAGE_SIZE; > + } else if (bar_size < MSIX_PAGE_SIZE) { > + bar_size = MSIX_PAGE_SIZE; > + new_size = MSIX_PAGE_SIZE * 2; > + } else { > + new_size = bar_size * 2; > + } > + > + pdev->msix_bar_size = new_size; > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, > + MSIX_CAP_LENGTH); > + if (config_offset < 0) > + return config_offset; > + config = pdev->config + config_offset; > + > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > + /* Table on top of BAR */ > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > + /* Pending bits on top of that */ > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > + bar_nr); > + } > pdev->msix_cap = config_offset; > /* Make flags bit writeable. */ > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, > return; > if (size <= offset) > return; > - cpu_register_physical_memory(addr + offset, size - offset, > + cpu_register_physical_memory(addr + offset, > + MIN(size - offset, MSIX_PAGE_SIZE), This is wrong I think, the table might not fit in a single page. You would need to read table size out of from device config. > d->msix_mmio_index); > } >
On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote: > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote: > > To enable common msix support to be used with pass through devices, > > don't attempt to change the BAR if the device already has an > > MSI-X capability. This also means we want to pay closer attention > > to the size when we map the msix table page, as it isn't necessarily > > covering the entire end of the BAR. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- > > 1 files changed, 38 insertions(+), 29 deletions(-) > > > > diff --git a/hw/msix.c b/hw/msix.c > > index 43efbd2..4122395 100644 > > --- a/hw/msix.c > > +++ b/hw/msix.c > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > > { > > int config_offset; > > uint8_t *config; > > - uint32_t new_size; > > > > - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) > > - return -EINVAL; > > - if (bar_size > 0x80000000) > > - return -ENOSPC; > > - > > - /* Add space for MSI-X structures */ > > - if (!bar_size) { > > - new_size = MSIX_PAGE_SIZE; > > - } else if (bar_size < MSIX_PAGE_SIZE) { > > - bar_size = MSIX_PAGE_SIZE; > > - new_size = MSIX_PAGE_SIZE * 2; > > - } else { > > - new_size = bar_size * 2; > > - } > > - > > - pdev->msix_bar_size = new_size; > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > - if (config_offset < 0) > > - return config_offset; > > - config = pdev->config + config_offset; > > - > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > - /* Table on top of BAR */ > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > - /* Pending bits on top of that */ > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > - bar_nr); > > + pdev->msix_bar_size = bar_size; > > + > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); > > + > > + if (!config_offset) { > > + uint32_t new_size; > > + > > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) > > + return -EINVAL; > > + if (bar_size > 0x80000000) > > + return -ENOSPC; > > + > > + /* Add space for MSI-X structures */ > > + if (!bar_size) { > > + new_size = MSIX_PAGE_SIZE; > > + } else if (bar_size < MSIX_PAGE_SIZE) { > > + bar_size = MSIX_PAGE_SIZE; > > + new_size = MSIX_PAGE_SIZE * 2; > > + } else { > > + new_size = bar_size * 2; > > + } > > + > > + pdev->msix_bar_size = new_size; > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, > > + MSIX_CAP_LENGTH); > > + if (config_offset < 0) > > + return config_offset; > > + config = pdev->config + config_offset; > > + > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > + /* Table on top of BAR */ > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > + /* Pending bits on top of that */ > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > + bar_nr); > > + } > > pdev->msix_cap = config_offset; > > /* Make flags bit writeable. */ > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, > > return; > > if (size <= offset) > > return; > > - cpu_register_physical_memory(addr + offset, size - offset, > > + cpu_register_physical_memory(addr + offset, > > + MIN(size - offset, MSIX_PAGE_SIZE), > > This is wrong I think, the table might not fit in a single page. > You would need to read table size out of from device config. That's true, but I was hoping to save that for later since we don't seem to be running into that problem yet. Current device assignment code assumes a single page, and I haven't heard of anyone with a vector table that exceeds that yet. Thanks, Alex
On 10/23/2010 06:55 PM, Alex Williamson wrote: > On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote: > > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote: > > > To enable common msix support to be used with pass through devices, > > > don't attempt to change the BAR if the device already has an > > > MSI-X capability. This also means we want to pay closer attention > > > to the size when we map the msix table page, as it isn't necessarily > > > covering the entire end of the BAR. > > > > > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com> > > > --- > > > > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- > > > 1 files changed, 38 insertions(+), 29 deletions(-) > > > > > > diff --git a/hw/msix.c b/hw/msix.c > > > index 43efbd2..4122395 100644 > > > --- a/hw/msix.c > > > +++ b/hw/msix.c > > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > > > { > > > int config_offset; > > > uint8_t *config; > > > - uint32_t new_size; > > > > > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > - return -EINVAL; > > > - if (bar_size> 0x80000000) > > > - return -ENOSPC; > > > - > > > - /* Add space for MSI-X structures */ > > > - if (!bar_size) { > > > - new_size = MSIX_PAGE_SIZE; > > > - } else if (bar_size< MSIX_PAGE_SIZE) { > > > - bar_size = MSIX_PAGE_SIZE; > > > - new_size = MSIX_PAGE_SIZE * 2; > > > - } else { > > > - new_size = bar_size * 2; > > > - } > > > - > > > - pdev->msix_bar_size = new_size; > > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > > - if (config_offset< 0) > > > - return config_offset; > > > - config = pdev->config + config_offset; > > > - > > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > - /* Table on top of BAR */ > > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > - /* Pending bits on top of that */ > > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > - bar_nr); > > > + pdev->msix_bar_size = bar_size; > > > + > > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); > > > + > > > + if (!config_offset) { > > > + uint32_t new_size; > > > + > > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > + return -EINVAL; > > > + if (bar_size> 0x80000000) > > > + return -ENOSPC; > > > + > > > + /* Add space for MSI-X structures */ > > > + if (!bar_size) { > > > + new_size = MSIX_PAGE_SIZE; > > > + } else if (bar_size< MSIX_PAGE_SIZE) { > > > + bar_size = MSIX_PAGE_SIZE; > > > + new_size = MSIX_PAGE_SIZE * 2; > > > + } else { > > > + new_size = bar_size * 2; > > > + } > > > + > > > + pdev->msix_bar_size = new_size; > > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, > > > + MSIX_CAP_LENGTH); > > > + if (config_offset< 0) > > > + return config_offset; > > > + config = pdev->config + config_offset; > > > + > > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > + /* Table on top of BAR */ > > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > + /* Pending bits on top of that */ > > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > + bar_nr); > > > + } > > > pdev->msix_cap = config_offset; > > > /* Make flags bit writeable. */ > > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, > > > return; > > > if (size<= offset) > > > return; > > > - cpu_register_physical_memory(addr + offset, size - offset, > > > + cpu_register_physical_memory(addr + offset, > > > + MIN(size - offset, MSIX_PAGE_SIZE), > > > > This is wrong I think, the table might not fit in a single page. > > You would need to read table size out of from device config. > > That's true, but I was hoping to save that for later since we don't seem > to be running into that problem yet. Current device assignment code > assumes a single page, and I haven't heard of anyone with a vector table > that exceeds that yet. Thanks, > Ok; applied. Please add some warning if the condition happens so the breakage is at least not silent.
On Thu, 2010-10-28 at 17:00 +0200, Avi Kivity wrote: > On 10/23/2010 06:55 PM, Alex Williamson wrote: > > On Sat, 2010-10-23 at 18:18 +0200, Michael S. Tsirkin wrote: > > > On Fri, Oct 22, 2010 at 02:40:31PM -0600, Alex Williamson wrote: > > > > To enable common msix support to be used with pass through devices, > > > > don't attempt to change the BAR if the device already has an > > > > MSI-X capability. This also means we want to pay closer attention > > > > to the size when we map the msix table page, as it isn't necessarily > > > > covering the entire end of the BAR. > > > > > > > > Signed-off-by: Alex Williamson<alex.williamson@redhat.com> > > > > --- > > > > > > > > hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- > > > > 1 files changed, 38 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/hw/msix.c b/hw/msix.c > > > > index 43efbd2..4122395 100644 > > > > --- a/hw/msix.c > > > > +++ b/hw/msix.c > > > > @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > > > > { > > > > int config_offset; > > > > uint8_t *config; > > > > - uint32_t new_size; > > > > > > > > - if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > > - return -EINVAL; > > > > - if (bar_size> 0x80000000) > > > > - return -ENOSPC; > > > > - > > > > - /* Add space for MSI-X structures */ > > > > - if (!bar_size) { > > > > - new_size = MSIX_PAGE_SIZE; > > > > - } else if (bar_size< MSIX_PAGE_SIZE) { > > > > - bar_size = MSIX_PAGE_SIZE; > > > > - new_size = MSIX_PAGE_SIZE * 2; > > > > - } else { > > > > - new_size = bar_size * 2; > > > > - } > > > > - > > > > - pdev->msix_bar_size = new_size; > > > > - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > > > - if (config_offset< 0) > > > > - return config_offset; > > > > - config = pdev->config + config_offset; > > > > - > > > > - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > > - /* Table on top of BAR */ > > > > - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > > - /* Pending bits on top of that */ > > > > - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > > - bar_nr); > > > > + pdev->msix_bar_size = bar_size; > > > > + > > > > + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); > > > > + > > > > + if (!config_offset) { > > > > + uint32_t new_size; > > > > + > > > > + if (nentries< 1 || nentries> PCI_MSIX_FLAGS_QSIZE + 1) > > > > + return -EINVAL; > > > > + if (bar_size> 0x80000000) > > > > + return -ENOSPC; > > > > + > > > > + /* Add space for MSI-X structures */ > > > > + if (!bar_size) { > > > > + new_size = MSIX_PAGE_SIZE; > > > > + } else if (bar_size< MSIX_PAGE_SIZE) { > > > > + bar_size = MSIX_PAGE_SIZE; > > > > + new_size = MSIX_PAGE_SIZE * 2; > > > > + } else { > > > > + new_size = bar_size * 2; > > > > + } > > > > + > > > > + pdev->msix_bar_size = new_size; > > > > + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, > > > > + MSIX_CAP_LENGTH); > > > > + if (config_offset< 0) > > > > + return config_offset; > > > > + config = pdev->config + config_offset; > > > > + > > > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > > > + /* Table on top of BAR */ > > > > + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); > > > > + /* Pending bits on top of that */ > > > > + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | > > > > + bar_nr); > > > > + } > > > > pdev->msix_cap = config_offset; > > > > /* Make flags bit writeable. */ > > > > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > > > > @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, > > > > return; > > > > if (size<= offset) > > > > return; > > > > - cpu_register_physical_memory(addr + offset, size - offset, > > > > + cpu_register_physical_memory(addr + offset, > > > > + MIN(size - offset, MSIX_PAGE_SIZE), > > > > > > This is wrong I think, the table might not fit in a single page. > > > You would need to read table size out of from device config. > > > > That's true, but I was hoping to save that for later since we don't seem > > to be running into that problem yet. Current device assignment code > > assumes a single page, and I haven't heard of anyone with a vector table > > that exceeds that yet. Thanks, > > > > Ok; applied. Please add some warning if the condition happens so the > breakage is at least not silent. Looks like this is already handled before we even get here. msix_init() checks nentries against the max supported in a single page and returns -EINVAL if over. So I think we're safely covered until we add support for more pages. Thanks, Alex
diff --git a/hw/msix.c b/hw/msix.c index 43efbd2..4122395 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -167,35 +167,43 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, { int config_offset; uint8_t *config; - uint32_t new_size; - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) - return -EINVAL; - if (bar_size > 0x80000000) - return -ENOSPC; - - /* Add space for MSI-X structures */ - if (!bar_size) { - new_size = MSIX_PAGE_SIZE; - } else if (bar_size < MSIX_PAGE_SIZE) { - bar_size = MSIX_PAGE_SIZE; - new_size = MSIX_PAGE_SIZE * 2; - } else { - new_size = bar_size * 2; - } - - pdev->msix_bar_size = new_size; - config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); - if (config_offset < 0) - return config_offset; - config = pdev->config + config_offset; - - pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); - /* Table on top of BAR */ - pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); - /* Pending bits on top of that */ - pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | - bar_nr); + pdev->msix_bar_size = bar_size; + + config_offset = pci_find_capability(pdev, PCI_CAP_ID_MSIX); + + if (!config_offset) { + uint32_t new_size; + + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) + return -EINVAL; + if (bar_size > 0x80000000) + return -ENOSPC; + + /* Add space for MSI-X structures */ + if (!bar_size) { + new_size = MSIX_PAGE_SIZE; + } else if (bar_size < MSIX_PAGE_SIZE) { + bar_size = MSIX_PAGE_SIZE; + new_size = MSIX_PAGE_SIZE * 2; + } else { + new_size = bar_size * 2; + } + + pdev->msix_bar_size = new_size; + config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX, + MSIX_CAP_LENGTH); + if (config_offset < 0) + return config_offset; + config = pdev->config + config_offset; + + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); + /* Table on top of BAR */ + pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr); + /* Pending bits on top of that */ + pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) | + bar_nr); + } pdev->msix_cap = config_offset; /* Make flags bit writeable. */ pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | @@ -337,7 +345,8 @@ void msix_mmio_map(PCIDevice *d, int region_num, return; if (size <= offset) return; - cpu_register_physical_memory(addr + offset, size - offset, + cpu_register_physical_memory(addr + offset, + MIN(size - offset, MSIX_PAGE_SIZE), d->msix_mmio_index); }
To enable common msix support to be used with pass through devices, don't attempt to change the BAR if the device already has an MSI-X capability. This also means we want to pay closer attention to the size when we map the msix table page, as it isn't necessarily covering the entire end of the BAR. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- hw/msix.c | 67 +++++++++++++++++++++++++++++++++++-------------------------- 1 files changed, 38 insertions(+), 29 deletions(-)