Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 29 Oct 2015 06:48:50 -0700
From:      John Baldwin <jhb@freebsd.org>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        "Conrad E. Meyer" <cem@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r290130 - head/sys/dev/ntb/ntb_hw
Message-ID:  <1703515.Ze3qo7rKK1@ralph.baldwin.cx>
In-Reply-To: <20151029075343.GO2257@kib.kiev.ua>
References:  <201510290416.t9T4GSG7044279@repo.freebsd.org> <20151029075343.GO2257@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, October 29, 2015 09:53:43 AM Konstantin Belousov wrote:
> On Thu, Oct 29, 2015 at 04:16:28AM +0000, Conrad E. Meyer wrote:
> > Author: cem
> > Date: Thu Oct 29 04:16:28 2015
> > New Revision: 290130
> > URL: https://svnweb.freebsd.org/changeset/base/290130
> > 
> > Log:
> >   ntb: Do not attempt to set write-combining on MWs
> >   
> >   AMD64 pmap assumes ranges will be in the DMAP, which isn't necessarily
> >   true for NTB memory windows (especially 64-bit BARs).
> I am not sure what do you mean.  pmap_change_attr() handles either DMAP
> or kernel mapped memory.

I think it assumes the DMAP is valid in the nested call here:

                        if (tmpva >= VM_MIN_KERNEL_ADDRESS) {
                                if (pa_start == pa_end) {
                                        /* Start physical address run. */
                                        pa_start = *pdpe & PG_PS_FRAME;
                                        pa_end = pa_start + NBPDP;
                                } else if (pa_end == (*pdpe & PG_PS_FRAME))
                                        pa_end += NBPDP;
                                else {
                                        /* Run ended, update direct map. */
                                        error = pmap_change_attr_locked(
                                            PHYS_TO_DMAP(pa_start),
                                            pa_end - pa_start, mode);
                                        if (error != 0)
                                                break;
                                        /* Start physical address run. */
                                        pa_start = *pdpe & PG_PS_FRAME;
                                        pa_end = pa_start + NBPDP;
                                }
                        }


That needs to do some sort of range checking on the (pa_start, pa_end)
range to only set values that are mapped in the DMAP.

In particular, that pmap_change_attr_locked() will fail here:

        /*
         * Pages that aren't mapped aren't supported.  Also break down 2MB pages
         * into 4KB pages if required.
         */
        for (tmpva = base; tmpva < base + size; ) {
                pdpe = pmap_pdpe(kernel_pmap, tmpva);
                if (*pdpe == 0)
                        return (EINVAL);

Since there won't be a valid pdpe, pde, or pte in the DMAP.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1703515.Ze3qo7rKK1>