Re: [Xen-devel] [v3 04/15] iommu: Add iommu_intpost to control VT-d Posted-Interrupts feature

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Thursday, June 25, 2015 6:16 PM
> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> Cc: Tian, Kevin; keir@xxxxxxx; george.dunlap@xxxxxxxxxxxxx;
> jbeulich@xxxxxxxx; Zhang, Yang Z
> Subject: Re: [Xen-devel] [v3 04/15] iommu: Add iommu_intpost to control VT-d
> Posted-Interrupts feature
> 
> On 25/06/15 10:47, Wu, Feng wrote:
> >
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> Sent: Thursday, June 25, 2015 5:06 PM
> >> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> >> Cc: Tian, Kevin; keir@xxxxxxx; george.dunlap@xxxxxxxxxxxxx;
> >> jbeulich@xxxxxxxx; Zhang, Yang Z
> >> Subject: Re: [Xen-devel] [v3 04/15] iommu: Add iommu_intpost to control
> VT-d
> >> Posted-Interrupts feature
> >>
> >> On 24/06/15 06:18, Feng Wu wrote:
> >>> VT-d Posted-Interrupts is an enhancement to CPU side Posted-Interrupt.
> >>> With VT-d Posted-Interrupts enabled, external interrupts from
> >>> direct-assigned devices can be delivered to guests without VMM
> >>> intervention when guest is running in non-root mode.
> >>>
> >>> This patch adds variable 'iommu_intpost' to control whether enable VT-d
> >>> posted-interrupt or not in the generic IOMMU code.
> >>>
> >>> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> >>> ---
> >>> v3:
> >>> - Remove pointless initializer for 'iommu_intpost'.
> >>> - Some adjustment for "if no intremap then no intpost" logic.
> >>>     * For parse_iommu_param(), move it to the end of the function,
> >>>       so we don't need to add the some logic when introduing the
> >>>       new kernel parameter 'intpost' in later patch.
> >>>     * Add this logic in iommu_setup() after iommu_hardware_setup()
> >>>       is called.
> >>>
> >>>  xen/drivers/passthrough/iommu.c | 10 +++++++++-
> >>>  xen/include/xen/iommu.h         |  2 +-
> >>>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/xen/drivers/passthrough/iommu.c
> >> b/xen/drivers/passthrough/iommu.c
> >>> index 06cb38f..597f676 100644
> >>> --- a/xen/drivers/passthrough/iommu.c
> >>> +++ b/xen/drivers/passthrough/iommu.c
> >>> @@ -39,6 +39,7 @@ static void iommu_dump_p2m_table(unsigned char
> >> key);
> >>>   *   no-snoop                   Disable VT-d Snoop Control
> >>>   *   no-qinval                  Disable VT-d Queued Invalidation
> >>>   *   no-intremap                Disable VT-d Interrupt Remapping
> >>> + *   no-intpost                 Disable VT-d Interrupt posting
> >>>   */
> >>>  custom_param("iommu", parse_iommu_param);
> >>>  bool_t __initdata iommu_enable = 1;
> >>> @@ -51,6 +52,7 @@ bool_t __read_mostly iommu_passthrough;
> >>>  bool_t __read_mostly iommu_snoop = 1;
> >>>  bool_t __read_mostly iommu_qinval = 1;
> >>>  bool_t __read_mostly iommu_intremap = 1;
> >>> +bool_t __read_mostly iommu_intpost;
> >>>  bool_t __read_mostly iommu_hap_pt_share = 1;
> >>>  bool_t __read_mostly iommu_debug;
> >>>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> >>> @@ -112,6 +114,9 @@ static void __init parse_iommu_param(char *s)
> >>>
> >>>          s = ss + 1;
> >>>      } while ( ss );
> >>> +
> >>> +    if ( !iommu_intremap )
> >>> +        iommu_intpost = 0;
> >> This check is redundant - It will unconditionally be performed by
> >> iommu_setup().  I would just drop the hunk.
> >>
> >> However, what you are missing is parsing logic to catch a command line
> >> configuration such as "iommu=intremap,no-intpost"
> > Doesn't the above code cover this command line?
> 
> How would you expect it to? I do not see any strcmp( , "intpost") in
> this patch.

Oh, the command line handle logic is covered in [15/15].

Thanks,
Feng
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel