Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 7 Nov 2019 19:02:27 +0100 (CET)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Ingo Molnar <mingo@kernel.org>
cc:     LKML <linux-kernel@vger.kernel.org>, x86@kernel.org,
        Stephen Hemminger <stephen@networkplumber.org>,
        Willy Tarreau <w@1wt.eu>, Juergen Gross <jgross@suse.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage
 further
In-Reply-To: <20191107081635.GE30739@gmail.com>
Message-ID: <alpine.DEB.2.21.1911071856420.27903@nanos.tec.linutronix.de>
References: <20191106193459.581614484@linutronix.de> <20191106202806.241007755@linutronix.de> <20191107081635.GE30739@gmail.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 7 Nov 2019, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> This might seem like a small detail, but since we do the range tracking 
> and copying at byte granularity anyway, why not do the zero range search 
> at byte granularity as well?
> 
> I bet it's faster and simpler as well than the bit-searching.

Not necessarily. The bitmap search uses unsigned long internally at least
to the point where it finds a zero bit.

But probably we should just ditch that optimization and rather have the
sequence number to figure out whether something needs to be copied at all.

> > +	if (first >= IO_BITMAP_BITS) {
> > +		/*
> > +		 * If the resulting bitmap has all permissions dropped, clear
> > +		 * TIF_IO_BITMAP and set the IO bitmap offset in the TSS to
> > +		 * invalid. Deallocate both the new and the thread's bitmap.
> > +		 */
> > +		clear_thread_flag(TIF_IO_BITMAP);
> > +		tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_INVALID;
> > +		tofree = bitmap;
> > +		bitmap = NULL;
> 
> BTW., wouldn't it be simpler to just say that if a thread uses IO ops 
> even once, it gets a bitmap and that's it? I.e. we could further simplify 
> this seldom used piece of code.

Maybe.
 
> > +	} else {
> >  		/*
> > +		 * I/O bitmap contains zero bits. Set TIF_IO_BITMAP, make
> > +		 * the bitmap offset valid and make sure that the TSS limit
> > +		 * is correct. It might have been wreckaged by a VMEXiT.
> >  		 */
> > +		set_thread_flag(TIF_IO_BITMAP);
> > +		tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> >  		refresh_tss_limit();
> >  	}
> 
> I'm wondering, shouldn't we call refresh_tss_limit() in both branches, or 
> is a VMEXIT-wreckaged TSS limit harmless if we otherwise have 
> io_bitmap_base set to IO_BITMAP_OFFSET_INVALID?

Yes. because the VMEXIT wreckage restricts TSS limit to 0x67 which is the
end of the hardware tss struct. As the invalid offset points in any case
outside the TSS limit it does not matter. It always #GP's when an I/O
access happens in user space.

Thanks,

	tglx