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

[Jan Kiszka]

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 <address@hidden>
> ---
>  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)

> +{
> +    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.

> +
> +    return msg;
> +}
> +
> +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);
> +}
> +
>  bool msi_enabled(const PCIDevice *dev)
>  {
>      return msi_present(dev) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..4b0f4f8 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -31,6 +31,8 @@ struct MSIMessage {
>  
>  extern bool msi_supported;
>  
> +MSIMessage msi_get_message(PCIDevice *dev);
> +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..9e8d8bb 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -35,7 +35,7 @@
>  #define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
>  #define MSIX_MAX_ENTRIES 32
>  
> -static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>  {
>      uint8_t *table_entry = dev->msix_table_page + vector * 
> PCI_MSIX_ENTRY_SIZE;
>      MSIMessage msg;
> @@ -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..3374cf8 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,6 +4,9 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> +MSIMessage msix_get_message(PCIDevice *dev, unsigned vector);
> +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);
> 

General remark: You will make the life of the maintainers easier by
formatting your patch in a way that a clean merge via git works without
hand-editing. E.g. this patch was no scissor line (---8<--- etc.)
between introductory text and the patch description. And the subject is
not "[PATCH] ...".

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux