Re: [PATCH v3 2/9] pci-host: add pcie-msi read method

[Peter Maydell]

On Tue, 30 Jun 2020 at 13:30, P J P <ppandit@redhat.com> wrote:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Add pcie-msi mmio read method to avoid NULL pointer dereference
> issue.

This change is specific to the designware pci host controller;
it would be nice to have "designware" in the commit subject.


> Reported-by: Lei Sun <slei.casper@gmail.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/pci-host/designware.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> Update v3: Add Reviewed-by: ...
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09400.html

> +static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
> +                                              unsigned size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
> +    return 0;

This is the right change, but is missing an explanation
of why it's right:

Looking at the data sheet, in the real hardware MSI interrupts
are handled by looking at writes to see whether they match the
configured address; if so then the write gets quashed and never
gets put out onto the AXI bus (to the CPU, memory, etc). This only
happens for writes, so reads from the magic address are just
allowed to pass through and will read from the system address
space like any other read from any other address. That's not trivial
to implement, though, and well-behaved guests won't care, so
we can just explain why we're not doing anything with a suitable
comment:

    /*
     * Attempts to read from the MSI address are undefined in
     * the PCI specifications. For this hardware, the datasheet
     * specifies that a read from the magic address is simply not
     * intercepted by the MSI controller, and will go out to the
     * AHB/AXI bus like any other PCI-device-initiated DMA read.
     * This is not trivial to implement in QEMU, so since
     * well-behaved guests won't ever ask a PCI device to DMA from
     * this address we just log the missing functionality.
     */
    qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
    return 0;

> +}
> +
>  static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
>                                             uint64_t val, unsigned len)
>  {
> @@ -77,6 +85,7 @@ static void designware_pcie_root_msi_write(void *opaque, 
> hwaddr addr,
>  }
>
>  static const MemoryRegionOps designware_pci_host_msi_ops = {
> +    .read = designware_pcie_root_msi_read,
>      .write = designware_pcie_root_msi_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid = {

With that comment,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM