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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2012-06-21 at 12:56 +0200, 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.

Curse my timezone for not getting to respond before Alexey dropped the
'get' function.  msi_get_message(PCIDevice *dev, int vector) is
necessary for vfio so that we can make use of irqfds in KVM.  I've
actually got this function coded into vfio for the moment because it's
so much fun to get pci changes into qemu.  When MSI gets enabled, we
setup an eventfd in vfio for each vector (linux only supports a single
MSI vector, but we pretend we can support the fully specification).  If
we can enabled irqfd, we walk through each vector, getting an MSIMessage
to program into KVM, which sets the MSI to be injected when the eventfd
fires.  MSI is non-symmetric because the vectors have to be consecutive.
So it doesn't make sense to have a 'set' with a vector number, but a
'get' with the vector number is useful.  Perhaps it should be
msi_set_message(PCIDevice *dev, MSIMessage *msg, int nr_vectors), but I
don't know how POWER is using it either.  Thanks,

Alex