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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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);
>> >      }
>> >
>> > +    if ( !iommu_intremap )
>> > +        iommu_intpost = 0;
>> 
>> Why is this being repeated here? iommu_setup() is already taking
>> care of this thanks to the earlier patch.
> 
> Here is my original thought, we have two path to get here as below:
> 
> iommu_setup() -> ... -> int_vtd_hw()
> vtd_resume() -> ... -> int_vtd_hw()
> 
> I added the logic here to cover the 'vtd_resume()' case, however,
> after thinking more, seems I don't need to do this, because the
> logic has been done before resume, it should be there after back,
> right?

Obviously any assignment to variables like this one should be benign
(as in not change the variable's value) during resume, or else it'll be
very likely for other stuff to get out of sync and hence broken.

>> > @@ -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.

Jan


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