On Mon, Oct 17, 2011 at 11:27:57AM +0200, Jan Kiszka wrote:
> MSI config notifiers are supposed to be triggered on every relevant
> configuration change of MSI vectors or if MSI is enabled/disabled.
>
> Two notifiers are established, one for vector changes and one for general
> enabling. The former notifier additionally passes the currently active
> MSI message.
> This will allow to update potential in-kernel IRQ routes on
> changes. The latter notifier is optional and will only be used by a
> subset of clients.
>
> These notifiers are currently only available for MSI-X but will be
> extended to legacy MSI as well.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
Passing message, always, does not seem to make sense: message is only
valid if it is unmasked.
Further, IIRC the spec requires any changes to be done while
message is masked. So mask notifier makes more sense to me:
it does the same thing using one notifier that you do
using two notifiers.
> ---
> hw/msix.c | 119 +++++++++++++++++++++++++++++++++++++-----------------
> hw/msix.h | 6 ++-
> hw/pci.h | 8 ++-
> hw/virtio-pci.c | 24 ++++++------
> 4 files changed, 102 insertions(+), 55 deletions(-)
>
> diff --git a/hw/msix.c b/hw/msix.c
> index 247b255..176bc76 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -219,16 +219,24 @@ static bool msix_is_masked(PCIDevice *dev, int vector)
> dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> }
>
> -static void msix_handle_mask_update(PCIDevice *dev, int vector)
> +static void msix_fire_vector_config_notifier(PCIDevice *dev,
> + unsigned int vector, bool masked)
> {
> - bool masked = msix_is_masked(dev, vector);
> + MSIMessage msg;
> int ret;
>
> - if (dev->msix_mask_notifier) {
> - ret = dev->msix_mask_notifier(dev, vector,
> - msix_is_masked(dev, vector));
> + if (dev->msix_vector_config_notifier) {
> + msix_message_from_vector(dev, vector, &msg);
> + ret = dev->msix_vector_config_notifier(dev, vector, &msg, masked);
> assert(ret >= 0);
> }
> +}
> +
> +static void msix_handle_mask_update(PCIDevice *dev, int vector)
> +{
> + bool masked = msix_is_masked(dev, vector);
> +
> + msix_fire_vector_config_notifier(dev, vector, masked);
> if (!masked && msix_is_pending(dev, vector)) {
> msix_clr_pending(dev, vector);
> msix_notify(dev, vector);
> @@ -240,20 +248,27 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> uint32_t old_val, int len)
> {
> unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
> - bool was_masked;
> + bool was_masked, was_enabled, is_enabled;
> int vector;
>
> if (!msix_present(dev) || !range_covers_byte(addr, len, enable_pos)) {
> return;
> }
>
> - if (!msix_enabled(dev)) {
> + old_val >>= (enable_pos - addr) * 8;
> +
> + was_enabled = old_val & MSIX_ENABLE_MASK;
> + is_enabled = msix_enabled(dev);
> + if (was_enabled != is_enabled && dev->msix_enable_notifier) {
> + dev->msix_enable_notifier(dev, is_enabled);
> + }
> +
> + if (!is_enabled) {
> return;
> }
>
> pci_device_deassert_intx(dev);
>
> - old_val >>= (enable_pos - addr) * 8;
> was_masked =
> (old_val & (MSIX_MASKALL_MASK | MSIX_ENABLE_MASK)) != MSIX_ENABLE_MASK;
> if (was_masked != msix_function_masked(dev)) {
> @@ -270,15 +285,20 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> unsigned int vector = offset / PCI_MSIX_ENTRY_SIZE;
> bool was_masked = msix_is_masked(dev, vector);
> + bool is_masked;
>
> pci_set_long(dev->msix_table_page + offset, val);
> if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector));
> }
>
> - if (vector < dev->msix_entries_nr &&
> - was_masked != msix_is_masked(dev, vector)) {
> - msix_handle_mask_update(dev, vector);
> + if (vector < dev->msix_entries_nr) {
> + is_masked = msix_is_masked(dev, vector);
> + if (was_masked != is_masked) {
> + msix_handle_mask_update(dev, vector);
> + } else {
> + msix_fire_vector_config_notifier(dev, vector, is_masked);
> + }
> }
> }
>
> @@ -305,17 +325,17 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
>
> static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> {
> - int vector, r;
> + int vector;
> +
> for (vector = 0; vector < nentries; ++vector) {
> unsigned offset =
> vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> bool was_masked = msix_is_masked(dev, vector);
> +
> dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> - if (was_masked != msix_is_masked(dev, vector) &&
> - dev->msix_mask_notifier) {
> - r = dev->msix_mask_notifier(dev, vector,
> - msix_is_masked(dev, vector));
> - assert(r >= 0);
> +
> + if (!was_masked) {
> + msix_handle_mask_update(dev, vector);
> }
> }
> }
> @@ -337,7 +357,6 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> if (nentries > MSIX_MAX_ENTRIES)
> return -EINVAL;
>
> - dev->msix_mask_notifier = NULL;
> dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> sizeof *dev->msix_entry_used);
>
> @@ -529,36 +548,50 @@ void msix_unuse_all_vectors(PCIDevice *dev)
> }
>
> /* Invoke the notifier if vector entry is used and unmasked. */
> -static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int masked)
> +static int
> +msix_notify_if_unmasked(PCIDevice *dev, unsigned int vector, bool masked)
> {
> - assert(dev->msix_mask_notifier);
> + MSIMessage msg;
> +
> + assert(dev->msix_vector_config_notifier);
> +
> if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
> return 0;
> }
> - return dev->msix_mask_notifier(dev, vector, masked);
> + msix_message_from_vector(dev, vector, &msg);
> + return dev->msix_vector_config_notifier(dev, vector, &msg, masked);
> }
>
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +static int
> +msix_set_config_notifier_for_vector(PCIDevice *dev, unsigned int vector)
> {
> - /* Notifier has been set. Invoke it on unmasked vectors. */
> - return msix_notify_if_unmasked(dev, vector, 0);
> + /* Notifier has been set. Invoke it on unmasked vectors. */
> + return msix_notify_if_unmasked(dev, vector, false);
> }
>
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +static int
> +msix_unset_config_notifier_for_vector(PCIDevice *dev, unsigned int vector)
> {
> - /* Notifier will be unset. Invoke it to mask unmasked entries. */
> - return msix_notify_if_unmasked(dev, vector, 1);
> + /* Notifier will be unset. Invoke it to mask unmasked entries. */
> + return msix_notify_if_unmasked(dev, vector, true);
> }
>
> -int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> +int msix_set_config_notifiers(PCIDevice *dev,
> + MSIEnableNotifier enable_notifier,
> + MSIVectorConfigNotifier vector_config_notifier)
> {
> int r, n;
> - assert(!dev->msix_mask_notifier);
> - dev->msix_mask_notifier = f;
> +
> + dev->msix_enable_notifier = enable_notifier;
> + dev->msix_vector_config_notifier = vector_config_notifier;
> +
> + if (enable_notifier && msix_enabled(dev)) {
> + enable_notifier(dev, true);
> + }
> if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> for (n = 0; n < dev->msix_entries_nr; ++n) {
> - r = msix_set_mask_notifier_for_vector(dev, n);
> + r = msix_set_config_notifier_for_vector(dev, n);
> if (r < 0) {
> goto undo;
> }
> @@ -568,31 +601,41 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>
> undo:
> while (--n >= 0) {
> - msix_unset_mask_notifier_for_vector(dev, n);
> + msix_unset_config_notifier_for_vector(dev, n);
> }
> - dev->msix_mask_notifier = NULL;
> + if (enable_notifier && msix_enabled(dev)) {
> + enable_notifier(dev, false);
> + }
> + dev->msix_enable_notifier = NULL;
> + dev->msix_vector_config_notifier = NULL;
> return r;
> }
>
> -int msix_unset_mask_notifier(PCIDevice *dev)
> +int msix_unset_config_notifiers(PCIDevice *dev)
> {
> int r, n;
> - assert(dev->msix_mask_notifier);
> +
> + assert(dev->msix_vector_config_notifier);
> +
> if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> for (n = 0; n < dev->msix_entries_nr; ++n) {
> - r = msix_unset_mask_notifier_for_vector(dev, n);
> + r = msix_unset_config_notifier_for_vector(dev, n);
> if (r < 0) {
> goto undo;
> }
> }
> }
> - dev->msix_mask_notifier = NULL;
> + if (dev->msix_enable_notifier && msix_enabled(dev)) {
> + dev->msix_enable_notifier(dev, false);
> + }
> + dev->msix_enable_notifier = NULL;
> + dev->msix_vector_config_notifier = NULL;
> return 0;
>
> undo:
> while (--n >= 0) {
> - msix_set_mask_notifier_for_vector(dev, n);
> + msix_set_config_notifier_for_vector(dev, n);
> }
> return r;
> }
> diff --git a/hw/msix.h b/hw/msix.h
> index 685dbe2..978f417 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -29,6 +29,8 @@ void msix_notify(PCIDevice *dev, unsigned vector);
>
> void msix_reset(PCIDevice *dev);
>
> -int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func);
> -int msix_unset_mask_notifier(PCIDevice *dev);
> +int msix_set_config_notifiers(PCIDevice *dev,
> + MSIEnableNotifier enable_notifier,
> + MSIVectorConfigNotifier vector_config_notifier);
> +int msix_unset_config_notifiers(PCIDevice *dev);
> #endif
> diff --git a/hw/pci.h b/hw/pci.h
> index 0177df4..4249c6a 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -127,8 +127,9 @@ enum {
> QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> };
>
> -typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
> - int masked);
> +typedef void (*MSIEnableNotifier)(PCIDevice *dev, bool enabled);
> +typedef int (*MSIVectorConfigNotifier)(PCIDevice *dev, unsigned int vector,
> + MSIMessage *msg, bool masked);
>
> struct PCIDevice {
> DeviceState qdev;
> @@ -210,7 +211,8 @@ struct PCIDevice {
> * on the rest of the region. */
> target_phys_addr_t msix_page_size;
>
> - msix_mask_notifier_func msix_mask_notifier;
> + MSIEnableNotifier msix_enable_notifier;
> + MSIVectorConfigNotifier msix_vector_config_notifier;
> };
>
> PCIDevice *pci_register_device(PCIBus *bus, const char *name,
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ad6a002..6718945 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -520,8 +520,8 @@ static void virtio_pci_guest_notifier_read(void *opaque)
> }
> }
>
> -static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
> - VirtQueue *vq, int masked)
> +static int virtio_pci_mask_vq(PCIDevice *dev, unsigned int vector,
> + VirtQueue *vq, bool masked)
> {
> EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
> int r = kvm_msi_irqfd_set(&dev->msix_cache[vector],
> @@ -540,8 +540,8 @@ static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
> return 0;
> }
>
> -static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
> - int masked)
> +static int virtio_pci_msi_vector_config(PCIDevice *dev, unsigned int vector,
> + MSIMessage *msg, bool masked)
> {
> VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
> VirtIODevice *vdev = proxy->vdev;
> @@ -608,11 +608,11 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
> VirtIODevice *vdev = proxy->vdev;
> int r, n;
>
> - /* Must unset mask notifier while guest notifier
> + /* Must unset vector config notifier while guest notifier
> * is still assigned */
> if (!assign) {
> - r = msix_unset_mask_notifier(&proxy->pci_dev);
> - assert(r >= 0);
> + r = msix_unset_config_notifiers(&proxy->pci_dev);
> + assert(r >= 0);
> }
>
> for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> @@ -626,11 +626,11 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
> }
> }
>
> - /* Must set mask notifier after guest notifier
> + /* Must set vector config notifier after guest notifier
> * has been assigned */
> if (assign) {
> - r = msix_set_mask_notifier(&proxy->pci_dev,
> - virtio_pci_mask_notifier);
> + r = msix_set_config_notifiers(&proxy->pci_dev, NULL,
> + virtio_pci_msi_vector_config);
> if (r < 0) {
> goto assign_error;
> }
> @@ -645,8 +645,8 @@ assign_error:
> }
>
> if (!assign) {
> - msix_set_mask_notifier(&proxy->pci_dev,
> - virtio_pci_mask_notifier);
> + msix_set_config_notifiers(&proxy->pci_dev, NULL,
> + virtio_pci_msi_vector_config);
> }
> return r;
> }
> --
> 1.7.3.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
