Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

["Michael S. Tsirkin" <mst@redhat.com>]

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 <aik@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 <aik@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