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 <a...@ozlabs.ru>
So guests do enable MSI through config space, but do
not fill in vectors? Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?
Can't we just fix guest drivers to program the vectors properly?
Also pls address the 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.
*/
> 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 <a...@ozlabs.ru>
> >>>>>> ---
> >>>>>> 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.
> >
> > Jan
> >
>
>
> --
> Alexey
