From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [v8][PATCH 06/16] hvmloader/pci: disable all pci
 devices conflicting with rdm
Date: Thu, 16 Jul 2015 21:21:56 +0800
Message-ID: <55A7AFF4.1040909@intel.com>
References: <1437029582-19564-1-git-send-email-tiejun.chen@intel.com>
	<1437029582-19564-7-git-send-email-tiejun.chen@intel.com>
	<CAFLBxZZhDHi2XQ9UYkt6gfEiVMNRGxxer3zNptFGVsns2OYOfw@mail.gmail.com>
	<55A79AFA.3040500@intel.com>
	<CAFLBxZY86xDsNKO9zHMouk+sEPuCcYaR_49TQ87zjXxtBPqKnQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAFLBxZY86xDsNKO9zHMouk+sEPuCcYaR_49TQ87zjXxtBPqKnQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Jan Beulich <jbeulich@suse.com>, Wei Liu <wei.liu2@citrix.com>
List-Id: xen-devel@lists.xenproject.org

>> Here what I intended to do is if one of all bars specific to one given
>> device already conflicts with RDM, its not necessary to continue check other
>> remaining bars of this device and other RDM regions, we just disable this
>> device simply then check next device.
>
> I know what you're trying to do; what I'm saying is I don't think it
> does what you want it to do.
>
> You have loops nested 3 deep:
> 1. for each dev
>    2.  for each bar
>      3. for each memory range
>
> This conditional is in loop 3; you want it to be in loop 2.
>
> (In fact, when you set is_conflict, you then break out of loop 3 back
> into loop 2; so this code will never actually be run.)

Sorry I should make this clear last time.

I mean I already knew what you were saying is right at this point so I 
tried to use goto to fix this bug.

>
>   >> Also, if you declare is_conflict inside the devfn loop, rather than in
>>> the main function, then you don't need this "is_conflict=false" here.
>>>
>>> It might also be more sensible to use a goto instead; but this is one
>>
>>

[snip]

> I'm not a fan of hard-coding the loop continuing condition like this;
> if I were going to do a goto, I'd want to go to the end of the loop.
>

I guess something like this,

			...
                         pci_writew(devfn, PCI_COMMAND, ~cmd);
                         /* Jump next device. */
                         goto check_next_device;
                     }
                 }
             }
         }
  check_next_device:
     }
}

> Anyway, the code is OK as it is; I'd rather spend time working on
> something that's more of a blocker.
>

Thanks
Tiejun