From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Shbzo-0003ek-HI for qemu-devel@nongnu.org; Thu, 21 Jun 2012 03:40:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Shbzh-0001HC-V9 for qemu-devel@nongnu.org; Thu, 21 Jun 2012 03:40:04 -0400 Received: from thoth.sbs.de ([192.35.17.2]:31113) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Shbzh-0001D5-Kg for qemu-devel@nongnu.org; Thu, 21 Jun 2012 03:39:57 -0400 Message-ID: <4FE2CFC8.509@siemens.com> Date: Thu, 21 Jun 2012 09:39:52 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <4FD968BB.2000505@ozlabs.ru> <4FD9693E.2090508@ozlabs.ru> <1339649800.24818.3.camel@ul30vt> <4FD973F7.7080207@ozlabs.ru> <4FD97A94.2080709@siemens.com> <4FE2C33E.1080808@ozlabs.ru> <4FE2C4DA.40403@siemens.com> <4FE2CABB.4070203@ozlabs.ru> In-Reply-To: <4FE2CABB.4070203@ozlabs.ru> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 2012-06-21 09:18, Alexey Kardashevskiy wrote: > > agrhhh. sha1 of the patch changed after rebasing :) > > > > Added (msi|msix)_(set|get)_message() function for whoever might > want to use them. > > Currently msi_notify()/msix_notify() write to these vectors to > signal the guest about an interrupt so the correct values have to > written there by the guest or QEMU. > > For example, POWER guest never initializes MSI/MSIX vectors, instead > it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on > POWER we have to initialize MSI/MSIX message from QEMU. > > As only set* function are required by now, the "get" functions were added > or made public for a symmetry. > > Signed-off-by: Alexey Kardashevskiy > --- > hw/msi.c | 29 +++++++++++++++++++++++++++++ > hw/msi.h | 2 ++ > hw/msix.c | 11 ++++++++++- > hw/msix.h | 3 +++ > 4 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/hw/msi.c b/hw/msi.c > index 5233204..9ad84a4 100644 > --- a/hw/msi.c > +++ b/hw/msi.c > @@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) > return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32); > } > > +MSIMessage msi_get_message(PCIDevice *dev) MSIMessage msi_get_message(PCIDevice *dev, unsigned vector) > +{ > + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > + bool msi64bit = flags & PCI_MSI_FLAGS_64BIT; > + MSIMessage msg; > + > + if (msi64bit) { > + msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev)); > + } else { > + msg.address = pci_get_long(dev->config + msi_address_lo_off(dev)); > + } > + msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit)); And I have this here in addition: unsigned int nr_vectors = msi_nr_vectors(flags); ... if (nr_vectors > 1) { msg.data &= ~(nr_vectors - 1); msg.data |= vector; } See PCI spec and existing code. > + > + return msg; > +} > + > +void msi_set_message(PCIDevice *dev, MSIMessage msg) > +{ > + 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), msg.address); > + } else { > + pci_set_long(dev->config + msi_address_lo_off(dev), msg.address); > + } > + pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data); > +} > + > bool msi_enabled(const PCIDevice *dev) > { > return msi_present(dev) && > diff --git a/hw/msi.h b/hw/msi.h > index 75747ab..4b0f4f8 100644 > --- a/hw/msi.h > +++ b/hw/msi.h > @@ -31,6 +31,8 @@ struct MSIMessage { > > extern bool msi_supported; > > +MSIMessage msi_get_message(PCIDevice *dev); > +void msi_set_message(PCIDevice *dev, MSIMessage msg); > bool msi_enabled(const PCIDevice *dev); > int msi_init(struct PCIDevice *dev, uint8_t offset, > unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask); > diff --git a/hw/msix.c b/hw/msix.c > index ded3c55..9e8d8bb 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -35,7 +35,7 @@ > #define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2) > #define MSIX_MAX_ENTRIES 32 > > -static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > { > uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > MSIMessage msg; > @@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > return msg; > } > > +void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg) > +{ > + uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > + > + pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address); > + pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data); > + table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT; > +} > + > /* Add MSI-X capability to the config space for the device. */ > /* Given a bar and its size, add MSI-X table on top of it > * and fill MSI-X capability in the config space. > diff --git a/hw/msix.h b/hw/msix.h > index 50aee82..3374cf8 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -4,6 +4,9 @@ > #include "qemu-common.h" > #include "pci.h" > > +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector); > +void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg); > + > int msix_init(PCIDevice *pdev, unsigned short nentries, > MemoryRegion *bar, > unsigned bar_nr, unsigned bar_size); > General remark: You will make the life of the maintainers easier by formatting your patch in a way that a clean merge via git works without hand-editing. E.g. this patch was no scissor line (---8<--- etc.) between introductory text and the patch description. And the subject is not "[PATCH] ...". Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Date: Thu, 21 Jun 2012 07:39:52 +0000 Subject: Re: [PATCH] msi/msix: added public API to set/get MSI message address, and data Message-Id: <4FE2CFC8.509@siemens.com> List-Id: References: <4FD968BB.2000505@ozlabs.ru> <4FD9693E.2090508@ozlabs.ru> <1339649800.24818.3.camel@ul30vt> <4FD973F7.7080207@ozlabs.ru> <4FD97A94.2080709@siemens.com> <4FE2C33E.1080808@ozlabs.ru> <4FE2C4DA.40403@siemens.com> <4FE2CABB.4070203@ozlabs.ru> In-Reply-To: <4FE2CABB.4070203@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexey Kardashevskiy Cc: Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 2012-06-21 09:18, Alexey Kardashevskiy wrote: > > agrhhh. sha1 of the patch changed after rebasing :) > > > > Added (msi|msix)_(set|get)_message() function for whoever might > want to use them. > > Currently msi_notify()/msix_notify() write to these vectors to > signal the guest about an interrupt so the correct values have to > written there by the guest or QEMU. > > For example, POWER guest never initializes MSI/MSIX vectors, instead > it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on > POWER we have to initialize MSI/MSIX message from QEMU. > > As only set* function are required by now, the "get" functions were added > or made public for a symmetry. > > Signed-off-by: Alexey Kardashevskiy > --- > hw/msi.c | 29 +++++++++++++++++++++++++++++ > hw/msi.h | 2 ++ > hw/msix.c | 11 ++++++++++- > hw/msix.h | 3 +++ > 4 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/hw/msi.c b/hw/msi.c > index 5233204..9ad84a4 100644 > --- a/hw/msi.c > +++ b/hw/msi.c > @@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) > return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32); > } > > +MSIMessage msi_get_message(PCIDevice *dev) MSIMessage msi_get_message(PCIDevice *dev, unsigned vector) > +{ > + uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev)); > + bool msi64bit = flags & PCI_MSI_FLAGS_64BIT; > + MSIMessage msg; > + > + if (msi64bit) { > + msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev)); > + } else { > + msg.address = pci_get_long(dev->config + msi_address_lo_off(dev)); > + } > + msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit)); And I have this here in addition: unsigned int nr_vectors = msi_nr_vectors(flags); ... if (nr_vectors > 1) { msg.data &= ~(nr_vectors - 1); msg.data |= vector; } See PCI spec and existing code. > + > + return msg; > +} > + > +void msi_set_message(PCIDevice *dev, MSIMessage msg) > +{ > + 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), msg.address); > + } else { > + pci_set_long(dev->config + msi_address_lo_off(dev), msg.address); > + } > + pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data); > +} > + > bool msi_enabled(const PCIDevice *dev) > { > return msi_present(dev) && > diff --git a/hw/msi.h b/hw/msi.h > index 75747ab..4b0f4f8 100644 > --- a/hw/msi.h > +++ b/hw/msi.h > @@ -31,6 +31,8 @@ struct MSIMessage { > > extern bool msi_supported; > > +MSIMessage msi_get_message(PCIDevice *dev); > +void msi_set_message(PCIDevice *dev, MSIMessage msg); > bool msi_enabled(const PCIDevice *dev); > int msi_init(struct PCIDevice *dev, uint8_t offset, > unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask); > diff --git a/hw/msix.c b/hw/msix.c > index ded3c55..9e8d8bb 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -35,7 +35,7 @@ > #define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2) > #define MSIX_MAX_ENTRIES 32 > > -static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > { > uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > MSIMessage msg; > @@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector) > return msg; > } > > +void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg) > +{ > + uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE; > + > + pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address); > + pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data); > + table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT; > +} > + > /* Add MSI-X capability to the config space for the device. */ > /* Given a bar and its size, add MSI-X table on top of it > * and fill MSI-X capability in the config space. > diff --git a/hw/msix.h b/hw/msix.h > index 50aee82..3374cf8 100644 > --- a/hw/msix.h > +++ b/hw/msix.h > @@ -4,6 +4,9 @@ > #include "qemu-common.h" > #include "pci.h" > > +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector); > +void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg); > + > int msix_init(PCIDevice *pdev, unsigned short nentries, > MemoryRegion *bar, > unsigned bar_nr, unsigned bar_size); > General remark: You will make the life of the maintainers easier by formatting your patch in a way that a clean merge via git works without hand-editing. E.g. this patch was no scissor line (---8<--- etc.) between introductory text and the patch description. And the subject is not "[PATCH] ...". Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux