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
