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

[Peter Xu]

On Thu, Sep 27, 2018 at 12:28:42PM +0000, Singh, Brijesh wrote:
> >> +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.

Thanks for the clarification.  Then I think this is still a good
candidate for error_report_once() otherwise we might face DOS attack
from the guest (or use trace if you prefer).

> >>  /* 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.

For interrupts, I _guess_ we should just drop the interrupt and
report.  For the reporting part, for sure you can still use either
trace or error_report_once() on QEMU side, meanwhile reporting this to
the guest kernel if AMD has such a feature (I didn't dig).

Regards,

-- 
Peter Xu