Re: [PATCH v5 18/20] hw/i386: convert apic access to use MemTxAttrs

[Peter Maydell]

On Fri, 11 Nov 2022 at 18:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This allows us to correctly model invalid accesses to the interrupt
> controller as well as avoiding the use of current_cpu hacks to find
> the APIC structure. We have to ensure we check for MSI signals first
> which shouldn't arrive from the CPU but are either triggered by PCI or
> internal IOAPIC writes.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>

> +static MemTxResult apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
> +                                  unsigned int size, MemTxAttrs attrs)
>  {
>      DeviceState *dev;
>      APICCommonState *s;
>      int index = (addr >> 4) & 0xff;
>
>      if (size < 4) {
> -        return;
> +        return MEMTX_ERROR;
>      }
>
> +    /*
> +     * MSI and MMIO APIC are at the same memory location, but actually
> +     * not on the global bus: MSI is on PCI bus APIC is connected
> +     * directly to the CPU.
> +     *
> +     * We can check the MemTxAttrs to check they are coming from where
> +     * we expect. Even though the MSI registers are reserved in APIC
> +     * MMIO and vice versa they shouldn't respond to CPU writes.
> +     */
>      if (addr > 0xfff || !index) {
> -        /* MSI and MMIO APIC are at the same memory location,
> -         * but actually not on the global bus: MSI is on PCI bus
> -         * APIC is connected directly to the CPU.
> -         * Mapping them on the global bus happens to work because
> -         * MSI registers are reserved in APIC MMIO and vice versa. */
> +        switch (attrs.requester_type) {
> +        case MTRT_MACHINE: /* MEMTX_IOPIC */
> +        case MTRT_PCI:     /* PCI signalled MSI */
> +            break;

If we always treat MTRT_MACHINE and MTRT_PCI identically, do we really
need to have different MTRT types for them ?

> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: rejecting write from %d",
> +                          __func__, attrs.requester_id);
> +            return MEMTX_ACCESS_ERROR;
> +        }
>          MSIMessage msi = { .address = addr, .data = val };
>          apic_send_msi(&msi);
> -        return;
> +        return MEMTX_OK;
>      }

thanks
-- PMM