On Fri, Oct 12, 2012 at 05:49:30PM +0800, Xiao Guangrong wrote: > On 10/11/2012 10:31 PM, Marcelo Tosatti wrote: > > On Thu, Oct 11, 2012 at 09:06:12PM +0800, Xiao Guangrong wrote: > >> On 10/10/2012 11:11 PM, Marcelo Tosatti wrote: > >> > >>> > >>> Why does is_error_pfn() return true for mmio spte? Its not an "error", > >>> after all. > >>> > >>> Please kill is_invalid_pfn and use > >>> > >>> -> is_error_pfn for checking for errors (mmio spte is not an error pfn, > >>> its a special pfn) > >>> > >>> -> add explicit is_noslot_pfn checks where necessary in the code > >>> (say to avoid interpreting a noslot_pfn's pfn "address" bits). > >>> > >>> (should have noticed this earlier, sorry). > >> > >> Never mind, your comments are always appreciated! ;) > >> > >> Marcelo, is it good to you? > >> (will post it after your check and full test) > > > > Yes, this works (please check the validity of each case in addition to > > testing, haven't done it). > > > > Also add a oneline comment on top of each > > is_error_pfn,is_noslot_pfn,is_error_noslot_pfn > > > > /* is_noslot_pfn: userspace translation valid but no memory slot */ > > /* is_error_pfn: ... */ > > > > etc. > > > > Marcelo, i think this fix should be backport and your idea can be a > separate patchset. Yes? The current invalid/is_error/noslot_pfn separation is confusing, leading to one immediate bug and IMO more future bugs. The proposed patch you sent is quite small, why is it troublesome to backport? (and i am just asking one line of comment, summing to 3 total of lines of comments). Can't see the advantage of a special easily backportable fix? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
