From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58760) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ShedC-0007CL-W9 for qemu-devel@nongnu.org; Thu, 21 Jun 2012 06:29:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Shed3-0005jM-1b for qemu-devel@nongnu.org; Thu, 21 Jun 2012 06:28:54 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:62243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Shed2-0005j9-Nh for qemu-devel@nongnu.org; Thu, 21 Jun 2012 06:28:44 -0400 Received: by dadn2 with SMTP id n2so705324dad.4 for ; Thu, 21 Jun 2012 03:28:41 -0700 (PDT) Message-ID: <4FE2F756.8020509@ozlabs.ru> Date: Thu, 21 Jun 2012 20:28:38 +1000 From: Alexey Kardashevskiy 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> <4FE2CFC8.509@siemens.com> In-Reply-To: <4FE2CFC8.509@siemens.com> 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: Jan Kiszka Cc: Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 21/06/12 17:39, Jan Kiszka wrote: > 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) Who/how/why is going to calculate the vector here? > >> +{ >> + 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. What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look? > >> + >> + 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] ...". In general you're right. I just kept in mind that this patch will be rejected anyway. -- Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Date: Thu, 21 Jun 2012 10:28:38 +0000 Subject: Re: [PATCH] msi/msix: added public API to set/get MSI message address, and data Message-Id: <4FE2F756.8020509@ozlabs.ru> 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> <4FE2CFC8.509@siemens.com> In-Reply-To: <4FE2CFC8.509@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jan Kiszka Cc: Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 21/06/12 17:39, Jan Kiszka wrote: > 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) Who/how/why is going to calculate the vector here? > >> +{ >> + 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. What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look? > >> + >> + 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] ...". In general you're right. I just kept in mind that this patch will be rejected anyway. -- Alexey