From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrU86-0001g3-Pb for qemu-devel@nongnu.org; Wed, 18 Jul 2012 09:17:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SrU81-0005n4-SW for qemu-devel@nongnu.org; Wed, 18 Jul 2012 09:17:26 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:42748) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrU81-0005mz-K1 for qemu-devel@nongnu.org; Wed, 18 Jul 2012 09:17:21 -0400 Received: by pbbro12 with SMTP id ro12so2617473pbb.4 for ; Wed, 18 Jul 2012 06:17:20 -0700 (PDT) Message-ID: <5006B758.8000306@ozlabs.ru> Date: Wed, 18 Jul 2012 23:17:12 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <4FD97A94.2080709@siemens.com> <4FE2C33E.1080808@ozlabs.ru> <4FE2C4DA.40403@siemens.com> <4FE2CABB.4070203@ozlabs.ru> <4FE2CFC8.509@siemens.com> <4FE2F756.8020509@ozlabs.ru> <4FE2F9AF.2050006@siemens.com> <4FE2FC5D.8040503@ozlabs.ru> <4FE2FDE1.1090401@siemens.com> <4FE307DE.5070002@ozlabs.ru> <20120718124313.GA5452@redhat.com> In-Reply-To: <20120718124313.GA5452@redhat.com> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Benjamin Herrenschmidt Cc: Jan Kiszka , Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 18/07/12 22:43, Michael S. Tsirkin wrote: > On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote: >> Added (msi|msix)_set_message() functions. >> >> 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. >> >> Signed-off-by: Alexey Kardashevskiy > > So guests do enable MSI through config space, but do > not fill in vectors? Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register. These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug. > Very strange. Are you sure it's not > just a guest bug? How does it work for other PCI devices? Did not get the question. It works the same for every PCI device under POWER guest. > Can't we just fix guest drivers to program the vectors properly? > > Also pls address the comment below. Comment below. > Thanks! > >> --- >> hw/msi.c | 13 +++++++++++++ >> hw/msi.h | 1 + >> hw/msix.c | 9 +++++++++ >> hw/msix.h | 2 ++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/hw/msi.c b/hw/msi.c >> index 5233204..cc6102f 100644 >> --- a/hw/msi.c >> +++ b/hw/msi.c >> @@ -105,6 +105,19 @@ 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); >> } >> >> +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); >> +} >> + > > Please add documentation. Something like > > /* > * Special API for POWER to configure the vectors through > * a side channel. Should never be used by devices. > */ It is useful for any para-virtualized environment I believe, is not it? For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :) >> bool msi_enabled(const PCIDevice *dev) >> { >> return msi_present(dev) && >> diff --git a/hw/msi.h b/hw/msi.h >> index 75747ab..6ec1f99 100644 >> --- a/hw/msi.h >> +++ b/hw/msi.h >> @@ -31,6 +31,7 @@ struct MSIMessage { >> >> extern bool msi_supported; >> >> +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..5f7d6d3 100644 >> --- a/hw/msix.c >> +++ b/hw/msix.c >> @@ -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..26a437e 100644 >> --- a/hw/msix.h >> +++ b/hw/msix.h >> @@ -4,6 +4,8 @@ >> #include "qemu-common.h" >> #include "pci.h" >> >> +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); >> -- >> 1.7.10 >> >> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :) >> >> >> >> >> >> >> On 21/06/12 20:56, Jan Kiszka wrote: >>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote: >>>> On 21/06/12 20:38, Jan Kiszka wrote: >>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote: >>>>>> 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? >>>>> >>>>> I'm not sure what your use case for reading the message is. For KVM >>>>> device assignment it is preparing an alternative message delivery path >>>>> for MSI vectors. And for this we will need vector notifier support for >>>>> MSI as well. You can check the MSI-X code for corresponding use cases of >>>>> msix_get_message. >>>> >>>>> And when we already have msi_get_message, another logical use case is >>>>> msi_notify. See msix.c again. >>>> >>>> Aaaa. >>>> >>>> I have no case for reading the message. All I need is writing. And I want it public as I want to use >>>> it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" >>>> ("get" returns what "set" wrote). You want a different thing which I can do but it is not >>>> msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or >>>> msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify(). >>>> >>>> Still can do what you suggested, it just does not seem right. >>> >>> It is right - when looking at it from a different angle. ;) >>> >>> I don't mind if you add msi_get_message now or leave this to me. Likely >>> the latter is better as you have no use case for msi_get_message (and >>> also msix_get_message!) outside of their modules, thus we should not >>> export those functions anyway. -- Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Date: Wed, 18 Jul 2012 13:17:12 +0000 Subject: Re: [PATCH] msi/msix: added API to set MSI message address and data Message-Id: <5006B758.8000306@ozlabs.ru> List-Id: References: <4FD97A94.2080709@siemens.com> <4FE2C33E.1080808@ozlabs.ru> <4FE2C4DA.40403@siemens.com> <4FE2CABB.4070203@ozlabs.ru> <4FE2CFC8.509@siemens.com> <4FE2F756.8020509@ozlabs.ru> <4FE2F9AF.2050006@siemens.com> <4FE2FC5D.8040503@ozlabs.ru> <4FE2FDE1.1090401@siemens.com> <4FE307DE.5070002@ozlabs.ru> <20120718124313.GA5452@redhat.com> In-Reply-To: <20120718124313.GA5452@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Michael S. Tsirkin" , Benjamin Herrenschmidt Cc: Jan Kiszka , Alex Williamson , "qemu-devel@nongnu.org" , "kvm-ppc@vger.kernel.org" On 18/07/12 22:43, Michael S. Tsirkin wrote: > On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote: >> Added (msi|msix)_set_message() functions. >> >> 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. >> >> Signed-off-by: Alexey Kardashevskiy > > So guests do enable MSI through config space, but do > not fill in vectors? Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register. These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug. > Very strange. Are you sure it's not > just a guest bug? How does it work for other PCI devices? Did not get the question. It works the same for every PCI device under POWER guest. > Can't we just fix guest drivers to program the vectors properly? > > Also pls address the comment below. Comment below. > Thanks! > >> --- >> hw/msi.c | 13 +++++++++++++ >> hw/msi.h | 1 + >> hw/msix.c | 9 +++++++++ >> hw/msix.h | 2 ++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/hw/msi.c b/hw/msi.c >> index 5233204..cc6102f 100644 >> --- a/hw/msi.c >> +++ b/hw/msi.c >> @@ -105,6 +105,19 @@ 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); >> } >> >> +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); >> +} >> + > > Please add documentation. Something like > > /* > * Special API for POWER to configure the vectors through > * a side channel. Should never be used by devices. > */ It is useful for any para-virtualized environment I believe, is not it? For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :) >> bool msi_enabled(const PCIDevice *dev) >> { >> return msi_present(dev) && >> diff --git a/hw/msi.h b/hw/msi.h >> index 75747ab..6ec1f99 100644 >> --- a/hw/msi.h >> +++ b/hw/msi.h >> @@ -31,6 +31,7 @@ struct MSIMessage { >> >> extern bool msi_supported; >> >> +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..5f7d6d3 100644 >> --- a/hw/msix.c >> +++ b/hw/msix.c >> @@ -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..26a437e 100644 >> --- a/hw/msix.h >> +++ b/hw/msix.h >> @@ -4,6 +4,8 @@ >> #include "qemu-common.h" >> #include "pci.h" >> >> +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); >> -- >> 1.7.10 >> >> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :) >> >> >> >> >> >> >> On 21/06/12 20:56, Jan Kiszka wrote: >>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote: >>>> On 21/06/12 20:38, Jan Kiszka wrote: >>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote: >>>>>> 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? >>>>> >>>>> I'm not sure what your use case for reading the message is. For KVM >>>>> device assignment it is preparing an alternative message delivery path >>>>> for MSI vectors. And for this we will need vector notifier support for >>>>> MSI as well. You can check the MSI-X code for corresponding use cases of >>>>> msix_get_message. >>>> >>>>> And when we already have msi_get_message, another logical use case is >>>>> msi_notify. See msix.c again. >>>> >>>> Aaaa. >>>> >>>> I have no case for reading the message. All I need is writing. And I want it public as I want to use >>>> it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" >>>> ("get" returns what "set" wrote). You want a different thing which I can do but it is not >>>> msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or >>>> msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify(). >>>> >>>> Still can do what you suggested, it just does not seem right. >>> >>> It is right - when looking at it from a different angle. ;) >>> >>> I don't mind if you add msi_get_message now or leave this to me. Likely >>> the latter is better as you have no use case for msi_get_message (and >>> also msix_get_message!) outside of their modules, thus we should not >>> export those functions anyway. -- Alexey