Re: [Qemu-devel] [PATCH v3 6/9] x86_iommu/amd: Add interrupt remap support when VAPIC is not enabled

[Peter Xu]

On Fri, Sep 21, 2018 at 02:25:40PM +0000, Singh, Brijesh wrote:
> Emulate the interrupt remapping support when guest virtual APIC is
> not enabled.
> 
> For more info Refer: AMD IOMMU spec Rev 3.0 - section 2.2.5.1
> 
> When VAPIC is not enabled, it uses interrupt remapping as defined in
> Table 20 and Figure 15 from IOMMU spec.
> 
> Signed-off-by: Brijesh Singh <address@hidden>
> Cc: Peter Xu <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Richard Henderson <address@hidden>
> Cc: Eduardo Habkost <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Cc: Tom Lendacky <address@hidden>
> Cc: Suravee Suthikulpanit <address@hidden>
> ---

[...]

> +static bool amdvi_validate_int_remap(AMDVIState *s, uint64_t *dte)
> +{
> +    /* Check if IR is enabled in DTE */
> +    if (!(dte[2] & AMDVI_IR_REMAP_ENABLE)) {
> +        return false;
> +    }
> +
> +    /* validate that we are configure with intremap=on */
> +    if (!X86_IOMMU_DEVICE(s)->intr_supported) {
> +        error_report("Interrupt remapping is enabled in the guest but "
> +                     "not in the host. Use intremap=on to enable interrupt "
> +                     "remapping in amd-iommu.");

Just to make sure: we should never get this with Linux, right?  Since
Linux should try to detect IOAPIC device first before enabling IR.

> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  /* Interrupt remapping for MSI/MSI-X entry */
>  static int amdvi_int_remap_msi(AMDVIState *iommu,
>                                 MSIMessage *origin,
>                                 MSIMessage *translated,
>                                 uint16_t sid)
>  {
> +    int ret = 0;
> +    uint64_t pass = 0;
> +    uint64_t dte[4] = { 0 };
> +    X86IOMMUIrq irq = { 0 };
> +    uint8_t dest_mode, delivery_mode;
> +
>      assert(origin && translated);
>  
> +    /*
> +     * When IOMMU is enabled, interrupt remap request will come either from
> +     * IO-APIC or PCI device. If interrupt is from PCI device then it will
> +     * have a valid requester id but if the interrupt is from IO-APIC
> +     * then requester id will be invalid.
> +     */
> +    if (sid == X86_IOMMU_SID_INVALID) {
> +        sid = AMDVI_IOAPIC_SB_DEVID;
> +    }
> +
>      trace_amdvi_ir_remap_msi_req(origin->address, origin->data, sid);
>  
> -    if (!iommu || !X86_IOMMU_DEVICE(iommu)->intr_supported) {
> +    /* verify that interrupt remapping is enabled before going further. */
> +    if (!iommu ||
> +        !amdvi_get_dte(iommu, sid, dte)  ||
> +        !amdvi_validate_int_remap(iommu, dte)) {

I'll have similar question as I left in patch 3 - IMHO we should have
three paths rather than two here:

- IR enabled
- passthrough
- error

The "error" path seems missing, and we're treating all the error path
as "passthrough" as well.  Is this really what you want?

>          memcpy(translated, origin, sizeof(*origin));
>          goto out;
>      }
> @@ -1055,10 +1183,81 @@ static int amdvi_int_remap_msi(AMDVIState *iommu,
>          return -AMDVI_IR_ERR;
>      }
>  
> +    /*
> +     * The MSI data register [10:8] are used to get the upstream interrupt 
> type.
> +     *
> +     * amd-iommu device does not emulate the HyperTransport hence use the
> +     * IO-APIC encoding definition (IO-APIC spec 3.2.4) instead of the

Nit: IMHO referencing to IOAPIC spec here might still be confusing to
readers.  IOAPIC spec chap 3.2.4 defines layout of IO redirection
table entries, and IMHO it has nothing to do with MSI, so it cannot
explain well on why you are using origin->data (this is DATA of a MSI
message).  Maybe you'd mention the MSI document somewhere either in
PCI spec or Intel/AMD spec (MSI layout is defined all over the
places...).

Regards,

-- 
Peter Xu