Re: [Xen-devel] [PATCH v6 04/18] vt-d: VT-d Posted-Interrupts feature detection

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> "Wu, Feng" <feng.wu@xxxxxxxxx> 09/08/15 4:39 AM >>>
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, September 07, 2015 6:43 PM
>> >>> On 06.09.15 at 03:49, <feng.wu@xxxxxxxxx> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> Sent: Friday, September 04, 2015 10:31 PM
>> >> >>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> > @@ -2079,6 +2079,9 @@ static int init_vtd_hw(void)
>> >> >                  disable_intremap(drhd->iommu);
>> >> >      }
>> >> > @@ -2176,6 +2179,14 @@ int __init intel_vtd_setup(void)
>> >> >          if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
>> >> >              iommu_intremap = 0;
>> >> >
>> >> > +        /*
>> >> > +         * We cannot use posted interrupt if X86_FEATURE_CX16 is
>> >> > +         * not supported, since we count on this feature to
>> >> > +         * atomically update 16-byte IRTE in posted format.
>> >> > +         */
>> >> > +        if ( !iommu_intremap || !cap_intr_post(iommu->cap)
>> >> || !cpu_has_cx16 )
>> >> > +            iommu_intpost = 0;
>> >>
>> >> You should check iommu_intpost instead of iommu_intremap to
>> >> match the other checks above here.
>> >
>> > Here I followed your advice:
>> > http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01261.html
>> >
>> > I think the following code should be ok here though, if you like it, I can
>> > change back.
>> >
>> > +        if ( !iommu_intpost || !cap_intr_post(iommu->cap)
>> > +            || !cpu_has_cx16 )
>> > +            iommu_intpost = 0;
>> 
>> The first part is neither in line with the earlier adjustments (as _still_
>> see in the patch context above) not does it make much sense to set
>> a variable to zero when it already is zero.
>
>So, what about this?
>
>+        if ( !cap_intr_post(iommu->cap) || !cpu_has_cx16 )
>+            iommu_intpost = 0;

Yes. While still not in sync with the other checks, this one is actually better 
than
those (which kind of pointlessly check the feature they want to disable).

Jan


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