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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/06/15 10:47, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Thursday, June 25, 2015 5:06 PM
>> To: Wu, Feng; xen-devel@lists.xen.org
>> Cc: Tian, Kevin; keir@xen.org; george.dunlap@eu.citrix.com;
>> jbeulich@suse.com; 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@intel.com>
>>> ---
>>> 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.

~Andrew