From: Leonardo Bras <leobras.c@gmail.com>
To: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Thiago Jung Bauermann <bauerman@linux.ibm.com>,
Ram Pai <linuxram@us.ibm.com>,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows
Date: Mon, 22 Jun 2020 23:14:51 -0300 [thread overview]
Message-ID: <c331742c9f7a3e3ccead5d9db99a66d3f268b95f.camel@gmail.com> (raw)
In-Reply-To: <4176ea2b-c778-2f59-ba57-7339b873ead5@ozlabs.ru>
On Tue, 2020-06-23 at 11:12 +1000, Alexey Kardashevskiy wrote:
[snip]
> > > > +static int query_ddw_out_sz(struct device_node *par_dn)
> > >
> > > Can easily be folded into query_ddw().
> >
> > Sure, but it will get inlined by the compiler, and I think it reads
> > better this way.
> > I mean, I understand you have a reason to think it's better to fold it
> > in query_ddw(), and I would like to better understand that to improve
> > my code in the future.
>
> You have numbers 5 and 6 (the number of parameters) twice in the file,
> this is why I brought it up. query_ddw_out_sz() can potentially return
> something else than 5 or 6 and you will have to change the callsite(s)
> then, since these are not macros, this allows to think there may be more
> places with 5 and 6. Dunno. A single function will simplify things imho.
That's a good point. Thanks!
>
>
> > > > +{
> > > > + int ret;
> > > > + u32 ddw_ext[3];
> > > > +
> > > > + ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
> > > > + &ddw_ext[0], 3);
> > > > + if (ret || ddw_ext[0] < 2 || ddw_ext[2] != 1)
> > >
> > > Oh that PAPR thing again :-/
> > >
> > > ===
> > > The “ibm,ddw-extensions” property value is a list of integers the first
> > > integer indicates the number of extensions implemented and subsequent
> > > integers, one per extension, provide a value associated with that
> > > extension.
> > > ===
> > >
> > > So ddw_ext[0] is length.
> > > Listindex==2 is for "reset" says PAPR and
> > > Listindex==3 is for this new 64bit "largest_available_block".
> > >
> > > So I'd expect ddw_ext[2] to have the "reset" token and ddw_ext[3] to
> > > have "1" for this new feature but indexes are smaller. I am confused.
> > > Either way these "2" and "3" needs to be defined in macros, "0" probably
> > > too.
> >
> > Remember these indexes are not C-like 0-starting indexes, where the
> > size would be Listindex==1.
>
> Yeah I can see that is the assumption but out of curiosity - is it
> written anywhere? Across PAPR, they index bytes from 1 but bits from 0 :-/
From LoPAR:
The “ibm,ddw-extensions” property value is a list of integers the first
integer indicates the number of extensions implemented and subsequent
integers, one per extension, provide a value associated with that
extension.
And the list/table then shows extensions from 2 on onwards:
List index 2 : Token of the ibm,reset-pe-dma-windows RTAS Call
(...)
> Either way make them macros.
>
>
> > Basically, in C-like array it's :
> > a[0] == size,
> > a[1] == reset_token,
> > a[2] == new 64bit "largest_available_block"
> >
> > > Please post 'lsprop "ibm,ddw-extensions"' here. Thanks,
> >
> > Sure:
> > [root@host pci@800000029004005]# lsprop "ibm,ddw-extensions"
> > ibm,dd
> > w-extensions
> > 00000002 00000056 00000000
>
> Right, good. Thanks,
Thank you for reviewing!
>
> >
> > > > + return 5;
> > > > + return 6;
> > > > +}
> > > > +
> > > > static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
> > > > - struct ddw_query_response *query)
> > > > + struct ddw_query_response *query,
> > > > + struct device_node *par_dn)
> > > > {
> > > > struct device_node *dn;
> > > > struct pci_dn *pdn;
> > > > - u32 cfg_addr;
> > > > + u32 cfg_addr, query_out[5];
> > > > u64 buid;
> > > > - int ret;
> > > > + int ret, out_sz;
> > > >
> > > > /*
> > > > * Get the config address and phb buid of the PE window.
> > > > @@ -888,12 +906,29 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
> > > > pdn = PCI_DN(dn);
> > > > buid = pdn->phb->buid;
> > > > cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
> > > > + out_sz = query_ddw_out_sz(par_dn);
> > > > +
> > > > + ret = rtas_call(ddw_avail[0], 3, out_sz, query_out,
> > > > + cfg_addr, BUID_HI(buid), BUID_LO(buid));
> > > > + dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x returned %d\n",
> > > > + ddw_avail[0], cfg_addr, BUID_HI(buid), BUID_LO(buid), ret);
> > > > +
> > > > + switch (out_sz) {
> > > > + case 5:
> > > > + query->windows_available = query_out[0];
> > > > + query->largest_available_block = query_out[1];
> > > > + query->page_size = query_out[2];
> > > > + query->migration_capable = query_out[3];
> > > > + break;
> > > > + case 6:
> > > > + query->windows_available = query_out[0];
> > > > + query->largest_available_block = ((u64)query_out[1] << 32) |
> > > > + query_out[2];
> > > > + query->page_size = query_out[3];
> > > > + query->migration_capable = query_out[4];
> > > > + break;
> > > > + }
> > > >
> > > > - ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
> > > > - cfg_addr, BUID_HI(buid), BUID_LO(buid));
> > > > - dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
> > > > - " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
> > > > - BUID_LO(buid), ret);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -1040,7 +1075,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > * of page sizes: supported and supported for migrate-dma.
> > > > */
> > > > dn = pci_device_to_OF_node(dev);
> > > > - ret = query_ddw(dev, ddw_avail, &query);
> > > > + ret = query_ddw(dev, ddw_avail, &query, pdn);
> > > > if (ret != 0)
> > > > goto out_failed;
> > > >
> > > > @@ -1068,7 +1103,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > /* check largest block * page size > max memory hotplug addr */
> > > > max_addr = ddw_memory_hotplug_max();
> > > > if (query.largest_available_block < (max_addr >> page_shift)) {
> > > > - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %u "
> > > > + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
> > > > "%llu-sized pages\n", max_addr, query.largest_available_block,
> > > > 1ULL << page_shift);
> > > > goto out_failed;
> > > >
> >
> > Best regards,
> > Leonardo
> >
Best regards,
Leonardo Bras
next prev parent reply other threads:[~2020-06-23 2:15 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 5:06 [PATCH 0/4] Remove default DMA window before creating DDW Leonardo Bras
2020-06-19 5:06 ` [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows Leonardo Bras
2020-06-22 10:02 ` Alexey Kardashevskiy
2020-06-22 18:58 ` Leonardo Bras
2020-06-23 1:12 ` Alexey Kardashevskiy
2020-06-23 2:14 ` Leonardo Bras [this message]
2020-06-23 2:29 ` Alexey Kardashevskiy
2020-06-19 5:06 ` [PATCH 2/4] powerpc/pseries/iommu: Implement ibm,reset-pe-dma-windows rtas call Leonardo Bras
2020-06-22 10:02 ` Alexey Kardashevskiy
2020-06-22 18:58 ` Leonardo Bras
2020-06-23 1:11 ` Alexey Kardashevskiy
2020-06-23 2:20 ` Leonardo Bras
2020-06-19 5:06 ` [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window Leonardo Bras
2020-06-22 10:02 ` Alexey Kardashevskiy
2020-06-22 18:59 ` Leonardo Bras
2020-06-23 1:12 ` Alexey Kardashevskiy
2020-06-23 1:33 ` Oliver O'Halloran
2020-06-23 2:26 ` Leonardo Bras
2020-06-23 2:22 ` Leonardo Bras
2020-06-19 5:06 ` [PATCH 4/4] powerpc/pseries/iommu: Remove default DMA window before creating DDW Leonardo Bras
2020-06-20 6:13 ` kernel test robot
2020-06-22 10:02 ` Alexey Kardashevskiy
2020-06-22 18:59 ` Leonardo Bras
2020-06-23 1:11 ` Alexey Kardashevskiy
2020-06-23 2:31 ` Leonardo Bras
2020-06-23 2:35 ` Alexey Kardashevskiy
2020-06-23 2:43 ` Leonardo Bras
2020-06-23 3:52 ` Alexey Kardashevskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c331742c9f7a3e3ccead5d9db99a66d3f268b95f.camel@gmail.com \
--to=leobras.c@gmail.com \
--cc=aik@ozlabs.ru \
--cc=bauerman@linux.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=linuxram@us.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).