Re: [PATCH v9 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Dec 03 2024 at 15:36, Frank Li wrote:
> +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc;
> +	struct pci_epf *epf;
> +
> +	epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));
> +	if (!epc)

This is wrong as pci_epc_get() never returns NULL on failure. It returns
an error pointer.

> +		return;
> +
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);

How can the list be empty?

> +	if (epf && epf->db_msg && desc->msi_index < epf->num_db)
> +		memcpy(&epf->db_msg[desc->msi_index].msg, msg, sizeof(*msg));

So now the message is copied out into that db_msg array which is
somewhere in the memory which was allocated on the EP side.

How is the host side supposed to know about the change of the message?

This only works reliably if:

  1) the message address/data pair is immutable once it is set up and
     subsequent affinity changes are not affecting it

  2) The ordering on the EP driver is:

     request_irq()
     publish_msg_to_host()
     tell_host_that_message_is_ready()

#2 is a documentation problem, but #1 needs some thought.

It only works for MSI parent domains which use a translation table and
affinity changes only happen at the translation table level, which means
the address/data pair is unaffected.

Sure GIC-ITS, AMD/Intel remap domains work that way, but what happens if
the underlying MSI parent domain actually changes the message
(address/data pair) during an affinity change?

These domains expect that the message is known to the other side at the
time when irq_set_affinity() returns. In case of regular PCI/MSI this is
not a problem because the message is written to the device before the
function returns, but in this EP case nothing guarantees that the
modified message is host visible at that point.

The fact that a MSI parent domain supports DOMAIN_BUS_DEVICE_MSI does
not guarantee that the parent is translation table based.

As this is intended to be a generic library for all sorts of EP
implementations, there needs to be

  - either a mechanism to prevent the initialization if the underlying
    MSI parent domain does not provide immutable messages

  - or support for endpoint specific msi_write_msg() implementations

Thanks,

        tglx