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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.


-- 
Alexey