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

[Singh, Brijesh]

On 9/25/18 1:36 AM, Peter Xu wrote:
> 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.

Yes, this should *never* happen with Linux.

>
>> +        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?

I will see what I can do to not "passthrough" messages in error case.
There are only two error cases here:

1) if reserved bits are set in DTE
2) if guest OS has enabled IR when intremap=off in amd-iommu.

Other than the above two cases everything should be pass through.
 
>
>>          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,
>