From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50721) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf26u-0006b2-RQ for qemu-devel@nongnu.org; Thu, 14 Jun 2012 00:56:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sf26t-0005ve-2E for qemu-devel@nongnu.org; Thu, 14 Jun 2012 00:56:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57469) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf26s-0005vO-Q8 for qemu-devel@nongnu.org; Thu, 14 Jun 2012 00:56:42 -0400 Message-ID: <1339649800.24818.3.camel@ul30vt> From: Alex Williamson Date: Wed, 13 Jun 2012 22:56:40 -0600 In-Reply-To: <4FD9693E.2090508@ozlabs.ru> References: <4FD968BB.2000505@ozlabs.ru> <4FD9693E.2090508@ozlabs.ru> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/3] msi/msix: added functions to API to set up message address and data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: "qemu-devel@nongnu.org" , kvm-ppc@vger.kernel.org On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote: > Normally QEMU expects the guest to initialize MSI/MSIX vectors. > However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and > does not write these vectors to device's config space or MSIX BAR. > > On the other hand, msi_notify()/msix_notify() write to these vectors to > signal the guest about an interrupt so we have to write correct vectors > to the devices in order not to change every user of MSI/MSIX. > > The first aim is to support MSIX for virtio-pci on POWER. There is > another patch for POWER coming which introduces a special memory region > where MSI/MSIX vectors point to. > > Signed-off-by: Alexey Kardashevskiy > --- > hw/msi.c | 14 ++++++++++++++ > hw/msi.h | 1 + > hw/msix.c | 10 ++++++++++ > hw/msix.h | 3 +++ > 4 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/hw/msi.c b/hw/msi.c > index 5d6ceb6..124878a 100644 > --- a/hw/msi.c > +++ b/hw/msi.c > @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) > uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > return msi_nr_vectors(flags); > } > + > +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data) > +{ > + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > + bool msi64bit = flags & PCI_MSI_FLAGS_64BIT; > + > + if (msi64bit) { > + pci_set_quad(dev->config + msi_address_lo_off(dev), address); > + } else { > + pci_set_long(dev->config + msi_address_lo_off(dev), address); > + } > + pci_set_word(dev->config + msi_data_off(dev, msi64bit), data); > +} Why not make it msi_set_message() and pass MSIMessage? I'd be great if you tossed in a msi_get_message() as well, I think we need it to be able to do a kvm_irqchip_add_msi_route() with MSI. Thanks, Alex > + > diff --git a/hw/msi.h b/hw/msi.h > index 3040bb0..0acf434 100644 > --- a/hw/msi.h > +++ b/hw/msi.h > @@ -34,6 +34,7 @@ void msi_reset(PCIDevice *dev); > void msi_notify(PCIDevice *dev, unsigned int vector); > void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); > unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); > +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data); > > static inline bool msi_present(const PCIDevice *dev) > { > diff --git a/hw/msix.c b/hw/msix.c > index 3835eaa..c57c299 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -414,3 +414,13 @@ void msix_unuse_all_vectors(PCIDevice *dev) > return; > msix_free_irq_entries(dev); > } > + > +void msix_set_address_data(PCIDevice *dev, int vector, > + uint64_t address, uint32_t data) > +{ > + uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > + pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, address); > + pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, data); > + table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT; > +} > + > diff --git a/hw/msix.h b/hw/msix.h > index 5aba22b..e6bb696 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -29,4 +29,7 @@ void msix_notify(PCIDevice *dev, unsigned vector); > > void msix_reset(PCIDevice *dev); > > +void msix_set_address_data(PCIDevice *dev, int vector, > + uint64_t address, uint32_t data); > + > #endif From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Date: Thu, 14 Jun 2012 04:56:40 +0000 Subject: Re: [Qemu-devel] [PATCH 1/3] msi/msix: added functions to API to set up message address and data Message-Id: <1339649800.24818.3.camel@ul30vt> List-Id: References: <4FD968BB.2000505@ozlabs.ru> <4FD9693E.2090508@ozlabs.ru> In-Reply-To: <4FD9693E.2090508@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexey Kardashevskiy Cc: "qemu-devel@nongnu.org" , kvm-ppc@vger.kernel.org On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote: > Normally QEMU expects the guest to initialize MSI/MSIX vectors. > However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and > does not write these vectors to device's config space or MSIX BAR. > > On the other hand, msi_notify()/msix_notify() write to these vectors to > signal the guest about an interrupt so we have to write correct vectors > to the devices in order not to change every user of MSI/MSIX. > > The first aim is to support MSIX for virtio-pci on POWER. There is > another patch for POWER coming which introduces a special memory region > where MSI/MSIX vectors point to. > > Signed-off-by: Alexey Kardashevskiy > --- > hw/msi.c | 14 ++++++++++++++ > hw/msi.h | 1 + > hw/msix.c | 10 ++++++++++ > hw/msix.h | 3 +++ > 4 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/hw/msi.c b/hw/msi.c > index 5d6ceb6..124878a 100644 > --- a/hw/msi.c > +++ b/hw/msi.c > @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) > uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > return msi_nr_vectors(flags); > } > + > +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data) > +{ > + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > + bool msi64bit = flags & PCI_MSI_FLAGS_64BIT; > + > + if (msi64bit) { > + pci_set_quad(dev->config + msi_address_lo_off(dev), address); > + } else { > + pci_set_long(dev->config + msi_address_lo_off(dev), address); > + } > + pci_set_word(dev->config + msi_data_off(dev, msi64bit), data); > +} Why not make it msi_set_message() and pass MSIMessage? I'd be great if you tossed in a msi_get_message() as well, I think we need it to be able to do a kvm_irqchip_add_msi_route() with MSI. Thanks, Alex > + > diff --git a/hw/msi.h b/hw/msi.h > index 3040bb0..0acf434 100644 > --- a/hw/msi.h > +++ b/hw/msi.h > @@ -34,6 +34,7 @@ void msi_reset(PCIDevice *dev); > void msi_notify(PCIDevice *dev, unsigned int vector); > void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); > unsigned int msi_nr_vectors_allocated(const PCIDevice *dev); > +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data); > > static inline bool msi_present(const PCIDevice *dev) > { > diff --git a/hw/msix.c b/hw/msix.c > index 3835eaa..c57c299 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -414,3 +414,13 @@ void msix_unuse_all_vectors(PCIDevice *dev) > return; > msix_free_irq_entries(dev); > } > + > +void msix_set_address_data(PCIDevice *dev, int vector, > + uint64_t address, uint32_t data) > +{ > + uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > + pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, address); > + pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, data); > + table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT; > +} > + > diff --git a/hw/msix.h b/hw/msix.h > index 5aba22b..e6bb696 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -29,4 +29,7 @@ void msix_notify(PCIDevice *dev, unsigned vector); > > void msix_reset(PCIDevice *dev); > > +void msix_set_address_data(PCIDevice *dev, int vector, > + uint64_t address, uint32_t data); > + > #endif