On Wed, Sep 12, 2018 at 01:50:34PM -0500, Brijesh Singh wrote: [...]
> > > + */ > > > + if (sid == X86_IOMMU_SID_INVALID) { > > > + sid = AMDVI_SB_IOAPIC_ID; > > > + } > > > + > > > + amdvi_get_dte(iommu, sid, dte); > > > > Mind to check the return value? > > > > After all it's provided, and we have the fault path to handle it in > > this function. > > > > The amdvi_get_dte(..) does not check the IR bit. It only checks for the > address-translation enabled bit. I can extend the function to check for > IR flag so that we can check the error status of this function. It seems to me that amdvi_get_dte() is checking against things like: general read errors, reserved bits error, and proper set of AMDVI_DEV_VALID. These seem still make sense to IR. > > > > > + > > > + /* interrupt remapping is disabled */ > > > + if (!(dte[2] & AMDVI_IR_REMAP_ENABLE)) { > > > + memcpy(translated, origin, sizeof(*origin)); > > > + goto out; > > > + } > > > + > > > + /* deliver_mode and dest_mode from MSI data */ > > > + dest_mode = (origin->data >> 11) & 1; /* Bit 11 */ > > > + delivery_mode = (origin->data >> 7) & 7; /* Bits 10:8 */ > > > > Nit: The comments are already nice, though IMHO some mask macros would > > be nicer, like AMDVI_IR_PHYS_ADDR_MASK. > > > > Also, could you hint me where are these things defined in the spec? > > I'm looking at Figure 14, but there MSI data bits 15:11 are defined as > > "XXXXXb", and bits 10:8 seems to be part of the interrupt table offset. > > > > Since we are not emulating the Hyper Transport hence we need to look at > the encoding of the delivery mode of MSI data, which is the same as > APIC/IOAPIC delivery mode encoding. See > > * For MSI Data, > https://pdfs.semanticscholar.org/presentation/9420/c279e942eca568157711ef5c92b800c40a79.pdf > (page 5) [1] > * For IOAPIC, https://wiki.osdev.org/APIC > > They are similar to Hyper Transport MT encoding, but not quite the same. > > enum ioapic_irq_destination_types { > dest_Fixed = 0, > dest_LowestPrio = 1, > dest_SMI = 2, > dest__reserved_1 = 3, > dest_NMI = 4, > dest_INIT = 5, > dest__reserved_2 = 6, > dest_ExtINT = 7 > }; > > I will add the comments in the code and also provide the link I just misunderstood since you did this before the switch. Maybe you could consider move this after the switch then since these dest_mode things are not valid for all the cases. Meanwhile, you are refering to general MSI/IOAPIC definitions, then I don't see why dest_mode is bit 11 of MSI data; what I see is that it should be bit 2 of MSI address. That's exactly on the slides you provided [1]. Would you help double confirm? [...] > > > +/* Generic IRQ entry information */ > > > +struct AMDVIIrq { > > > + /* Used by both IOAPIC/MSI interrupt remapping */ > > > + uint8_t trigger_mode; > > > + uint8_t vector; > > > + uint8_t delivery_mode; > > > + uint32_t dest; > > > + uint8_t dest_mode; > > > + > > > + /* only used by MSI interrupt remapping */ > > > + uint8_t redir_hint; > > > + uint8_t msi_addr_last_bits; > > > +}; > > > > This is VTDIrq. > > > > We're having some similar codes between the vt-d and amd-vi ir > > implementations. I'm thinking to what extend we could share the code. > > I don't think it would worth it if we need to spend too much extra > > time, but things like this one might still be good candidates that we > > can share them at the very beginning since it won't be that hard (like > > what you did in patch 1). > > > > (maybe also things like amdvi_generate_msi_message but I'm not sure) > > > > I will see what can be done to move the VTDIrq struct and msi message > generation in common file. That'll be nice. Thank you. -- Peter Xu