* RE: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC
From: Xiaowei Bao @ 2019-08-28 7:22 UTC (permalink / raw)
To: Andrew Murray
Cc: mark.rutland@arm.com, Roy Zang, lorenzo.pieralisi@arm.co,
arnd@arndb.de, devicetree@vger.kernel.org,
gregkh@linuxfoundation.org, linuxppc-dev@lists.ozlabs.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kishon@ti.com, M.h. Lian, robh+dt@kernel.org,
gustavo.pimentel@synopsys.com, jingoohan1@gmail.com,
bhelgaas@google.com, Leo Li, shawnguo@kernel.org, Mingkai Hu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190827131048.GK14582@e119886-lin.cambridge.arm.com>
> -----Original Message-----
> From: Andrew Murray <andrew.murray@arm.com>
> Sent: 2019年8月27日 21:11
> To: Xiaowei Bao <xiaowei.bao@nxp.com>
> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h.
> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support
> for DWC
>
> On Fri, Aug 23, 2019 at 11:50:20PM +0000, Xiaowei Bao wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Murray <andrew.murray@arm.com>
> > > Sent: 2019年8月23日 21:25
> > > To: Xiaowei Bao <xiaowei.bao@nxp.com>
> > > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> > > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> > > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org;
> M.h.
> > > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> > > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> > > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs
> > > support for DWC
> > >
> > > On Thu, Aug 22, 2019 at 07:22:33PM +0800, Xiaowei Bao wrote:
> > > > Add multiple PFs support for DWC, different PF have different
> > > > config space we use pf-offset property which get from the DTS to
> > > > access the different pF config space.
> > >
> > > It looks like you're missing a --cover-letter again.
> > >
> > > >
> > > > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> > > > ---
> > > > v2:
> > > > - Remove duplicate redundant code.
> > > > - Reimplement the PF config space access way.
> > > >
> > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 122
> > > ++++++++++++++++--------
> > > > drivers/pci/controller/dwc/pcie-designware.c | 59
> ++++++++----
> > > > drivers/pci/controller/dwc/pcie-designware.h | 11 ++-
> > > > 3 files changed, 134 insertions(+), 58 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 2bf5a35..3e2b740 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -19,12 +19,17 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep
> *ep)
> > > > pci_epc_linkup(epc);
> > > > }
> > > >
> > > > -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum
> > > > pci_barno
> > > bar,
> > > > - int flags)
> > > > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > > > + enum pci_barno bar, int flags)
> > > > {
> > > > u32 reg;
> > > > + unsigned int func_offset = 0;
> > > > + struct dw_pcie_ep *ep = &pci->ep;
> > > >
> > > > - reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > > +
> > > > + reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> > >
> > > This pattern of checking if func_conf_select exists and using it to
> > > get an offset is repeated a lot throughout this file. You could move
> > > this functionality into a new function (similar to dw_pcie_read_dbi
> > > etc). Or perhaps a new variant of dw_pcie_writel_ should be created that
> writes takes a func_no argument.
> >
> > Thanks for your comments, I thought about this method before, but
> > there is a issue about the method of access the different func config
> > space, due to our platform use this method that different func have
> > different offset from dbi_base to access the different config space,
> > but others platform maybe use the way that write a register to
> > implement different func config space access, so I think reserve a
> > callback function
>
> My point here was really to move out duplicated code to its own small
> function.
> I wasn't making any comment about (removing) the callback, just that the test
> and callback could be in one function.
>
> > to different platform to implement the own method, my point is that,
> > if use register method they can implement the code in this function
> > and return offset is 0, if use offset method, they can return the offset value
> which can be use by dw_pcie_ep driver.
>
> By the way, I haven't looked to see how many of the dw_pcie_write_xxx
> functions would benefit from a func_no argument - if there were many calls to
> dw_pcie_write_xxx that all used a reg value originated from func_conf_select
> then an approach similar to the implementation of dw_pcie_write_dbi could
> probably be justified (i.e. rather than change the value of reg) for writing to
> functions.
I think you mean that move the if (ep->ops->func_conf_select)
func_offset = ep->ops->func_conf_select(ep, func_no); to a new function,
I will modify it in next version patch, thanks a lot.
Thanks
Xiaowei
>
> >
> > >
> > >
> > > > dw_pcie_dbi_ro_wr_en(pci);
> > > > dw_pcie_writel_dbi2(pci, reg, 0x0);
> > > > dw_pcie_writel_dbi(pci, reg, 0x0);
> > >
> > >
> > > > @@ -235,7 +257,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc
> > > *epc, u8 func_no,
> > > > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > >
> > > > - ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
> > > > + ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr,
> > > > +size);
> > > > if (ret) {
> > > > dev_err(pci->dev, "Failed to enable address\n");
> > > > return ret;
> > > > @@ -249,11 +271,15 @@ static int dw_pcie_ep_get_msi(struct pci_epc
> > > *epc, u8 func_no)
> > > > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > u32 val, reg;
> > > > + unsigned int func_offset = 0;
> > > > +
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > >
> > > > if (!ep->msi_cap)
> > > > return -EINVAL;
> > > >
> > > > - reg = ep->msi_cap + PCI_MSI_FLAGS;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS;
> > >
> > > This makes me nervous.
> > >
> > > From a PCI viewpoint, each function has it's own capability
> > > structure and within each function there may exist a MSI capability.
> > > Yet what we're doing here is using dw_pcie_ep_find_capability to get
> > > the list of capabilities for function 0, and then applying offsets from that for
> subsequent functions. I.e.
> > > we're applying DW specific knowledge to find the correct capability,
> > > rather than following the general PCI approach.
> > >
> > > I think the above hunk shouldn't be required - but instead
> > > dw_pcie_ep_find_capability is updated to take a func_no parameter.
> > >
> > > Have I understood this correctly?
> >
> > Yes, this is a issue, I think the different func maybe have different
> > capability, but the dw_pcie_ep_find_capability function is called by
> > dw_pcie_ep_init function, we can't add func_no parameter to
> > dw_pcie_ep_find_capability,
>
> Why not?
>
> Given that 'struct dw_pcie' represents a controller - and thus potentially
> multiple functions - then the _find_capability function should be able to
> provide the correct offset for the given function. Surely it needs to know
> which function number? Unless there is some reason why we can assume that
> all functions share the same capability offset.
>
> Also the 'struct dw_pcie_ep' which represents an endpoint controller - this
> has msi_cap and msix_cap fields - given this may be a multifunction device
> what do these fields actually refer to?
>
> Perhaps Jungoo/Gustavo can comment.
I have two method to fix this issue, define the msi_cap to *msi_cap, msi_cap[0]
indicate func0, msi_cap[1] indicate func1..., like this:
+ for (func_no = 0; func_no < epc->max_functions; func_no++) {
+ ep->msi_cap[func_no] =
+ dw_pcie_ep_find_capability(pci, func_no,
+ PCI_CAP_ID_MSI);
+ ep->msix_cap[func_no] =
+ dw_pcie_ep_find_capability(pci, func_no,
+ PCI_CAP_ID_MSIX);
+ }
But in Layerscape EP driver, we can't set the msi_capable of pci_epc_features struct
by ep->msix_cap, this is not correct, unless we assume if msi_cap[0] is 1, all function
will support the MSI feature. and we can return error from the get_msi or set_msi
function. I think this is the simplest way in current EP framework.
Another method is that add a callback function in pci_epc_ops, don't use the pci_epc_features
mode, but that's a big change, we need to implement the other platform callback function
like rickchip and so on.
Thanks
Xiaowei
>
>
> > I will try to fix it use a new patch, I think move this function to
> > ep_init callback function If better, thanks.
> >
> >
> > >
> > > > val = dw_pcie_readw_dbi(pci, reg);
> > > > if (!(val & PCI_MSI_FLAGS_ENABLE))
> > > > return -EINVAL;
> > > > @@ -268,11 +294,15 @@ static int dw_pcie_ep_set_msi(struct pci_epc
> > > *epc, u8 func_no, u8 interrupts)
> > > > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > u32 val, reg;
> > > > + unsigned int func_offset = 0;
> > > > +
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > >
> > > > if (!ep->msi_cap)
> > > > return -EINVAL;
> > > >
> > > > - reg = ep->msi_cap + PCI_MSI_FLAGS;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS;
> > > > val = dw_pcie_readw_dbi(pci, reg);
> > > > val &= ~PCI_MSI_FLAGS_QMASK;
> > > > val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK; @@ -288,11
> > > > +318,15 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8
> func_no)
> > > > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > u32 val, reg;
> > > > + unsigned int func_offset = 0;
> > > > +
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > >
> > > > if (!ep->msix_cap)
> > > > return -EINVAL;
> > > >
> > > > - reg = ep->msix_cap + PCI_MSIX_FLAGS;
> > > > + reg = ep->msix_cap + func_offset + PCI_MSIX_FLAGS;
> > >
> > > Same for MSIX.
> >
> > Yes.
> >
> > >
> > > > val = dw_pcie_readw_dbi(pci, reg);
> > > > if (!(val & PCI_MSIX_FLAGS_ENABLE))
> > > > return -EINVAL;
> > > > @@ -307,11 +341,15 @@ static int dw_pcie_ep_set_msix(struct
> > > > pci_epc
> > > *epc, u8 func_no, u16 interrupts)
> > > > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > u32 val, reg;
> > > > + unsigned int func_offset = 0;
> > > > +
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > >
> > > > if (!ep->msix_cap)
> > > > return -EINVAL;
> > > >
> > > > - reg = ep->msix_cap + PCI_MSIX_FLAGS;
> > > > + reg = ep->msix_cap + func_offset + PCI_MSIX_FLAGS;
> > > > val = dw_pcie_readw_dbi(pci, reg);
> > > > val &= ~PCI_MSIX_FLAGS_QSIZE;
> > > > val |= interrupts;
> > > > @@ -398,29 +436,33 @@ int dw_pcie_ep_raise_msi_irq(struct
> > > dw_pcie_ep *ep, u8 func_no,
> > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > struct pci_epc *epc = ep->epc;
> > > > unsigned int aligned_offset;
> > > > + unsigned int func_offset = 0;
> > > > u16 msg_ctrl, msg_data;
> > > > u32 msg_addr_lower, msg_addr_upper, reg;
> > > > u64 msg_addr;
> > > > bool has_upper;
> > > > int ret;
> > > >
> > > > + if (ep->ops->func_conf_select)
> > > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > > +
> > >
> > > You could probably move this hunk below the test for msi_cap to save
> > > some cycles.
> >
> > Sorry, I didn't understand the means, please explain it detailly,
> > thanks a lot, ^_^
>
> If you insert the call to func_conf_select *after* the test for !msi_cap below -
> then in the case where msi_cap is NULL then you will save some CPU cycles
> by not bothering to call func_conf_select.
Got it, thanks a lot. ^_^
Thanks
Xiaowei
>
>
> > >
> > > > if (!ep->msi_cap)
> > > > return -EINVAL;
> > > >
> > > > /* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1.
> */
> > > > - reg = ep->msi_cap + PCI_MSI_FLAGS;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS;
> > > > msg_ctrl = dw_pcie_readw_dbi(pci, reg);
> > > > has_upper = !!(msg_ctrl & PCI_MSI_FLAGS_64BIT);
> > > > - reg = ep->msi_cap + PCI_MSI_ADDRESS_LO;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_ADDRESS_LO;
> > > > msg_addr_lower = dw_pcie_readl_dbi(pci, reg);
> > > > if (has_upper) {
> > > > - reg = ep->msi_cap + PCI_MSI_ADDRESS_HI;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_ADDRESS_HI;
> > > > msg_addr_upper = dw_pcie_readl_dbi(pci, reg);
> > > > - reg = ep->msi_cap + PCI_MSI_DATA_64;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_DATA_64;
> > > > msg_data = dw_pcie_readw_dbi(pci, reg);
> > > > } else {
> > > > msg_addr_upper = 0;
> > > > - reg = ep->msi_cap + PCI_MSI_DATA_32;
> > > > + reg = ep->msi_cap + func_offset + PCI_MSI_DATA_32;
> > > > msg_data = dw_pcie_readw_dbi(pci, reg);
> > > > }
> > > > aligned_offset = msg_addr_lower & (epc->mem->page_size - 1);
> > >
> > >
> > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 7d25102..305e73d 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -158,9 +158,10 @@ static void dw_pcie_writel_ob_unroll(struct
> > > dw_pcie *pci, u32 index, u32 reg,
> > > > dw_pcie_writel_atu(pci, offset + reg, val); }
> > > >
> > > > -static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci,
> > > > int
> > > index,
> > > > - int type, u64 cpu_addr,
> > > > - u64 pci_addr, u32 size)
> > > > +static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci,
> > > > +u8
> > > func_no,
> > > > + int index, int type,
> > > > + u64 cpu_addr, u64 pci_addr,
> > > > + u32 size)
> > > > {
> > > > u32 retries, val;
> > > >
> > > > @@ -175,7 +176,7 @@ static void
> > > dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> > > > dw_pcie_writel_ob_unroll(pci, index,
> PCIE_ATU_UNR_UPPER_TARGET,
> > > > upper_32_bits(pci_addr));
> > > > dw_pcie_writel_ob_unroll(pci, index,
> PCIE_ATU_UNR_REGION_CTRL1,
> > > > - type);
> > > > + type | PCIE_ATU_FUNC_NUM(func_no));
> > >
> > > Much better :)
> >
> > Do you mean that use the expression "a? b:c"
> >
> > >
> > > > dw_pcie_writel_ob_unroll(pci, index,
> PCIE_ATU_UNR_REGION_CTRL2,
> > > > PCIE_ATU_ENABLE);
> > > >
> > > > @@ -194,8 +195,9 @@ static void
> > > dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
> > > > dev_err(pci->dev, "Outbound iATU is not being enabled\n"); }
> > > >
> > > > -void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int
> type,
> > > > - u64 cpu_addr, u64 pci_addr, u32 size)
> > > > +static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8
> > > func_no,
> > > > + int index, int type, u64 cpu_addr,
> > > > + u64 pci_addr, u32 size)
> > > > {
> > > > u32 retries, val;
> > > >
> > > > @@ -203,8 +205,8 @@ void dw_pcie_prog_outbound_atu(struct
> dw_pcie
> > > *pci, int index, int type,
> > > > cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > > >
> > > > if (pci->iatu_unroll_enabled) {
> > > > - dw_pcie_prog_outbound_atu_unroll(pci, index, type, cpu_addr,
> > > > - pci_addr, size);
> > > > + dw_pcie_prog_outbound_atu_unroll(pci, func_no, index, type,
> > > > + cpu_addr, pci_addr, size);
> > > > return;
> > > > }
> > > >
> > >
> > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index ffed084..a0fdbf7 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -71,9 +71,11 @@
> > > > #define PCIE_ATU_TYPE_IO 0x2
> > > > #define PCIE_ATU_TYPE_CFG0 0x4
> > > > #define PCIE_ATU_TYPE_CFG1 0x5
> > > > +#define PCIE_ATU_FUNC_NUM(pf) (pf << 20)
> > >
> > > "Macro argument 'pf' may be better as '(pf)' to avoid precedence issues"
> > >
> > > > #define PCIE_ATU_CR2 0x908
> > > > #define PCIE_ATU_ENABLE BIT(31)
> > > > #define PCIE_ATU_BAR_MODE_ENABLE BIT(30)
> > > > +#define PCIE_ATU_FUNC_NUM_MATCH_EN BIT(19)
> > > > #define PCIE_ATU_LOWER_BASE 0x90C
> > > > #define PCIE_ATU_UPPER_BASE 0x910
> > > > #define PCIE_ATU_LIMIT 0x914
> > > > @@ -197,6 +199,7 @@ struct dw_pcie_ep_ops {
> > > > int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
> > > > enum pci_epc_irq_type type, u16 interrupt_num);
> > > > const struct pci_epc_features* (*get_features)(struct dw_pcie_ep
> > > > *ep);
> > > > + unsigned int (*func_conf_select)(struct dw_pcie_ep *ep, u8
> > > > +func_no);
> > >
> > > Given that this function will return an offset, I'm not sure the
> > > name you have is suitable. Something like get_pf_offset or similar is more
> descriptive.
> >
> > As above explain, my initial view is that this function can return 0
> > or offset depends on the platform implement mechanism, so I named it
> > func_conf_select, I think add a comment for this function, like this:
> > /*
> > * provide a method to implement the method of different func config
> > space access,
> > * if use offset method, return the offset from dbi_base, if your
> > register method, implement
> > * the code in this callback function and return 0.
> > */
> > How about it?
>
> This means that func_conf_select can never (easily) indicate an error to the
> caller as this would change the offset. Where func_conf_select doesn't
> change the offset there probably isn't much else it can do instead (unless it
> was responsible for doing the write as well). So I'm not sure how well this
> approach works.
We can use int type of this function and return a negative value when a error
occurred, that is to say:
if(func_no && !pcie->drvdata->func_offset)
return -1;
but we need to set a flag to differentiate the method of config space access, that
is we can return -1 in func_offset method, another method we only return 0.
Thanks
Xiaowei
>
> Thanks,
>
> Andrew Murray
>
> >
> > >
> > > Thanks,
> > >
> > > Andrew Murray
> > >
> > > > };
> > > >
> > > > struct dw_pcie_ep {
> > > > @@ -265,8 +268,12 @@ int dw_pcie_wait_for_link(struct dw_pcie
> > > > *pci); void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
> > > > int type, u64 cpu_addr, u64 pci_addr,
> > > > u32 size);
> > > > -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
> > > > - u64 cpu_addr, enum dw_pcie_as_type as_type);
> > > > +void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8
> > > > +func_no, int
> > > index,
> > > > + int type, u64 cpu_addr, u64 pci_addr,
> > > > + u32 size);
> > > > +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int
> index,
> > > > + int bar, u64 cpu_addr,
> > > > + enum dw_pcie_as_type as_type);
> > > > void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
> > > > enum dw_pcie_region_type type); void
> dw_pcie_setup(struct
> > > > dw_pcie *pci);
> > > > --
> > > > 2.9.5
> > > >
^ permalink raw reply
* [DOC][PATCH v2] powerpc: Provide initial documentation for PAPR hcalls
From: Vaibhav Jain @ 2019-08-28 8:27 UTC (permalink / raw)
To: linuxppc-dev
Cc: msuchanek, Nicholas Piggin, Oliver O'Halloran,
Aneesh Kumar K . V, Vaibhav Jain, Laurent Dufour, David Gibson
This doc patch provides an initial description of the hcall op-codes
that are used by Linux kernel running as a guest (LPAR) on top of
PowerVM or any other sPAPR compliant hyper-visor (e.g qemu).
Apart from documenting the hcalls the doc-patch also provides a
rudimentary overview of how hcall ABI, how they are issued with the
Linux kernel and how information/control flows between the guest and
hypervisor.
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log:
v2:
* Added a section on Register conventions to be followed when invoking
hcalls. [Nick]
* Updated section on HCALL ABI to clarify that byte ordering only
matters to in/out values passed by a memory buffer. [Laurent]
* Updated a code comment in 'exceptions-64s.S' describing hypercall
register conventions to point it to 'papr_hcalls.rst' [Nick]
v1:
Initial version of this doc-patch was posted and reviewed as part of
the patch-series "[PATCH v5 0/4] powerpc/papr_scm: Workaround for
failure of drc bind after kexec"
https://patchwork.ozlabs.org/patch/1136022/. Changes introduced on top
the original patch:
* Replaced the of term PHYP with Hypervisor to indicate both
PowerVM/Qemu [Laurent]
* Emphasized that In/Out arguments to hcalls are in Big-endian format
[Laurent]
* Fixed minor word repetition, spell issues and grammatical error
[Michal, Mpe]
* Replaced various variant of term 'hcall' with a single
variant. [Mpe]
* Changed the documentation format from txt to ReST. [Mpe]
* Changed the name of documentation file to papr_hcalls.rst. [Mpe]
* Updated the section describing privileged operation by hypervisor
to be more accurate [Mpe].
* Fixed up mention of register notation used for describing
hcalls. [Mpe]
* s/NVDimm/NVDIMM [Mpe]
* Added section on return values from hcall [Mpe]
* Described H_CONTINUE return-value for long running hcalls.
---
Documentation/powerpc/papr_hcalls.rst | 248 ++++++++++++++++++++++++++
arch/powerpc/kernel/exceptions-64s.S | 19 +-
2 files changed, 251 insertions(+), 16 deletions(-)
create mode 100644 Documentation/powerpc/papr_hcalls.rst
diff --git a/Documentation/powerpc/papr_hcalls.rst b/Documentation/powerpc/papr_hcalls.rst
new file mode 100644
index 000000000000..4d270c7ae26d
--- /dev/null
+++ b/Documentation/powerpc/papr_hcalls.rst
@@ -0,0 +1,248 @@
+===========================
+Hypercall Op-codes (hcalls)
+===========================
+
+Overview
+=========
+
+Virtualization on 64-bit Power Book3S Platforms is based on the PAPR
+specification [1]_ which describes the run-time environment for a guest
+operating system and how it should interact with the hypervisor for
+privileged operations. Currently there are two PAPR compliant hypervisors:
+
+- **IBM PowerVM (PHYP)**: IBM's proprietary hypervisor that supports AIX,
+ IBM-i and Linux as supported guests (termed as Logical Partitions
+ or LPARS). It supports the full PAPR specification.
+
+- **Qemu/KVM**: Supports PPC64 linux guests running on a PPC64 linux host.
+ Though it only implements a subset of PAPR specification called LoPAPR [2]_.
+
+On PPC64 arch a guest kernel running on top of a PAPR hypervisor is called
+a *pSeries guest*. A pseries guest runs in a supervisor mode (HV=0) and must
+issue hypercalls to the hypervisor whenever it needs to perform an action
+that is hypervisor priviledged [3]_ or for other services managed by the
+hypervisor.
+
+Hence a Hypercall (hcall) is essentially a request by the pseries guest
+asking hypervisor to perform a privileged operation on behalf of the guest. The
+guest issues a with necessary input operands. The hypervisor after performing
+the privilege operation returns a status code and output operands back to the
+guest.
+
+HCALL ABI
+=========
+The ABI specification for a hcall between a pseries guest and PAPR hypervisor
+is covered in section 14.5.3 of ref [2]_. Switch to the Hypervisor context is
+done via the instruction **HVCS** that expects the Opcode for hcall is set in *r3*
+and any in-arguments for the hcall are provided in registers *r4-r12*. If values
+have to be passed through a memory buffer, the data stored in that buffer should be
+in Big-endian byte order.
+
+Once control is returns back to the guest after hypervisor has serviced the
+'HVCS' instruction the return value of the hcall is available in *r3* and any
+out values are returned in registers *r4-r12*. Again like in case of in-arguments,
+any out values stored in a memory buffer will be in Big-endian byte order.
+
+Powerpc arch code provides convenient wrappers named **plpar_hcall_xxx** defined
+in a arch specific header [4]_ to issue hcalls from the linux kernel
+running as pseries guest.
+
+Register Conventions
+====================
+
+Any hcall should follow same register convention as described in section 2.2.1.1
+of "64-Bit ELF V2 ABI Specification: Power Architecture"[5]_. Table below
+summarizes these conventions:
+
++----------+----------+-------------------------------------------+
+| Register |Volatile | Purpose |
+| Range |(Y/N) | |
++==========+==========+===========================================+
+| r0 | Y | Optional-usage |
++----------+----------+-------------------------------------------+
+| r1 | N | Stack Pointer |
++----------+----------+-------------------------------------------+
+| r2 | N | TOC |
++----------+----------+-------------------------------------------+
+| r3 | Y | hcall opcode/return value |
++----------+----------+-------------------------------------------+
+| r4-r10 | Y | in and out values |
++----------+----------+-------------------------------------------+
+| r11 | Y | Optional-usage/Environmental pointer |
++----------+----------+-------------------------------------------+
+| r12 | Y | Optional-usage/Function entry address at |
+| | | global entry point |
++----------+----------+-------------------------------------------+
+| r13 | N | Thread-Pointer |
++----------+----------+-------------------------------------------+
+| r14-r31 | N | Local Variables |
++----------+----------+-------------------------------------------+
+| LR | Y | Link Register |
++----------+----------+-------------------------------------------+
+| CTR | Y | Loop Counter |
++----------+----------+-------------------------------------------+
+| XER | Y | Fixed-point exception register. |
++----------+----------+-------------------------------------------+
+| CR0-1 | Y | Condition register fields. |
++----------+----------+-------------------------------------------+
+| CR2-4 | N | Condition register fields. |
++----------+----------+-------------------------------------------+
+| CR5-7 | Y | Condition register fields. |
++----------+----------+-------------------------------------------+
+| Others | N | |
++----------+----------+-------------------------------------------+
+
+DRC & DRC Indexes
+=================
+::
+
+ DR1 Guest
+ +--+ +------------+ +---------+
+ | | <----> | | | User |
+ +--+ DRC1 | | DRC | Space |
+ | PAPR | Index +---------+
+ DR2 | Hypervisor | | |
+ +--+ | | <-----> | Kernel |
+ | | <----> | | Hcall | |
+ +--+ DRC2 +------------+ +---------+
+
+PAPR hypervisor terms shared hardware resources like PCI devices, NVDIMMs etc
+available for use by LPARs as Dynamic Resource (DR). When a DR is allocated to
+an LPAR, PHYP creates a data-structure called Dynamic Resource Connector (DRC)
+to manage LPAR access. An LPAR refers to a DRC via an opaque 32-bit number
+called DRC-Index. The DRC-index value is provided to the LPAR via device-tree
+where its present as an attribute in the device tree node associated with the
+DR.
+
+HCALL Return-values
+===================
+
+After servicing the hcall, hypervisor sets the return-value in *r3* indicating
+success or failure of the hcall. In case of a failure an error code indicates
+the cause for error. These codes are defined and documented in arch specific
+header [4]_.
+
+In some cases a hcall can potentially take a long time and need to be issued
+multiple times in order to be completely serviced. These hcalls will usually
+accept an opaque value *continue-token* within there argument list and a
+return value of *H_CONTINUE* indicates that hypervisor hasn't still finished
+servicing the hcall yet.
+
+To make such hcalls the guest need to set *continue-token == 0* for the
+initial call and use the hypervisor returned value of *continue-token*
+for each subsequent hcall until hypervisor returns a non *H_CONTINUE*
+return value.
+
+HCALL Op-codes
+==============
+
+Below is a partial list of HCALLs that are supported by PHYP. For the
+corresponding opcode values please look into the arch specific header [4]_:
+
+**H_SCM_READ_METADATA**
+
+| Input: *drcIndex, offset, buffer-address, numBytesToRead*
+| Out: *numBytesRead*
+| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_Hardware*
+
+Given a DRC Index of an NVDIMM, read N-bytes from the the metadata area
+associated with it, at a specified offset and copy it to provided buffer.
+The metadata area stores configuration information such as label information,
+bad-blocks etc. The metadata area is located out-of-band of NVDIMM storage
+area hence a separate access semantics is provided.
+
+**H_SCM_WRITE_METADATA**
+
+| Input: *drcIndex, offset, data, numBytesToWrite*
+| Out: *None*
+| Return Value: *H_Success, H_Parameter, H_P2, H_P4, H_Hardware*
+
+Given a DRC Index of an NVDIMM, write N-bytes to the metadata area
+associated with it, at the specified offset and from the provided buffer.
+
+**H_SCM_BIND_MEM**
+
+| Input: *drcIndex, startingScmBlockIndex, numScmBlocksToBind,*
+| *targetLogicalMemoryAddress, continue-token*
+| Out: *continue-token, targetLogicalMemoryAddress, numScmBlocksToBound*
+| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_P4, H_Overlap,*
+| *H_Too_Big, H_P5, H_Busy*
+
+Given a DRC-Index of an NVDIMM, map a continuous SCM blocks range
+*(startingScmBlockIndex, startingScmBlockIndex+numScmBlocksToBind)* to the guest
+at *targetLogicalMemoryAddress* within guest physical address space. In
+case *targetLogicalMemoryAddress == 0xFFFFFFFF_FFFFFFFF* then hypervisor
+assigns a target address to the guest. The HCALL can fail if the Guest has
+an active PTE entry to the SCM block being bound.
+
+**H_SCM_UNBIND_MEM**
+| Input: drcIndex, startingScmLogicalMemoryAddress, numScmBlocksToUnbind
+| Out: numScmBlocksUnbound
+| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_In_Use, H_Overlap,*
+| *H_Busy, H_LongBusyOrder1mSec, H_LongBusyOrder10mSec*
+
+Given a DRC-Index of an NVDimm, unmap *numScmBlocksToUnbind* SCM blocks starting
+at *startingScmLogicalMemoryAddress* from guest physical address space. The
+HCALL can fail if the Guest has an active PTE entry to the SCM block being
+unbound.
+
+**H_SCM_QUERY_BLOCK_MEM_BINDING**
+
+| Input: *drcIndex, scmBlockIndex*
+| Out: *Guest-Physical-Address*
+| Return Value: *H_Success, H_Parameter, H_P2, H_NotFound*
+
+Given a DRC-Index and an SCM Block index return the guest physical address to
+which the SCM block is mapped to.
+
+**H_SCM_QUERY_LOGICAL_MEM_BINDING**
+
+| Input: *Guest-Physical-Address*
+| Out: *drcIndex, scmBlockIndex*
+| Return Value: *H_Success, H_Parameter, H_P2, H_NotFound*
+
+Given a guest physical address return which DRC Index and SCM block is mapped
+to that address.
+
+**H_SCM_UNBIND_ALL**
+
+| Input: *scmTargetScope, drcIndex*
+| Out: *None*
+| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_In_Use, H_Busy,*
+| *H_LongBusyOrder1mSec, H_LongBusyOrder10mSec*
+
+Depending on the Target scope unmap all SCM blocks belonging to all NVDIMMs
+or all SCM blocks belonging to a single NVDIMM identified by its drcIndex
+from the LPAR memory.
+
+**H_SCM_HEALTH**
+
+| Input: drcIndex
+| Out: *health-bitmap, health-bit-valid-bitmap*
+| Return Value: *H_Success, H_Parameter, H_Hardware*
+
+Given a DRC Index return the info on predictive failure and overall health of
+the NVDIMM. The asserted bits in the health-bitmap indicate a single predictive
+failure and health-bit-valid-bitmap indicate which bits in health-bitmap are
+valid.
+
+**H_SCM_PERFORMANCE_STATS**
+
+| Input: drcIndex, resultBuffer Addr
+| Out: None
+| Return Value: *H_Success, H_Parameter, H_Unsupported, H_Hardware, H_Authority, H_Privilege*
+
+Given a DRC Index collect the performance statistics for NVDIMM and copy them
+to the resultBuffer.
+
+References
+==========
+.. [1] "Power Architecture Platform Reference"
+ https://en.wikipedia.org/wiki/Power_Architecture_Platform_Reference
+.. [2] "Linux on Power Architecture Platform Reference"
+ https://members.openpowerfoundation.org/document/dl/469
+.. [3] "Definitions and Notation" Book III-Section 14.5.3
+ https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
+.. [4] arch/powerpc/include/asm/hvcall.h
+.. [5] "64-Bit ELF V2 ABI Specification: Power Architecture"
+ https://openpowerfoundation.org/?resource_lib=64-bit-elf-v2-abi-specification-power-architecture
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 6ba3cc2ef8ab..efe5d8b13a2e 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1531,22 +1531,9 @@ EXC_COMMON(trap_0b_common, 0xb00, unknown_exception)
*
* Call convention:
*
- * syscall register convention is in Documentation/powerpc/syscall64-abi.rst
- *
- * For hypercalls, the register convention is as follows:
- * r0 volatile
- * r1-2 nonvolatile
- * r3 volatile parameter and return value for status
- * r4-r10 volatile input and output value
- * r11 volatile hypercall number and output value
- * r12 volatile input and output value
- * r13-r31 nonvolatile
- * LR nonvolatile
- * CTR volatile
- * XER volatile
- * CR0-1 CR5-7 volatile
- * CR2-4 nonvolatile
- * Other registers nonvolatile
+ * syscall and hypercalls register conventions are documented in
+ * Documentation/powerpc/syscall64-abi.rst and
+ * Documentation/powerpc/papr_hcalls.rst respectively.
*
* The intersection of volatile registers that don't contain possible
* inputs is: cr0, xer, ctr. We may use these as scratch regs upon entry
--
2.21.0
^ permalink raw reply related
* Re: [DOC][PATCH v2] powerpc: Provide initial documentation for PAPR hcalls
From: Laurent Dufour @ 2019-08-28 9:00 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Aneesh Kumar K . V, Nicholas Piggin, Oliver O'Halloran,
msuchanek, David Gibson
In-Reply-To: <20190828082729.16695-1-vaibhav@linux.ibm.com>
Le 28/08/2019 à 10:27, Vaibhav Jain a écrit :
> This doc patch provides an initial description of the hcall op-codes
> that are used by Linux kernel running as a guest (LPAR) on top of
> PowerVM or any other sPAPR compliant hyper-visor (e.g qemu).
>
> Apart from documenting the hcalls the doc-patch also provides a
> rudimentary overview of how hcall ABI, how they are issued with the
> Linux kernel and how information/control flows between the guest and
> hypervisor.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
FWIW,
Reviewed-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> Change-log:
>
> v2:
> * Added a section on Register conventions to be followed when invoking
> hcalls. [Nick]
> * Updated section on HCALL ABI to clarify that byte ordering only
> matters to in/out values passed by a memory buffer. [Laurent]
> * Updated a code comment in 'exceptions-64s.S' describing hypercall
> register conventions to point it to 'papr_hcalls.rst' [Nick]
>
> v1:
> Initial version of this doc-patch was posted and reviewed as part of
> the patch-series "[PATCH v5 0/4] powerpc/papr_scm: Workaround for
> failure of drc bind after kexec"
> https://patchwork.ozlabs.org/patch/1136022/. Changes introduced on top
> the original patch:
>
> * Replaced the of term PHYP with Hypervisor to indicate both
> PowerVM/Qemu [Laurent]
> * Emphasized that In/Out arguments to hcalls are in Big-endian format
> [Laurent]
> * Fixed minor word repetition, spell issues and grammatical error
> [Michal, Mpe]
> * Replaced various variant of term 'hcall' with a single
> variant. [Mpe]
> * Changed the documentation format from txt to ReST. [Mpe]
> * Changed the name of documentation file to papr_hcalls.rst. [Mpe]
> * Updated the section describing privileged operation by hypervisor
> to be more accurate [Mpe].
> * Fixed up mention of register notation used for describing
> hcalls. [Mpe]
> * s/NVDimm/NVDIMM [Mpe]
> * Added section on return values from hcall [Mpe]
> * Described H_CONTINUE return-value for long running hcalls.
> ---
> Documentation/powerpc/papr_hcalls.rst | 248 ++++++++++++++++++++++++++
> arch/powerpc/kernel/exceptions-64s.S | 19 +-
> 2 files changed, 251 insertions(+), 16 deletions(-)
> create mode 100644 Documentation/powerpc/papr_hcalls.rst
>
> diff --git a/Documentation/powerpc/papr_hcalls.rst b/Documentation/powerpc/papr_hcalls.rst
> new file mode 100644
> index 000000000000..4d270c7ae26d
> --- /dev/null
> +++ b/Documentation/powerpc/papr_hcalls.rst
> @@ -0,0 +1,248 @@
> +===========================
> +Hypercall Op-codes (hcalls)
> +===========================
> +
> +Overview
> +=========
> +
> +Virtualization on 64-bit Power Book3S Platforms is based on the PAPR
> +specification [1]_ which describes the run-time environment for a guest
> +operating system and how it should interact with the hypervisor for
> +privileged operations. Currently there are two PAPR compliant hypervisors:
> +
> +- **IBM PowerVM (PHYP)**: IBM's proprietary hypervisor that supports AIX,
> + IBM-i and Linux as supported guests (termed as Logical Partitions
> + or LPARS). It supports the full PAPR specification.
> +
> +- **Qemu/KVM**: Supports PPC64 linux guests running on a PPC64 linux host.
> + Though it only implements a subset of PAPR specification called LoPAPR [2]_.
> +
> +On PPC64 arch a guest kernel running on top of a PAPR hypervisor is called
> +a *pSeries guest*. A pseries guest runs in a supervisor mode (HV=0) and must
> +issue hypercalls to the hypervisor whenever it needs to perform an action
> +that is hypervisor priviledged [3]_ or for other services managed by the
> +hypervisor.
> +
> +Hence a Hypercall (hcall) is essentially a request by the pseries guest
> +asking hypervisor to perform a privileged operation on behalf of the guest. The
> +guest issues a with necessary input operands. The hypervisor after performing
> +the privilege operation returns a status code and output operands back to the
> +guest.
> +
> +HCALL ABI
> +=========
> +The ABI specification for a hcall between a pseries guest and PAPR hypervisor
> +is covered in section 14.5.3 of ref [2]_. Switch to the Hypervisor context is
> +done via the instruction **HVCS** that expects the Opcode for hcall is set in *r3*
> +and any in-arguments for the hcall are provided in registers *r4-r12*. If values
> +have to be passed through a memory buffer, the data stored in that buffer should be
> +in Big-endian byte order.
> +
> +Once control is returns back to the guest after hypervisor has serviced the
> +'HVCS' instruction the return value of the hcall is available in *r3* and any
> +out values are returned in registers *r4-r12*. Again like in case of in-arguments,
> +any out values stored in a memory buffer will be in Big-endian byte order.
> +
> +Powerpc arch code provides convenient wrappers named **plpar_hcall_xxx** defined
> +in a arch specific header [4]_ to issue hcalls from the linux kernel
> +running as pseries guest.
> +
> +Register Conventions
> +====================
> +
> +Any hcall should follow same register convention as described in section 2.2.1.1
> +of "64-Bit ELF V2 ABI Specification: Power Architecture"[5]_. Table below
> +summarizes these conventions:
> +
> ++----------+----------+-------------------------------------------+
> +| Register |Volatile | Purpose |
> +| Range |(Y/N) | |
> ++==========+==========+===========================================+
> +| r0 | Y | Optional-usage |
> ++----------+----------+-------------------------------------------+
> +| r1 | N | Stack Pointer |
> ++----------+----------+-------------------------------------------+
> +| r2 | N | TOC |
> ++----------+----------+-------------------------------------------+
> +| r3 | Y | hcall opcode/return value |
> ++----------+----------+-------------------------------------------+
> +| r4-r10 | Y | in and out values |
> ++----------+----------+-------------------------------------------+
> +| r11 | Y | Optional-usage/Environmental pointer |
> ++----------+----------+-------------------------------------------+
> +| r12 | Y | Optional-usage/Function entry address at |
> +| | | global entry point |
> ++----------+----------+-------------------------------------------+
> +| r13 | N | Thread-Pointer |
> ++----------+----------+-------------------------------------------+
> +| r14-r31 | N | Local Variables |
> ++----------+----------+-------------------------------------------+
> +| LR | Y | Link Register |
> ++----------+----------+-------------------------------------------+
> +| CTR | Y | Loop Counter |
> ++----------+----------+-------------------------------------------+
> +| XER | Y | Fixed-point exception register. |
> ++----------+----------+-------------------------------------------+
> +| CR0-1 | Y | Condition register fields. |
> ++----------+----------+-------------------------------------------+
> +| CR2-4 | N | Condition register fields. |
> ++----------+----------+-------------------------------------------+
> +| CR5-7 | Y | Condition register fields. |
> ++----------+----------+-------------------------------------------+
> +| Others | N | |
> ++----------+----------+-------------------------------------------+
> +
> +DRC & DRC Indexes
> +=================
> +::
> +
> + DR1 Guest
> + +--+ +------------+ +---------+
> + | | <----> | | | User |
> + +--+ DRC1 | | DRC | Space |
> + | PAPR | Index +---------+
> + DR2 | Hypervisor | | |
> + +--+ | | <-----> | Kernel |
> + | | <----> | | Hcall | |
> + +--+ DRC2 +------------+ +---------+
> +
> +PAPR hypervisor terms shared hardware resources like PCI devices, NVDIMMs etc
> +available for use by LPARs as Dynamic Resource (DR). When a DR is allocated to
> +an LPAR, PHYP creates a data-structure called Dynamic Resource Connector (DRC)
> +to manage LPAR access. An LPAR refers to a DRC via an opaque 32-bit number
> +called DRC-Index. The DRC-index value is provided to the LPAR via device-tree
> +where its present as an attribute in the device tree node associated with the
> +DR.
> +
> +HCALL Return-values
> +===================
> +
> +After servicing the hcall, hypervisor sets the return-value in *r3* indicating
> +success or failure of the hcall. In case of a failure an error code indicates
> +the cause for error. These codes are defined and documented in arch specific
> +header [4]_.
> +
> +In some cases a hcall can potentially take a long time and need to be issued
> +multiple times in order to be completely serviced. These hcalls will usually
> +accept an opaque value *continue-token* within there argument list and a
> +return value of *H_CONTINUE* indicates that hypervisor hasn't still finished
> +servicing the hcall yet.
> +
> +To make such hcalls the guest need to set *continue-token == 0* for the
> +initial call and use the hypervisor returned value of *continue-token*
> +for each subsequent hcall until hypervisor returns a non *H_CONTINUE*
> +return value.
> +
> +HCALL Op-codes
> +==============
> +
> +Below is a partial list of HCALLs that are supported by PHYP. For the
> +corresponding opcode values please look into the arch specific header [4]_:
> +
> +**H_SCM_READ_METADATA**
> +
> +| Input: *drcIndex, offset, buffer-address, numBytesToRead*
> +| Out: *numBytesRead*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_Hardware*
> +
> +Given a DRC Index of an NVDIMM, read N-bytes from the the metadata area
> +associated with it, at a specified offset and copy it to provided buffer.
> +The metadata area stores configuration information such as label information,
> +bad-blocks etc. The metadata area is located out-of-band of NVDIMM storage
> +area hence a separate access semantics is provided.
> +
> +**H_SCM_WRITE_METADATA**
> +
> +| Input: *drcIndex, offset, data, numBytesToWrite*
> +| Out: *None*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_P4, H_Hardware*
> +
> +Given a DRC Index of an NVDIMM, write N-bytes to the metadata area
> +associated with it, at the specified offset and from the provided buffer.
> +
> +**H_SCM_BIND_MEM**
> +
> +| Input: *drcIndex, startingScmBlockIndex, numScmBlocksToBind,*
> +| *targetLogicalMemoryAddress, continue-token*
> +| Out: *continue-token, targetLogicalMemoryAddress, numScmBlocksToBound*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_P4, H_Overlap,*
> +| *H_Too_Big, H_P5, H_Busy*
> +
> +Given a DRC-Index of an NVDIMM, map a continuous SCM blocks range
> +*(startingScmBlockIndex, startingScmBlockIndex+numScmBlocksToBind)* to the guest
> +at *targetLogicalMemoryAddress* within guest physical address space. In
> +case *targetLogicalMemoryAddress == 0xFFFFFFFF_FFFFFFFF* then hypervisor
> +assigns a target address to the guest. The HCALL can fail if the Guest has
> +an active PTE entry to the SCM block being bound.
> +
> +**H_SCM_UNBIND_MEM**
> +| Input: drcIndex, startingScmLogicalMemoryAddress, numScmBlocksToUnbind
> +| Out: numScmBlocksUnbound
> +| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_In_Use, H_Overlap,*
> +| *H_Busy, H_LongBusyOrder1mSec, H_LongBusyOrder10mSec*
> +
> +Given a DRC-Index of an NVDimm, unmap *numScmBlocksToUnbind* SCM blocks starting
> +at *startingScmLogicalMemoryAddress* from guest physical address space. The
> +HCALL can fail if the Guest has an active PTE entry to the SCM block being
> +unbound.
> +
> +**H_SCM_QUERY_BLOCK_MEM_BINDING**
> +
> +| Input: *drcIndex, scmBlockIndex*
> +| Out: *Guest-Physical-Address*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_NotFound*
> +
> +Given a DRC-Index and an SCM Block index return the guest physical address to
> +which the SCM block is mapped to.
> +
> +**H_SCM_QUERY_LOGICAL_MEM_BINDING**
> +
> +| Input: *Guest-Physical-Address*
> +| Out: *drcIndex, scmBlockIndex*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_NotFound*
> +
> +Given a guest physical address return which DRC Index and SCM block is mapped
> +to that address.
> +
> +**H_SCM_UNBIND_ALL**
> +
> +| Input: *scmTargetScope, drcIndex*
> +| Out: *None*
> +| Return Value: *H_Success, H_Parameter, H_P2, H_P3, H_In_Use, H_Busy,*
> +| *H_LongBusyOrder1mSec, H_LongBusyOrder10mSec*
> +
> +Depending on the Target scope unmap all SCM blocks belonging to all NVDIMMs
> +or all SCM blocks belonging to a single NVDIMM identified by its drcIndex
> +from the LPAR memory.
> +
> +**H_SCM_HEALTH**
> +
> +| Input: drcIndex
> +| Out: *health-bitmap, health-bit-valid-bitmap*
> +| Return Value: *H_Success, H_Parameter, H_Hardware*
> +
> +Given a DRC Index return the info on predictive failure and overall health of
> +the NVDIMM. The asserted bits in the health-bitmap indicate a single predictive
> +failure and health-bit-valid-bitmap indicate which bits in health-bitmap are
> +valid.
> +
> +**H_SCM_PERFORMANCE_STATS**
> +
> +| Input: drcIndex, resultBuffer Addr
> +| Out: None
> +| Return Value: *H_Success, H_Parameter, H_Unsupported, H_Hardware, H_Authority, H_Privilege*
> +
> +Given a DRC Index collect the performance statistics for NVDIMM and copy them
> +to the resultBuffer.
> +
> +References
> +==========
> +.. [1] "Power Architecture Platform Reference"
> + https://en.wikipedia.org/wiki/Power_Architecture_Platform_Reference
> +.. [2] "Linux on Power Architecture Platform Reference"
> + https://members.openpowerfoundation.org/document/dl/469
> +.. [3] "Definitions and Notation" Book III-Section 14.5.3
> + https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
> +.. [4] arch/powerpc/include/asm/hvcall.h
> +.. [5] "64-Bit ELF V2 ABI Specification: Power Architecture"
> + https://openpowerfoundation.org/?resource_lib=64-bit-elf-v2-abi-specification-power-architecture
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 6ba3cc2ef8ab..efe5d8b13a2e 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1531,22 +1531,9 @@ EXC_COMMON(trap_0b_common, 0xb00, unknown_exception)
> *
> * Call convention:
> *
> - * syscall register convention is in Documentation/powerpc/syscall64-abi.rst
> - *
> - * For hypercalls, the register convention is as follows:
> - * r0 volatile
> - * r1-2 nonvolatile
> - * r3 volatile parameter and return value for status
> - * r4-r10 volatile input and output value
> - * r11 volatile hypercall number and output value
> - * r12 volatile input and output value
> - * r13-r31 nonvolatile
> - * LR nonvolatile
> - * CTR volatile
> - * XER volatile
> - * CR0-1 CR5-7 volatile
> - * CR2-4 nonvolatile
> - * Other registers nonvolatile
> + * syscall and hypercalls register conventions are documented in
> + * Documentation/powerpc/syscall64-abi.rst and
> + * Documentation/powerpc/papr_hcalls.rst respectively.
> *
> * The intersection of volatile registers that don't contain possible
> * inputs is: cr0, xer, ctr. We may use these as scratch regs upon entry
>
^ permalink raw reply
* Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a
From: Andrew Murray @ 2019-08-28 9:01 UTC (permalink / raw)
To: Xiaowei Bao
Cc: mark.rutland@arm.com, Roy Zang, lorenzo.pieralisi@arm.co,
arnd@arndb.de, devicetree@vger.kernel.org,
gregkh@linuxfoundation.org, linuxppc-dev@lists.ozlabs.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kishon@ti.com, M.h. Lian, robh+dt@kernel.org,
gustavo.pimentel@synopsys.com, jingoohan1@gmail.com,
bhelgaas@google.com, Leo Li, shawnguo@kernel.org, Mingkai Hu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB3310F78C86F775BB1F5B7E0CF5A30@VI1PR04MB3310.eurprd04.prod.outlook.com>
On Wed, Aug 28, 2019 at 04:29:32AM +0000, Xiaowei Bao wrote:
>
>
> > -----Original Message-----
> > From: Andrew Murray <andrew.murray@arm.com>
> > Sent: 2019年8月27日 21:34
> > To: Xiaowei Bao <xiaowei.bao@nxp.com>
> > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h.
> > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> > ls1088a and ls2088a
> >
> > On Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Andrew Murray <andrew.murray@arm.com>
> > > > Sent: 2019年8月23日 22:28
> > > > To: Xiaowei Bao <xiaowei.bao@nxp.com>
> > > > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> > > > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> > > > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org;
> > M.h.
> > > > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> > > > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> > > > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> > > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > > > for ls1088a and ls2088a
> > > >
> > > > On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > > > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > > > > difference between LS1 and LS2 platform, so refactor the code of
> > > > > the EP driver.
> > > > >
> > > > > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> > > > > ---
> > > > > v2:
> > > > > - New mechanism for layerscape EP driver.
> > > >
> > > > Was there a v1 of this patch?
> > > >
> > > > >
> > > > > drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > > > ++++++++++++++++++++------
> > > > > 1 file changed, 58 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > index 7ca5fe8..2a66f07 100644
> > > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > @@ -20,27 +20,29 @@
> > > > >
> > > > > #define PCIE_DBI2_OFFSET 0x1000 /* DBI2 base address*/
> > > > >
> > > > > -struct ls_pcie_ep {
> > > > > - struct dw_pcie *pci;
> > > > > - struct pci_epc_features *ls_epc;
> > > > > +#define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
> > > > > +
> > > > > +struct ls_pcie_ep_drvdata {
> > > > > + u32 func_offset;
> > > > > + const struct dw_pcie_ep_ops *ops;
> > > > > + const struct dw_pcie_ops *dw_pcie_ops;
> > > > > };
> > > > >
> > > > > -#define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
> > > > > +struct ls_pcie_ep {
> > > > > + struct dw_pcie *pci;
> > > > > + struct pci_epc_features *ls_epc;
> > > > > + const struct ls_pcie_ep_drvdata *drvdata; };
> > > > >
> > > > > static int ls_pcie_establish_link(struct dw_pcie *pci) {
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > > > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > > > > .start_link = ls_pcie_establish_link, };
> > > > >
> > > > > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > - { .compatible = "fsl,ls-pcie-ep",},
> > > > > - { },
> > > > > -};
> > > > > -
> > > > > static const struct pci_epc_features*
> > > > > ls_pcie_ep_get_features(struct dw_pcie_ep *ep) { @@ -82,10 +84,44
> > > > > @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > > > > }
> > > > > }
> > > > >
> > > > > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> > *ep,
> > > > > + u8 func_no)
> > > > > +{
> > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > + struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > > > + u8 header_type;
> > > > > +
> > > > > + header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > > > +
> > > > > + if (header_type & (1 << 7))
> > > > > + return pcie->drvdata->func_offset * func_no;
> > > > > + else
> > > > > + return 0;
> > > >
> > > > It looks like there isn't a PCI define for multi function, the
> > > > nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > > > hotplug/ibmphp.h. A comment above the test might be helpful to explain
> > the test.
> > >
> > > OK, I will add a comment above this code.
> > >
> > > >
> > > > As the ls_pcie_ep_drvdata structures are static, the unset
> > > > .func_offset will be initialised to 0, so you could just drop the test above.
> > >
> > > Due to the different PCIe controller have different property, e.g.
> > > PCIe controller1 support multiple function feature, but PCIe
> > > controller2 don't support this feature, so I need to check which
> > > controller support it and return the correct offset value, but each board only
> > have one ls_pcie_ep_drvdata, ^_^.
> >
> > Yes but if they don't support the feature then func_offset will be 0.
> >
> > >
> > > >
> > > > However something to the effect of the following may help spot
> > > > misconfiguration:
> > > >
> > > > WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > > > pcie->drvdata->func_offset * func_no;
> > > >
> > > > The WARN is probably quite useful as if you are attempting to use
> > > > non-zero functions and func_offset isn't set - then things may
> > > > appear to work normally but actually will break horribly.
> > >
> > > As discussion before, I think the func_offset should not depends on
> > > the function number, even if other platforms of NXP may be use write
> > > registers way to access the different function config space.
> >
> > I agree that func_offset is an optional parameter. But if you are attempting to
> > determine the offset of a function and you are given a non-zero function
> > number - then something has gone wrong if func_offset is 0.
>
> I have understood you means, maybe I need to set a flag in the driver_data struct,
> because I may add other platform of NXP, these platform use the write register
> method to access different function, e.g.
> write func_num to register, then we can access this func_num config space.
>
> I will modify the code like this? Do you have better advice?
> Case1:
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index 004a7e8..8a0d6df 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -23,6 +23,7 @@
> #define to_ls_pcie_ep(x) dev_get_drvdata((x)->dev)
>
> struct ls_pcie_ep_drvdata {
> + u8 func_config_flag;
> u32 func_offset;
> const struct dw_pcie_ep_ops *ops;
> const struct dw_pcie_ops *dw_pcie_ops;
> @@ -97,8 +98,14 @@ static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> * Read the Header Type register of config space to check
> * whether this PCI device support the multiple function.
> */
> - if (header_type & (1 << 7))
> - return pcie->drvdata->func_offset * func_no;
> + if (header_type & (1 << 7)) {
> + if (pcie->drvdata->func_config_flag) {
> + iowrite32((func_num << n), pci->dbi_base + PCI_XXXX_XXX);
> + } else {
> + WARN_ON(func_no && !pcie->drvdata->func_offset);
> + return pcie->drvdata->func_offset * func_no;
> + }
> + }
>
> return 0;
> }
>
> Of course, I don't need to set the flag this time, because I don't use the second method(write
> register method), so the code like this:
> case2:
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> u8 func_no) {
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> u8 header_type;
>
> of course, this code is not requied, due to the
> pcie->drvdata->func_offset is 0, but I think this is more clear
> if use this code.
> header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
>
> /*
> * Read the Header Type register of config space to check
> * whether this PCI device support the multiple function.
> */
> if (header_type & (1 << 7)) {
> WARN_ON(func_no && !pcie->drvdata->func_offset);
> return pcie->drvdata->func_offset * func_no;
> }
>
> return 0;
> }
>
> Or like this:
> Case3:
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> u8 func_no) {
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
>
> WARN_ON(func_no && !pcie->drvdata->func_offset);
> return pcie->drvdata->func_offset * func_no;
This is better. Given there is only currently one method of calculating
an offset for layerscape, I'd recommend you add additional methods when
the need arises.
Thanks,
Andrew Murray
>
> }
> Of course, we can return a -1 by adjuring the (func_no && !pcie->drvdata->func_offset)
> Valu in case1
>
> Thanks
> Xiaowei
>
> >
> > Thanks,
> >
> > Andrew Murray
> >
> > >
> > > I have added the comments above the code, as follow, do you have any
> > advice?
> > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > > + u8 func_no) {
> > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > + struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > + u8 header_type;
> > > +
> > > + header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > +
> > > + /*
> > > + * Read the Header Type register of config space to check
> > > + * whether this PCI device support the multiple function.
> > > + */
> > > + if (header_type & (1 << 7))
> > > + return pcie->drvdata->func_offset * func_no;
> > > +
> > > + return 0;
> > > +}
> > >
> > > Thanks a lot for your detail comments.
> > >
> > > >
> > > > Thanks,
> > > >
> > > > Andrew Murray
> > > >
> > > > > +}
> > > > > +
> > > > > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > > > > .ep_init = ls_pcie_ep_init,
> > > > > .raise_irq = ls_pcie_ep_raise_irq,
> > > > > .get_features = ls_pcie_ep_get_features,
> > > > > + .func_conf_select = ls_pcie_ep_func_conf_select, };
> > > > > +
> > > > > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > > > + .ops = &ls_pcie_ep_ops,
> > > > > + .dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > +
> > > > > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > > > + .func_offset = 0x20000,
> > > > > + .ops = &ls_pcie_ep_ops,
> > > > > + .dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > +
> > > > > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > + { .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > > > > + { .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > > + { .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > > + { },
> > > > > };
> > > > >
> > > > > static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@
> > > > > -98,7
> > > > > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep
> > > > > +*pcie,
> > > > > int ret;
> > > > >
> > > > > ep = &pci->ep;
> > > > > - ep->ops = &pcie_ep_ops;
> > > > > + ep->ops = pcie->drvdata->ops;
> > > > >
> > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > "addr_space");
> > > > > if (!res)
> > > > > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > > > platform_device *pdev)
> > > > > if (!ls_epc)
> > > > > return -ENOMEM;
> > > > >
> > > > > - dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > > > "regs");
> > > > > - pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > > - if (IS_ERR(pci->dbi_base))
> > > > > - return PTR_ERR(pci->dbi_base);
> > > > > + pcie->drvdata = of_device_get_match_data(dev);
> > > > >
> > > > > - pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > > pci->dev = dev;
> > > > > - pci->ops = &ls_pcie_ep_ops;
> > > > > + pci->ops = pcie->drvdata->dw_pcie_ops;
> > > > > +
> > > > > pcie->pci = pci;
> > > > >
> > > > > ls_epc->linkup_notifier = false, @@ -152,6 +185,13 @@ static int
> > > > > __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > > >
> > > > > pcie->ls_epc = ls_epc;
> > > > >
> > > > > + dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > > > "regs");
> > > > > + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > > + if (IS_ERR(pci->dbi_base))
> > > > > + return PTR_ERR(pci->dbi_base);
> > > > > +
> > > > > + pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > > +
> > > > > platform_set_drvdata(pdev, pcie);
> > > > >
> > > > > ret = ls_add_pcie_ep(pcie, pdev);
> > > > > --
> > > > > 2.9.5
> > > > >
^ permalink raw reply
* Re: [PATCH v2 3/4] powerpc/64: system call remove non-volatile GPR save optimisation
From: Christophe Leroy @ 2019-08-28 9:02 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20190827135548.21457-4-npiggin@gmail.com>
Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
> powerpc has an optimisation where interrupts avoid saving the
> non-volatile (or callee saved) registers to the interrupt stack frame if
> they are not required.
>
> Two problems with this are that an interrupt does not always know
> whether it will need non-volatiles; and if it does need them, they can
> only be saved from the entry-scoped asm code (because we don't control
> what the C compiler does with these registers).
>
> system calls are the most difficult: some system calls always require
> all registers (e.g., fork, to copy regs into the child). Sometimes
> registers are only required under certain conditions (e.g., tracing,
> signal delivery). These cases require ugly logic in the call chains
> (e.g., ppc_fork), and require a lot of logic to be implemented in asm.
Do you really find it ugly to just call function nvgprs() before calling
sys_fork() ? I guess there are things a lot uglier.
>
> So remove the optimisation for system calls, and always save NVGPRs on
> entry. Modern high performance CPUs are not so sensitive, because the
> stores are dense in cache and can be hidden by other expensive work in
> the syscall path -- the null syscall selftests benchmark on POWER9 is
> not slowed (124.40ns before and 123.64ns after, i.e., within the noise).
I did the test on PPC32:
On an 885, null_syscall reports 2227ns (132MHz)
If saving non-volatile regs, it goes to 2419, ie +8.6%
On an 8321, null_syscall reports 1021ns (333MHz)
If saving non-volatile regs, it goes to 1100, ie +7.7%
So unless going to C compensates this degradation, I guess it is not
worth it on PPC32.
>
> Other interrupts retain the NVGPR optimisation for now.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Changes since v1:
> - Improve changelog
> - Fix clone3 spu table entry (Segher)
>
> arch/powerpc/kernel/entry_64.S | 72 +++++-------------------
> arch/powerpc/kernel/syscalls/syscall.tbl | 22 +++++---
> 2 files changed, 28 insertions(+), 66 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 6467bdab8d40..5a3e0b5c9ad1 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -98,13 +98,14 @@ END_BTB_FLUSH_SECTION
> std r11,_XER(r1)
> std r11,_CTR(r1)
> std r9,GPR13(r1)
> + SAVE_NVGPRS(r1)
> mflr r10
> /*
> * This clears CR0.SO (bit 28), which is the error indication on
> * return from this system call.
> */
> rldimi r2,r11,28,(63-28)
> - li r11,0xc01
> + li r11,0xc00
> std r10,_LINK(r1)
> std r11,_TRAP(r1)
> std r3,ORIG_GPR3(r1)
> @@ -323,7 +324,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>
> /* Traced system call support */
> .Lsyscall_dotrace:
> - bl save_nvgprs
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl do_syscall_trace_enter
>
> @@ -408,7 +408,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> mtmsrd r10,1
> #endif /* CONFIG_PPC_BOOK3E */
>
> - bl save_nvgprs
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl do_syscall_trace_leave
> b ret_from_except
> @@ -442,62 +441,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> _ASM_NOKPROBE_SYMBOL(system_call_common);
> _ASM_NOKPROBE_SYMBOL(system_call_exit);
>
> -/* Save non-volatile GPRs, if not already saved. */
> -_GLOBAL(save_nvgprs)
> - ld r11,_TRAP(r1)
> - andi. r0,r11,1
> - beqlr-
> - SAVE_NVGPRS(r1)
> - clrrdi r0,r11,1
> - std r0,_TRAP(r1)
> - blr
> -_ASM_NOKPROBE_SYMBOL(save_nvgprs);
I see it is added back somewhere below. Why don't you leave it where it is ?
> -
> -
> -/*
> - * The sigsuspend and rt_sigsuspend system calls can call do_signal
> - * and thus put the process into the stopped state where we might
> - * want to examine its user state with ptrace. Therefore we need
> - * to save all the nonvolatile registers (r14 - r31) before calling
> - * the C code. Similarly, fork, vfork and clone need the full
> - * register state on the stack so that it can be copied to the child.
> - */
> -
> -_GLOBAL(ppc_fork)
> - bl save_nvgprs
> - bl sys_fork
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc_vfork)
> - bl save_nvgprs
> - bl sys_vfork
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc_clone)
> - bl save_nvgprs
> - bl sys_clone
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc_clone3)
> - bl save_nvgprs
> - bl sys_clone3
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc32_swapcontext)
> - bl save_nvgprs
> - bl compat_sys_swapcontext
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc64_swapcontext)
> - bl save_nvgprs
> - bl sys_swapcontext
> - b .Lsyscall_exit
> -
> -_GLOBAL(ppc_switch_endian)
> - bl save_nvgprs
> - bl sys_switch_endian
> - b .Lsyscall_exit
> -
> _GLOBAL(ret_from_fork)
> bl schedule_tail
> REST_NVGPRS(r1)
> @@ -516,6 +459,17 @@ _GLOBAL(ret_from_kernel_thread)
> li r3,0
> b .Lsyscall_exit
>
> +/* Save non-volatile GPRs, if not already saved. */
> +_GLOBAL(save_nvgprs)
> + ld r11,_TRAP(r1)
> + andi. r0,r11,1
> + beqlr-
> + SAVE_NVGPRS(r1)
> + clrrdi r0,r11,1
> + std r0,_TRAP(r1)
> + blr
> +_ASM_NOKPROBE_SYMBOL(save_nvgprs);
> +
Moved here.
> #ifdef CONFIG_PPC_BOOK3S_64
>
> #define FLUSH_COUNT_CACHE \
Christophe
^ permalink raw reply
* Re: [PATCH v2 0/4] powerpc/64: syscalls in C
From: Christophe Leroy @ 2019-08-28 9:06 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20190827135548.21457-1-npiggin@gmail.com>
Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
> Accounted for some feedback.
>
> Nicholas Piggin (4):
> powerpc: convert to copy_thread_tls
> powerpc/64: remove support for kernel-mode syscalls
> powerpc/64: system call remove non-volatile GPR save optimisation
> powerpc/64: system call implement the bulk of the logic in C
Would it be possible to split in the following parts:
1/ Implement in C whatever can be implemented without removing
non-volatile GPR save optimisation
2/ Remove non-volatile GPR save optimisation
3/ Implement in C everything else
Christophe
>
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/asm-prototypes.h | 11 -
> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +-
> arch/powerpc/include/asm/cputime.h | 22 +
> arch/powerpc/include/asm/ptrace.h | 3 +
> arch/powerpc/include/asm/signal.h | 2 +
> arch/powerpc/include/asm/switch_to.h | 5 +
> arch/powerpc/include/asm/time.h | 3 +
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/entry_64.S | 421 +++---------------
> arch/powerpc/kernel/exceptions-64s.S | 2 -
> arch/powerpc/kernel/process.c | 9 +-
> arch/powerpc/kernel/signal.h | 2 -
> arch/powerpc/kernel/syscall_64.c | 177 ++++++++
> arch/powerpc/kernel/syscalls/syscall.tbl | 22 +-
> 15 files changed, 307 insertions(+), 388 deletions(-)
> create mode 100644 arch/powerpc/kernel/syscall_64.c
>
^ permalink raw reply
* [PATCH] powerpc/64: interrupt return in C
From: Nicholas Piggin @ 2019-08-28 9:06 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
This is a work in progress that goes on top of the syscalls in C patch.
It's not quite complete, 64e low level exit is not taken care of, and
the new return is hacked into the existing interrupt handlers pretty
quickly (e.g., full gprs handling is still ugly and could be cleaned),
but that code touches exception-64s.S which is under heavy modification
in parallel so I will rebase on top of that before polishing it properly.
I will also try to convert to more IS_ENABLED() for Christophe.
I guess syscall_64.c will change to interrupt_64.c with this.
Hopefully there is no fundamental problem with it, it replaces some
fairly horrific asm code with stack frame bouncing and reusing for
replay, and almost imcomprehensible maze of exit points and labels.
This is just a quick preview because it's booting, if anyone can spot
major issue with the approach would be good.
Thanks,
Nick
---
.../powerpc/include/asm/book3s/64/kup-radix.h | 6 +
arch/powerpc/kernel/entry_64.S | 475 ++++--------------
arch/powerpc/kernel/exceptions-64s.S | 55 +-
arch/powerpc/kernel/syscall_64.c | 147 +++++-
arch/powerpc/kernel/vector.S | 2 +-
5 files changed, 288 insertions(+), 397 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index ef2e65ea8a73..62ff2509cf51 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -60,6 +60,12 @@
#include <asm/mmu.h>
#include <asm/ptrace.h>
+static inline void kuap_restore_amr(struct pt_regs *regs)
+{
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ mtspr(SPRN_AMR, regs->kuap);
+}
+
static inline void kuap_check_amr(void)
{
#ifdef CONFIG_PPC_KUAP_DEBUG
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index d2efd1a96487..f1e973789a2a 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -16,6 +16,7 @@
#include <linux/errno.h>
#include <linux/err.h>
+#include <asm/cache.h>
#include <asm/unistd.h>
#include <asm/processor.h>
#include <asm/page.h>
@@ -130,9 +131,6 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
mtspr SPRN_SRR0,r4
mtspr SPRN_SRR1,r5
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- std r5,PACATMSCRATCH(r13)
-#endif
mtlr r6
cmpdi r3,0
@@ -197,6 +195,7 @@ tabort_syscall:
RFI_TO_USER
b . /* prevent speculative execution */
#endif
+
_GLOBAL(ret_from_fork)
bl schedule_tail
REST_NVGPRS(r1)
@@ -436,409 +435,149 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
addi r1,r1,SWITCH_FRAME_SIZE
blr
- .align 7
-_GLOBAL(ret_from_except)
- ld r11,_TRAP(r1)
- andi. r0,r11,1
- bne ret_from_except_lite
- REST_NVGPRS(r1)
-
-_GLOBAL(ret_from_except_lite)
/*
- * Disable interrupts so that current_thread_info()->flags
- * can't change between when we test it and when we return
- * from the interrupt.
- */
-#ifdef CONFIG_PPC_BOOK3E
- wrteei 0
-#else
- li r10,MSR_RI
- mtmsrd r10,1 /* Update machine state */
-#endif /* CONFIG_PPC_BOOK3E */
+ * If MSR EE/RI was never enabled, IRQs not reconciled, NVGPRs not
+ * touched, AMR not set, no exit work created, then this can be used.
+ */
+ .balign IFETCH_ALIGN_BYTES
+_GLOBAL(fast_interrupt_return)
+ ld r4,_MSR(r1)
+ andi. r0,r4,MSR_PR
+ bne .Lfast_user_interrupt_return
+ andi. r0,r4,MSR_RI
+ bne+ .Lfast_kernel_interrupt_return
+ addi r3,r1,STACK_FRAME_OVERHEAD
+ bl unrecoverable_exception
+ b . /* should not get here */
- ld r9, PACA_THREAD_INFO(r13)
- ld r3,_MSR(r1)
-#ifdef CONFIG_PPC_BOOK3E
- ld r10,PACACURRENT(r13)
-#endif /* CONFIG_PPC_BOOK3E */
- ld r4,TI_FLAGS(r9)
- andi. r3,r3,MSR_PR
- beq resume_kernel
-#ifdef CONFIG_PPC_BOOK3E
- lwz r3,(THREAD+THREAD_DBCR0)(r10)
-#endif /* CONFIG_PPC_BOOK3E */
+ .balign IFETCH_ALIGN_BYTES
+_GLOBAL(interrupt_return)
+ REST_NVGPRS(r1)
- /* Check current_thread_info()->flags */
- andi. r0,r4,_TIF_USER_WORK_MASK
- bne 1f
-#ifdef CONFIG_PPC_BOOK3E
- /*
- * Check to see if the dbcr0 register is set up to debug.
- * Use the internal debug mode bit to do this.
- */
- andis. r0,r3,DBCR0_IDM@h
- beq restore
- mfmsr r0
- rlwinm r0,r0,0,~MSR_DE /* Clear MSR.DE */
- mtmsr r0
- mtspr SPRN_DBCR0,r3
- li r10, -1
- mtspr SPRN_DBSR,r10
- b restore
-#else
- addi r3,r1,STACK_FRAME_OVERHEAD
- bl restore_math
- b restore
-#endif
-1: andi. r0,r4,_TIF_NEED_RESCHED
- beq 2f
- bl restore_interrupts
- SCHEDULE_USER
- b ret_from_except_lite
-2:
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- andi. r0,r4,_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM
- bne 3f /* only restore TM if nothing else to do */
+ .balign IFETCH_ALIGN_BYTES
+_GLOBAL(interrupt_return_lite)
+ ld r4,_MSR(r1)
+ andi. r0,r4,MSR_PR
+ beq kernel_interrupt_return
+user_interrupt_return:
addi r3,r1,STACK_FRAME_OVERHEAD
- bl restore_tm_state
- b restore
-3:
-#endif
- bl save_nvgprs
- /*
- * Use a non volatile GPR to save and restore our thread_info flags
- * across the call to restore_interrupts.
- */
- mr r30,r4
- bl restore_interrupts
- mr r4,r30
- addi r3,r1,STACK_FRAME_OVERHEAD
- bl do_notify_resume
- b ret_from_except
-
-resume_kernel:
- /* check current_thread_info, _TIF_EMULATE_STACK_STORE */
- andis. r8,r4,_TIF_EMULATE_STACK_STORE@h
- beq+ 1f
+ bl interrupt_exit_user_prepare
+ cmpdi r3,0
+ bne- .Lrestore_nvgprs
- addi r8,r1,INT_FRAME_SIZE /* Get the kprobed function entry */
+.Lfast_user_interrupt_return:
+ ld r11,_NIP(r1)
+ ld r12,_MSR(r1)
+BEGIN_FTR_SECTION
+ ld r10,_PPR(r1)
+ mtspr SPRN_PPR,r10
+END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
+ mtspr SPRN_SRR0,r11
+ mtspr SPRN_SRR1,r12
- ld r3,GPR1(r1)
- subi r3,r3,INT_FRAME_SIZE /* dst: Allocate a trampoline exception frame */
- mr r4,r1 /* src: current exception frame */
- mr r1,r3 /* Reroute the trampoline frame to r1 */
+BEGIN_FTR_SECTION
+ stdcx. r0,0,r1 /* to clear the reservation */
+FTR_SECTION_ELSE
+ ldarx r0,0,r1
+ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
- /* Copy from the original to the trampoline. */
- li r5,INT_FRAME_SIZE/8 /* size: INT_FRAME_SIZE */
- li r6,0 /* start offset: 0 */
- mtctr r5
-2: ldx r0,r6,r4
- stdx r0,r6,r3
- addi r6,r6,8
- bdnz 2b
-
- /* Do real store operation to complete stdu */
- ld r5,GPR1(r1)
- std r8,0(r5)
-
- /* Clear _TIF_EMULATE_STACK_STORE flag */
- lis r11,_TIF_EMULATE_STACK_STORE@h
- addi r5,r9,TI_FLAGS
-0: ldarx r4,0,r5
- andc r4,r4,r11
- stdcx. r4,0,r5
- bne- 0b
-1:
-
-#ifdef CONFIG_PREEMPT
- /* Check if we need to preempt */
- andi. r0,r4,_TIF_NEED_RESCHED
- beq+ restore
- /* Check that preempt_count() == 0 and interrupts are enabled */
- lwz r8,TI_PREEMPT(r9)
- cmpwi cr0,r8,0
- bne restore
- ld r0,SOFTE(r1)
- andi. r0,r0,IRQS_DISABLED
- bne restore
+ ld r3,_CCR(r1)
+ ld r4,_LINK(r1)
+ ld r5,_CTR(r1)
+ ld r6,_XER(r1)
+ li r0,0
- /*
- * Here we are preempting the current task. We want to make
- * sure we are soft-disabled first and reconcile irq state.
- */
- RECONCILE_IRQ_STATE(r3,r4)
- bl preempt_schedule_irq
+ REST_4GPRS(7, r1)
+ REST_2GPRS(11, r1)
+ REST_GPR(13, r1)
- /*
- * arch_local_irq_restore() from preempt_schedule_irq above may
- * enable hard interrupt but we really should disable interrupts
- * when we return from the interrupt, and so that we don't get
- * interrupted after loading SRR0/1.
- */
-#ifdef CONFIG_PPC_BOOK3E
- wrteei 0
-#else
- li r10,MSR_RI
- mtmsrd r10,1 /* Update machine state */
-#endif /* CONFIG_PPC_BOOK3E */
-#endif /* CONFIG_PREEMPT */
+ mtcr r3
+ mtlr r4
+ mtctr r5
+ mtspr SPRN_XER,r6
- .globl fast_exc_return_irq
-fast_exc_return_irq:
-restore:
- /*
- * This is the main kernel exit path. First we check if we
- * are about to re-enable interrupts
- */
- ld r5,SOFTE(r1)
- lbz r6,PACAIRQSOFTMASK(r13)
- andi. r5,r5,IRQS_DISABLED
- bne .Lrestore_irq_off
+ REST_4GPRS(2, r1)
+ REST_GPR(6, r1)
+ REST_GPR(0, r1)
+ REST_GPR(1, r1)
+ RFI_TO_USER
+ b . /* prevent speculative execution */
- /* We are enabling, were we already enabled ? Yes, just return */
- andi. r6,r6,IRQS_DISABLED
- beq cr0,.Ldo_restore
+.Lrestore_nvgprs:
+ REST_NVGPRS(r1)
+ b .Lfast_user_interrupt_return
- /*
- * We are about to soft-enable interrupts (we are hard disabled
- * at this point). We check if there's anything that needs to
- * be replayed first.
- */
- lbz r0,PACAIRQHAPPENED(r13)
- cmpwi cr0,r0,0
- bne- .Lrestore_check_irq_replay
+ .balign IFETCH_ALIGN_BYTES
+kernel_interrupt_return:
+ addi r3,r1,STACK_FRAME_OVERHEAD
+ bl interrupt_exit_kernel_prepare
+ cmpdi cr1,r3,0
- /*
- * Get here when nothing happened while soft-disabled, just
- * soft-enable and move-on. We will hard-enable as a side
- * effect of rfi
- */
-.Lrestore_no_replay:
- TRACE_ENABLE_INTS
- li r0,IRQS_ENABLED
- stb r0,PACAIRQSOFTMASK(r13);
+.Lfast_kernel_interrupt_return:
+ ld r11,_NIP(r1)
+ ld r12,_MSR(r1)
+ mtspr SPRN_SRR0,r11
+ mtspr SPRN_SRR1,r12
- /*
- * Final return path. BookE is handled in a different file
- */
-.Ldo_restore:
-#ifdef CONFIG_PPC_BOOK3E
- b exception_return_book3e
-#else
- /*
- * Clear the reservation. If we know the CPU tracks the address of
- * the reservation then we can potentially save some cycles and use
- * a larx. On POWER6 and POWER7 this is significantly faster.
- */
BEGIN_FTR_SECTION
stdcx. r0,0,r1 /* to clear the reservation */
FTR_SECTION_ELSE
- ldarx r4,0,r1
+ ldarx r0,0,r1
ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
- /*
- * Some code path such as load_up_fpu or altivec return directly
- * here. They run entirely hard disabled and do not alter the
- * interrupt state. They also don't use lwarx/stwcx. and thus
- * are known not to leave dangling reservations.
- */
- .globl fast_exception_return
-fast_exception_return:
- ld r3,_MSR(r1)
- ld r4,_CTR(r1)
- ld r0,_LINK(r1)
- mtctr r4
- mtlr r0
- ld r4,_XER(r1)
- mtspr SPRN_XER,r4
-
- kuap_check_amr r5, r6
-
- REST_8GPRS(5, r1)
-
- andi. r0,r3,MSR_RI
- beq- .Lunrecov_restore
+ ld r3,_CCR(r1)
+ ld r4,_LINK(r1)
+ ld r5,_CTR(r1)
+ ld r6,_XER(r1)
+ li r0,0
- /*
- * Clear RI before restoring r13. If we are returning to
- * userspace and we take an exception after restoring r13,
- * we end up corrupting the userspace r13 value.
- */
- li r4,0
- mtmsrd r4,1
+ REST_4GPRS(7, r1)
+ REST_2GPRS(11, r1)
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- /* TM debug */
- std r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */
-#endif
- /*
- * r13 is our per cpu area, only restore it if we are returning to
- * userspace the value stored in the stack frame may belong to
- * another CPU.
- */
- andi. r0,r3,MSR_PR
- beq 1f
-BEGIN_FTR_SECTION
- /* Restore PPR */
- ld r2,_PPR(r1)
- mtspr SPRN_PPR,r2
-END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
- ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
- REST_GPR(13, r1)
+ bne- cr1,1f /* emulate stack store */
+ mtcr r3
+ mtlr r4
+ mtctr r5
+ mtspr SPRN_XER,r6
/*
- * We don't need to restore AMR on the way back to userspace for KUAP.
- * The value of AMR only matters while we're in the kernel.
+ * Leaving a stale exception_marker on the stack can confuse
+ * the reliable stack unwinder later on. Clear it.
*/
- mtspr SPRN_SRR1,r3
+ std r0,STACK_FRAME_OVERHEAD-16(r1)
- ld r2,_CCR(r1)
- mtcrf 0xFF,r2
- ld r2,_NIP(r1)
- mtspr SPRN_SRR0,r2
-
- ld r0,GPR0(r1)
- ld r2,GPR2(r1)
- ld r3,GPR3(r1)
- ld r4,GPR4(r1)
- ld r1,GPR1(r1)
- RFI_TO_USER
+ REST_4GPRS(2, r1)
+ REST_GPR(6, r1)
+ REST_GPR(0, r1)
+ REST_GPR(1, r1)
+ RFI_TO_KERNEL
b . /* prevent speculative execution */
-1: mtspr SPRN_SRR1,r3
-
- ld r2,_CCR(r1)
- mtcrf 0xFF,r2
- ld r2,_NIP(r1)
- mtspr SPRN_SRR0,r2
+1: mtcr r3
+ mtlr r4
+ mtctr r5
+ mtspr SPRN_XER,r6
/*
* Leaving a stale exception_marker on the stack can confuse
* the reliable stack unwinder later on. Clear it.
*/
- li r2,0
- std r2,STACK_FRAME_OVERHEAD-16(r1)
+ std r0,STACK_FRAME_OVERHEAD-16(r1)
- ld r0,GPR0(r1)
- ld r2,GPR2(r1)
- ld r3,GPR3(r1)
+ REST_4GPRS(2, r1)
+ REST_GPR(6, r1)
+ REST_GPR(0, r1)
- kuap_restore_amr r4
+ /* Nasty emulate stack store case. */
+ std r9,PACA_EXGEN+0(r13)
+ addi r9,r1,INT_FRAME_SIZE /* get original r1 */
+ REST_GPR(1, r1)
+ std r9,0(r1)
+ ld r9,PACA_EXGEN+0(r13)
- ld r4,GPR4(r1)
- ld r1,GPR1(r1)
RFI_TO_KERNEL
b . /* prevent speculative execution */
-#endif /* CONFIG_PPC_BOOK3E */
-
- /*
- * We are returning to a context with interrupts soft disabled.
- *
- * However, we may also about to hard enable, so we need to
- * make sure that in this case, we also clear PACA_IRQ_HARD_DIS
- * or that bit can get out of sync and bad things will happen
- */
-.Lrestore_irq_off:
- ld r3,_MSR(r1)
- lbz r7,PACAIRQHAPPENED(r13)
- andi. r0,r3,MSR_EE
- beq 1f
- rlwinm r7,r7,0,~PACA_IRQ_HARD_DIS
- stb r7,PACAIRQHAPPENED(r13)
-1:
-#if defined(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG) && defined(CONFIG_BUG)
- /* The interrupt should not have soft enabled. */
- lbz r7,PACAIRQSOFTMASK(r13)
-1: tdeqi r7,IRQS_ENABLED
- EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,BUGFLAG_WARNING
-#endif
- b .Ldo_restore
-
- /*
- * Something did happen, check if a re-emit is needed
- * (this also clears paca->irq_happened)
- */
-.Lrestore_check_irq_replay:
- /* XXX: We could implement a fast path here where we check
- * for irq_happened being just 0x01, in which case we can
- * clear it and return. That means that we would potentially
- * miss a decrementer having wrapped all the way around.
- *
- * Still, this might be useful for things like hash_page
- */
- bl __check_irq_replay
- cmpwi cr0,r3,0
- beq .Lrestore_no_replay
-
- /*
- * We need to re-emit an interrupt. We do so by re-using our
- * existing exception frame. We first change the trap value,
- * but we need to ensure we preserve the low nibble of it
- */
- ld r4,_TRAP(r1)
- clrldi r4,r4,60
- or r4,r4,r3
- std r4,_TRAP(r1)
-
- /*
- * PACA_IRQ_HARD_DIS won't always be set here, so set it now
- * to reconcile the IRQ state. Tracing is already accounted for.
- */
- lbz r4,PACAIRQHAPPENED(r13)
- ori r4,r4,PACA_IRQ_HARD_DIS
- stb r4,PACAIRQHAPPENED(r13)
-
- /*
- * Then find the right handler and call it. Interrupts are
- * still soft-disabled and we keep them that way.
- */
- cmpwi cr0,r3,0x500
- bne 1f
- addi r3,r1,STACK_FRAME_OVERHEAD;
- bl do_IRQ
- b ret_from_except
-1: cmpwi cr0,r3,0xf00
- bne 1f
- addi r3,r1,STACK_FRAME_OVERHEAD;
- bl performance_monitor_exception
- b ret_from_except
-1: cmpwi cr0,r3,0xe60
- bne 1f
- addi r3,r1,STACK_FRAME_OVERHEAD;
- bl handle_hmi_exception
- b ret_from_except
-1: cmpwi cr0,r3,0x900
- bne 1f
- addi r3,r1,STACK_FRAME_OVERHEAD;
- bl timer_interrupt
- b ret_from_except
-#ifdef CONFIG_PPC_DOORBELL
-1:
-#ifdef CONFIG_PPC_BOOK3E
- cmpwi cr0,r3,0x280
-#else
- cmpwi cr0,r3,0xa00
-#endif /* CONFIG_PPC_BOOK3E */
- bne 1f
- addi r3,r1,STACK_FRAME_OVERHEAD;
- bl doorbell_exception
-#endif /* CONFIG_PPC_DOORBELL */
-1: b ret_from_except /* What else to do here ? */
-
-.Lunrecov_restore:
- addi r3,r1,STACK_FRAME_OVERHEAD
- bl unrecoverable_exception
- b .Lunrecov_restore
-
-_ASM_NOKPROBE_SYMBOL(ret_from_except);
-_ASM_NOKPROBE_SYMBOL(ret_from_except_lite);
-_ASM_NOKPROBE_SYMBOL(resume_kernel);
-_ASM_NOKPROBE_SYMBOL(fast_exc_return_irq);
-_ASM_NOKPROBE_SYMBOL(restore);
-_ASM_NOKPROBE_SYMBOL(fast_exception_return);
-
-
#ifdef CONFIG_PPC_RTAS
/*
* On CHRP, the Run-Time Abstraction Services (RTAS) have to be
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 768f133de4f1..51223299d22d 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -696,7 +696,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
RECONCILE_IRQ_STATE(r10, r11); \
addi r3,r1,STACK_FRAME_OVERHEAD; \
bl hdlr; \
- b ret_from_except
+ b interrupt_return
/*
* Like EXC_COMMON, but for exceptions that can occur in the idle task and
@@ -706,11 +706,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
EXC_COMMON_BEGIN(name); \
EXCEPTION_COMMON(PACA_EXGEN, realvec); \
FINISH_NAP; \
+ bl save_nvgprs; \
RECONCILE_IRQ_STATE(r10, r11); \
RUNLATCH_ON; \
addi r3,r1,STACK_FRAME_OVERHEAD; \
bl hdlr; \
- b ret_from_except_lite
+ b interrupt_return
/*
@@ -1058,7 +1059,7 @@ EXC_COMMON_BEGIN(machine_check_common)
bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl machine_check_exception
- b ret_from_except
+ b interrupt_return
#define MACHINE_CHECK_HANDLER_WINDUP \
/* Clear MSR_RI before setting SRR0 and SRR1. */\
@@ -1301,7 +1302,7 @@ BEGIN_MMU_FTR_SECTION
bl do_slb_fault
cmpdi r3,0
bne- 1f
- b fast_exception_return
+ b fast_interrupt_return
1: /* Error case */
MMU_FTR_SECTION_ELSE
/* Radix case, access is outside page table range */
@@ -1314,7 +1315,7 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
ld r5,RESULT(r1)
addi r3,r1,STACK_FRAME_OVERHEAD
bl do_bad_slb_fault
- b ret_from_except
+ b interrupt_return
EXC_REAL(instruction_access, 0x400, 0x80)
@@ -1350,7 +1351,7 @@ BEGIN_MMU_FTR_SECTION
bl do_slb_fault
cmpdi r3,0
bne- 1f
- b fast_exception_return
+ b fast_interrupt_return
1: /* Error case */
MMU_FTR_SECTION_ELSE
/* Radix case, access is outside page table range */
@@ -1363,7 +1364,7 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
ld r5,RESULT(r1)
addi r3,r1,STACK_FRAME_OVERHEAD
bl do_bad_slb_fault
- b ret_from_except
+ b interrupt_return
EXC_REAL_BEGIN(hardware_interrupt, 0x500, 0x100)
@@ -1416,7 +1417,7 @@ EXC_COMMON_BEGIN(alignment_common)
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl alignment_exception
- b ret_from_except
+ b interrupt_return
EXC_REAL(program_check, 0x700, 0x100)
@@ -1454,7 +1455,7 @@ EXC_COMMON_BEGIN(program_check_common)
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl program_check_exception
- b ret_from_except
+ b interrupt_return
EXC_REAL(fp_unavailable, 0x800, 0x100)
@@ -1479,14 +1480,14 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_TM)
#endif
bl load_up_fpu
- b fast_exception_return
+ b fast_interrupt_return
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
2: /* User process was in a transaction */
bl save_nvgprs
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl fp_unavailable_tm
- b ret_from_except
+ b interrupt_return
#endif
@@ -1676,7 +1677,7 @@ BEGIN_MMU_FTR_SECTION
MMU_FTR_SECTION_ELSE
bl unknown_exception
ALT_MMU_FTR_SECTION_END_IFSET(MMU_FTR_TYPE_RADIX)
- b ret_from_except
+ b interrupt_return
EXC_REAL_OOL_HV(h_instr_storage, 0xe20, 0x20)
@@ -1744,7 +1745,7 @@ EXC_COMMON_BEGIN(hmi_exception_common)
RUNLATCH_ON
addi r3,r1,STACK_FRAME_OVERHEAD
bl handle_hmi_exception
- b ret_from_except
+ b interrupt_return
EXC_REAL_OOL_MASKABLE_HV(h_doorbell, 0xe80, 0x20, IRQS_DISABLED)
EXC_VIRT_OOL_MASKABLE_HV(h_doorbell, 0x4e80, 0x20, 0xe80, IRQS_DISABLED)
@@ -1792,14 +1793,14 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_NESTED(CPU_FTR_TM, CPU_FTR_TM, 69)
#endif
bl load_up_altivec
- b fast_exception_return
+ b fast_interrupt_return
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
2: /* User process was in a transaction */
bl save_nvgprs
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl altivec_unavailable_tm
- b ret_from_except
+ b interrupt_return
#endif
1:
END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
@@ -1808,7 +1809,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl altivec_unavailable_exception
- b ret_from_except
+ b interrupt_return
EXC_REAL_OOL(vsx_unavailable, 0xf40, 0x20)
@@ -1835,7 +1836,7 @@ BEGIN_FTR_SECTION
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl vsx_unavailable_tm
- b ret_from_except
+ b interrupt_return
#endif
1:
END_FTR_SECTION_IFSET(CPU_FTR_VSX)
@@ -1844,7 +1845,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl vsx_unavailable_exception
- b ret_from_except
+ b interrupt_return
EXC_REAL_OOL(facility_unavailable, 0xf60, 0x20)
@@ -2046,7 +2047,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
RECONCILE_IRQ_STATE(r10, r11)
addi r3,r1,STACK_FRAME_OVERHEAD
bl soft_nmi_interrupt
- b ret_from_except
+ b interrupt_return
#else /* CONFIG_PPC_WATCHDOG */
#define MASKED_DEC_HANDLER_LABEL 2f /* normal return */
@@ -2329,7 +2330,7 @@ do_hash_page:
cmpdi r3,0 /* see if __hash_page succeeded */
/* Success */
- beq fast_exc_return_irq /* Return from exception on success */
+ beq interrupt_return_lite /* Return from exception on success */
/* Error */
blt- 13f
@@ -2344,16 +2345,16 @@ handle_page_fault:
bne- handle_dabr_fault
ld r4,_DAR(r1)
ld r5,_DSISR(r1)
+ bl save_nvgprs
addi r3,r1,STACK_FRAME_OVERHEAD
bl do_page_fault
cmpdi r3,0
- beq+ ret_from_except_lite
- bl save_nvgprs
+ beq+ interrupt_return_lite
mr r5,r3
addi r3,r1,STACK_FRAME_OVERHEAD
lwz r4,_DAR(r1)
bl bad_page_fault
- b ret_from_except
+ b interrupt_return
/* We have a data breakpoint exception - handle it */
handle_dabr_fault:
@@ -2365,9 +2366,9 @@ handle_dabr_fault:
/*
* do_break() may have changed the NV GPRS while handling a breakpoint.
* If so, we need to restore them with their updated values. Don't use
- * ret_from_except_lite here.
+ * interrupt_return_lite here.
*/
- b ret_from_except
+ b interrupt_return
#ifdef CONFIG_PPC_BOOK3S_64
@@ -2379,7 +2380,7 @@ handle_dabr_fault:
addi r3,r1,STACK_FRAME_OVERHEAD
ld r4,_DAR(r1)
bl low_hash_fault
- b ret_from_except
+ b interrupt_return
#endif
/*
@@ -2394,7 +2395,7 @@ handle_dabr_fault:
addi r3,r1,STACK_FRAME_OVERHEAD
li r5,SIGSEGV
bl bad_page_fault
- b ret_from_except
+ b interrupt_return
/*
* When doorbell is triggered from system reset wakeup, the message is
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index d42519b86ddd..1e7c3e47a8b8 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -161,13 +161,17 @@ unsigned long syscall_exit_prepare(unsigned long r3, struct pt_regs *regs)
__mtmsrd(MSR_RI, 1);
local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
local_irq_enable();
- /* Took an interrupt which may have more exit work to do. */
+ /* Took an interrupt, may have more exit work to do. */
goto again;
}
trace_hardirqs_on();
local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
irq_soft_mask_set(IRQS_ENABLED);
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+ local_paca->tm_scratch = regs->msr;
+#endif
+
kuap_check_amr();
account_cpu_user_exit();
@@ -175,3 +179,144 @@ unsigned long syscall_exit_prepare(unsigned long r3, struct pt_regs *regs)
return ret;
}
+unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
+{
+#ifdef CONFIG_PPC_BOOK3E
+ struct thread_struct *ts = ¤t->thread;
+#endif
+ unsigned long *ti_flagsp = ¤t_thread_info()->flags;
+ unsigned long ti_flags;
+ unsigned long flags;
+ unsigned long ret = 0;
+
+ BUG_ON(!FULL_REGS(regs));
+ BUG_ON(!(regs->msr & MSR_RI));
+ BUG_ON(regs->softe != IRQS_ENABLED);
+
+ local_irq_save(flags);
+
+again:
+ ti_flags = READ_ONCE(*ti_flagsp);
+ while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
+ local_irq_enable(); /* returning to user: may enable */
+ if (ti_flags & _TIF_NEED_RESCHED) {
+ schedule();
+ } else {
+ if (ti_flags & _TIF_SIGPENDING)
+ ret |= _TIF_RESTOREALL;
+ do_notify_resume(regs, ti_flags);
+ }
+ local_irq_disable();
+ ti_flags = READ_ONCE(*ti_flagsp);
+ }
+
+#ifdef CONFIG_PPC_BOOK3S
+ if (IS_ENABLED(CONFIG_PPC_FPU)) {
+ unsigned long mathflags = MSR_FP;
+
+ if (IS_ENABLED(CONFIG_ALTIVEC))
+ mathflags |= MSR_VEC;
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+ if (ti_flags & _TIF_RESTORE_TM)
+ restore_tm_state(regs);
+ else
+#endif
+ if ((regs->msr & mathflags) != mathflags)
+ restore_math(regs);
+ }
+#endif
+
+ __mtmsrd(0, 1); /* Disable MSR_EE and MSR_RI */
+ if (unlikely(lazy_irq_pending())) {
+ __mtmsrd(MSR_RI, 1);
+ local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
+ local_irq_enable();
+ local_irq_disable();
+ /* Took an interrupt, may have more exit work to do. */
+ goto again;
+ }
+ trace_hardirqs_on();
+ local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
+ irq_soft_mask_set(IRQS_ENABLED);
+
+#ifdef CONFIG_PPC_BOOK3E
+ if (unlikely(ts->debug.debug.dbcr0 & DBCR0_IDM)) {
+ /*
+ * Check to see if the dbcr0 register is set up to debug.
+ * Use the internal debug mode bit to do this.
+ */
+ mtmsr(mfmsr() & ~MSR_DE);
+ mtspr(SPRN_DBCR0, ts->debug.debug.dbcr0);
+ mtspr(SPRN_DBSR, -1);
+ }
+#endif
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+ local_paca->tm_scratch = regs->msr;
+#endif
+
+ kuap_check_amr();
+
+ account_cpu_user_exit();
+
+ return ret;
+}
+
+void unrecoverable_exception(struct pt_regs *regs);
+void preempt_schedule_irq(void);
+
+
+unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
+{
+ unsigned long *ti_flagsp = ¤t_thread_info()->flags;
+ unsigned long flags;
+
+ BUG_ON(!FULL_REGS(regs));
+ local_irq_save(flags);
+
+ if (unlikely(!(regs->msr & MSR_RI)))
+ unrecoverable_exception(regs);
+
+again:
+ if (IS_ENABLED(CONFIG_PREEMPT)) {
+ /* Return to preemptible kernel context */
+ if (unlikely(*ti_flagsp & _TIF_NEED_RESCHED)) {
+ if (preempt_count() == 0 && regs->softe == IRQS_ENABLED)
+ preempt_schedule_irq();
+ }
+ }
+
+ __mtmsrd(0, 1); /* Disable MSR_EE and MSR_RI */
+ if (regs->softe == IRQS_ENABLED) {
+ if (unlikely(lazy_irq_pending())) {
+ __mtmsrd(MSR_RI, 1);
+ local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
+ local_irq_enable();
+ local_irq_disable();
+ /* Took an interrupt, may have more exit work to do. */
+ goto again;
+ }
+ trace_hardirqs_on();
+ irq_soft_mask_set(IRQS_ENABLED);
+ }
+ if (regs->msr & MSR_EE)
+ local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+ local_paca->tm_scratch = regs->msr;
+#endif
+
+ /*
+ * We don't need to restore AMR on the way back to userspace for KUAP.
+ * The value of AMR only matters while we're in the kernel.
+ */
+ kuap_restore_amr(regs);
+
+ if (unlikely(*ti_flagsp & _TIF_EMULATE_STACK_STORE)) {
+ clear_bits(_TIF_EMULATE_STACK_STORE, ti_flagsp);
+ return 1;
+ }
+ return 0;
+}
+
diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
index 8eb867dbad5f..44e7a776e56f 100644
--- a/arch/powerpc/kernel/vector.S
+++ b/arch/powerpc/kernel/vector.S
@@ -131,7 +131,7 @@ _GLOBAL(load_up_vsx)
/* enable use of VSX after return */
oris r12,r12,MSR_VSX@h
std r12,_MSR(r1)
- b fast_exception_return
+ b fast_interrupt_return
#endif /* CONFIG_VSX */
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v2] powerpc/powernv: Add ultravisor message log interface
From: Vaidyanathan Srinivasan @ 2019-08-28 9:22 UTC (permalink / raw)
To: Claudio Carvalho
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, kvm-ppc,
linuxppc-dev, Ryan Grimm, Oliver O'Halloran
In-Reply-To: <4e577a36-4ce1-410b-3ceb-d31bbf564b3d@linux.ibm.com>
* Claudio Carvalho <cclaudio@linux.ibm.com> [2019-08-24 23:19:19]:
>
> On 8/23/19 9:48 AM, Michael Ellerman wrote:
> > Hi Claudio,
>
> Hi Michael,
>
> >
> > Claudio Carvalho <cclaudio@linux.ibm.com> writes:
> >> Ultravisor (UV) provides an in-memory console which follows the OPAL
> >> in-memory console structure.
> >>
> >> This patch extends the OPAL msglog code to also initialize the UV memory
> >> console and provide a sysfs interface (uv_msglog) for userspace to view
> >> the UV message log.
> >>
> >> CC: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> >> CC: Oliver O'Halloran <oohall@gmail.com>
> >> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> >> ---
> >> This patch depends on the "kvmppc: Paravirtualize KVM to support
> >> ultravisor" patchset submitted by Claudio Carvalho.
> >> ---
> >> arch/powerpc/platforms/powernv/opal-msglog.c | 99 ++++++++++++++------
> >> 1 file changed, 72 insertions(+), 27 deletions(-)
> > I think the code changes look mostly OK here.
> >
> > But I'm not sure about the end result in sysfs.
> >
> > If I'm reading it right this will create:
> >
> > /sys/firmware/opal/uv_msglog
> >
> > Which I think is a little weird, because the UV is not OPAL.
> >
> > So I guess I wonder if the file should be created elsewhere to avoid any
> > confusion and keep things nicely separated.
> >
> > Possibly /sys/firmware/ultravisor/msglog ?
>
>
> Yes, makes sense. I will do that.
+1
Letting the UV have its own /sys/firmware/ultravisor/xxx is a good
idea. We may have a need to export more runtime data from UV for
debug/profile purposes and this sysfs directory will come handy.
--Vaidy
^ permalink raw reply
* Re: [RFC V2 0/1] mm/debug: Add tests for architecture exported page table helpers
From: Anshuman Khandual @ 2019-08-28 9:22 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm, Dave Hansen,
Paul Mackerras, sparclinux, Thomas Gleixner, linux-s390, x86,
Russell King - ARM Linux, Steven Price, Jason Gunthorpe,
linux-arm-kernel, linux-snps-arc, Kees Cook, Masahiro Yamada,
Mark Brown, Dan Williams, Vlastimil Babka, Sri Krishna chowdary,
Ard Biesheuvel, Greg Kroah-Hartman, linux-mips, Ralf Baechle,
linux-kernel, Paul Burton, Mike Rapoport, Vineet Gupta,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20190826131308.GA15933@bombadil.infradead.org>
On 08/26/2019 06:43 PM, Matthew Wilcox wrote:
> On Mon, Aug 26, 2019 at 08:07:13AM +0530, Anshuman Khandual wrote:
>> On 08/09/2019 07:22 PM, Matthew Wilcox wrote:
>>> On Fri, Aug 09, 2019 at 04:05:07PM +0530, Anshuman Khandual wrote:
>>>> On 08/09/2019 03:46 PM, Matthew Wilcox wrote:
>>>>> On Fri, Aug 09, 2019 at 01:03:17PM +0530, Anshuman Khandual wrote:
>>>>>> Should alloc_gigantic_page() be made available as an interface for general
>>>>>> use in the kernel. The test module here uses very similar implementation from
>>>>>> HugeTLB to allocate a PUD aligned memory block. Similar for mm_alloc() which
>>>>>> needs to be exported through a header.
>>>>>
>>>>> Why are you allocating memory at all instead of just using some
>>>>> known-to-exist PFNs like I suggested?
>>>>
>>>> We needed PFN to be PUD aligned for pfn_pud() and PMD aligned for mk_pmd().
>>>> Now walking the kernel page table for a known symbol like kernel_init()
>>>
>>> I didn't say to walk the kernel page table. I said to call virt_to_pfn()
>>> for a known symbol like kernel_init().
>>>
>>>> as you had suggested earlier we might encounter page table page entries at PMD
>>>> and PUD which might not be PMD or PUD aligned respectively. It seemed to me
>>>> that alignment requirement is applicable only for mk_pmd() and pfn_pud()
>>>> which create large mappings at those levels but that requirement does not
>>>> exist for page table pages pointing to next level. Is not that correct ? Or
>>>> I am missing something here ?
>>>
>>> Just clear the bottom bits off the PFN until you get a PMD or PUD aligned
>>> PFN. It's really not hard.
>>
>> As Mark pointed out earlier that might end up being just a synthetic PFN
>> which might not even exist on a given system.
>
> And why would that matter?
>
To start with the test uses struct page with mk_pte() and mk_pmd() while
pfn gets used in pfn_pud() during pXX_basic_tests(). So we will not be able
to derive a valid struct page from a synthetic pfn. Also if synthetic pfn is
going to be used anyway then why derive it from a real kernel symbol like
kernel_init(). Could not one be just made up with right alignment ?
Currently the test allocates 'mm_struct' and other page table pages from real
memory then why should it use synthetic pfn while creating actual page table
entries ? Couple of benefits going with synthetic pfn will be..
- It simplifies the test a bit removing PUD_SIZE allocation helpers
- It might enable the test to be run on systems without adequate memory
In the current proposal the allocation happens during boot making it much more
likely to succeed than not and when it fails, respective tests will be skipped.
I am just wondering if being able to run complete set of tests on smaller
systems with less memory weighs lot more in favor of going with synthetic
pfn instead.
^ permalink raw reply
* Re: [PATCH] powerpc/64: interrupt return in C
From: Christophe Leroy @ 2019-08-28 9:28 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20190828090606.5028-1-npiggin@gmail.com>
Le 28/08/2019 à 11:06, Nicholas Piggin a écrit :
> This is a work in progress that goes on top of the syscalls in C patch.
> It's not quite complete, 64e low level exit is not taken care of, and
> the new return is hacked into the existing interrupt handlers pretty
> quickly (e.g., full gprs handling is still ugly and could be cleaned),
> but that code touches exception-64s.S which is under heavy modification
> in parallel so I will rebase on top of that before polishing it properly.
> I will also try to convert to more IS_ENABLED() for Christophe.
I am flattered.
I guess at the end you are not doing it for me but because it is on
purpose in your code improvement process.
See
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
Christophe
^ permalink raw reply
* Re: [PATCH v2 3/4] powerpc/64: system call remove non-volatile GPR save optimisation
From: Nicholas Piggin @ 2019-08-28 9:32 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <3f91c62a-6d77-d34c-9477-3077afa5f4e8@c-s.fr>
Christophe Leroy's on August 28, 2019 7:02 pm:
>
>
> Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
>> powerpc has an optimisation where interrupts avoid saving the
>> non-volatile (or callee saved) registers to the interrupt stack frame if
>> they are not required.
>>
>> Two problems with this are that an interrupt does not always know
>> whether it will need non-volatiles; and if it does need them, they can
>> only be saved from the entry-scoped asm code (because we don't control
>> what the C compiler does with these registers).
>>
>> system calls are the most difficult: some system calls always require
>> all registers (e.g., fork, to copy regs into the child). Sometimes
>> registers are only required under certain conditions (e.g., tracing,
>> signal delivery). These cases require ugly logic in the call chains
>> (e.g., ppc_fork), and require a lot of logic to be implemented in asm.
>
> Do you really find it ugly to just call function nvgprs() before calling
> sys_fork() ? I guess there are things a lot uglier.
That's not the ugly part, the ugly part is trashing the link register
and then branching directly to where it was supposed to return, which
is bad for any CPU which has a return predictor so we try to eliminate
it from the ppc64 kernel.
>> So remove the optimisation for system calls, and always save NVGPRs on
>> entry. Modern high performance CPUs are not so sensitive, because the
>> stores are dense in cache and can be hidden by other expensive work in
>> the syscall path -- the null syscall selftests benchmark on POWER9 is
>> not slowed (124.40ns before and 123.64ns after, i.e., within the noise).
>
> I did the test on PPC32:
>
> On an 885, null_syscall reports 2227ns (132MHz)
> If saving non-volatile regs, it goes to 2419, ie +8.6%
>
> On an 8321, null_syscall reports 1021ns (333MHz)
> If saving non-volatile regs, it goes to 1100, ie +7.7%
>
> So unless going to C compensates this degradation, I guess it is not
> worth it on PPC32.
Yeah that's unfortunate. It is a good optimization for small cores.
I doubt going to C would help for PPC32, probably be even slower.
>>
>> -/* Save non-volatile GPRs, if not already saved. */
>> -_GLOBAL(save_nvgprs)
>> - ld r11,_TRAP(r1)
>> - andi. r0,r11,1
>> - beqlr-
>> - SAVE_NVGPRS(r1)
>> - clrrdi r0,r11,1
>> - std r0,_TRAP(r1)
>> - blr
>> -_ASM_NOKPROBE_SYMBOL(save_nvgprs);
>
> I see it is added back somewhere below. Why don't you leave it where it is ?
No longer used by syscalls so I it out from between other syscall
related code to improve icache.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v2 4/4] powerpc/64: system call implement the bulk of the logic in C
From: Nicholas Piggin @ 2019-08-28 9:41 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <1015bea4-8b51-ea3b-7e45-8825635261bb@c-s.fr>
Christophe Leroy's on August 28, 2019 4:51 pm:
>
>
> Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
>> -#include <asm/reg.h>
>> +#include <asm/mmu.h>
>> +#include <asm/ptrace.h>
>> +
>> +static inline void kuap_check_amr(void)
>> +{
>> +#ifdef CONFIG_PPC_KUAP_DEBUG
>> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
>
> Better:
>
> if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) &&
> mmu_has_feature(MMU_FTR_RADIX_KUAP))
That is better.
>> + WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
>> +#endif
>> +}
>>
>> /*
>> * We support individually allowing read or write, but we don't support nesting
>> diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
>> index 2431b4ada2fa..f3aa9db1a3cc 100644
>> --- a/arch/powerpc/include/asm/cputime.h
>> +++ b/arch/powerpc/include/asm/cputime.h
>> @@ -60,6 +60,28 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
>> }
>> #endif
>>
>> +static inline void account_cpu_user_entry(void)
>> +{
>> + unsigned long tb = mftb();
>> +
>> + get_accounting(current)->utime += (tb - get_accounting(current)->starttime_user);
>> + get_accounting(current)->starttime = tb;
>> +}
>
> Can you check the generated assembly ? I remember having bad result with
> get_accouting() being used several times in a arch_vtime_task_switch()
> before commit 60f1d2893ee6 ("powerpc/time: inline
> arch_vtime_task_switch()")
It's fine on 64s but it's accounting is a constant offset from r13 so
simple load/store can be done.
> Regardless, I think it would look better as:
>
> static inline void account_cpu_user_entry(void)
> {
> unsigned long tb = mftb();
> struct cpu_accounting_data *acct = get_accounting(current);
>
> acct->utime += (tb - acct->starttime_user);
> acct->starttime = tb;
> }
Yeah that's nicer.
>
>> +static inline void account_cpu_user_exit(void)
>> +{
>> + unsigned long tb = mftb();
>> +
>> + get_accounting(current)->stime += (tb - get_accounting(current)->starttime);
>> + get_accounting(current)->starttime_user = tb;
>> +}
>
> Same here.
Will do.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v2 0/4] powerpc/64: syscalls in C
From: Nicholas Piggin @ 2019-08-28 9:49 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <5ecd9d1a-d35e-dc8c-9ad4-a830a8b1a952@c-s.fr>
Christophe Leroy's on August 28, 2019 7:06 pm:
>
>
> Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
>> Accounted for some feedback.
>>
>> Nicholas Piggin (4):
>> powerpc: convert to copy_thread_tls
>> powerpc/64: remove support for kernel-mode syscalls
>> powerpc/64: system call remove non-volatile GPR save optimisation
>> powerpc/64: system call implement the bulk of the logic in C
>
> Would it be possible to split in the following parts:
>
> 1/ Implement in C whatever can be implemented without removing
> non-volatile GPR save optimisation
> 2/ Remove non-volatile GPR save optimisation
> 3/ Implement in C everything else
Hmm. I'll have a look but I would rather not go back and add the
intermediate state I was hoping to avoid. I'll think about it and
if it's not too difficult I will try to add something. I have an
idea.
With your nvregs performance test on ppc32, are you doing the
nvgpr restore? The fast path should be able to avoid that.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v2 0/4] powerpc/64: syscalls in C
From: Christophe Leroy @ 2019-08-28 9:55 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <1566985278.ehbnos9t6c.astroid@bobo.none>
Le 28/08/2019 à 11:49, Nicholas Piggin a écrit :
> Christophe Leroy's on August 28, 2019 7:06 pm:
>>
>>
>> Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
>>> Accounted for some feedback.
>>>
>>> Nicholas Piggin (4):
>>> powerpc: convert to copy_thread_tls
>>> powerpc/64: remove support for kernel-mode syscalls
>>> powerpc/64: system call remove non-volatile GPR save optimisation
>>> powerpc/64: system call implement the bulk of the logic in C
>>
>> Would it be possible to split in the following parts:
>>
>> 1/ Implement in C whatever can be implemented without removing
>> non-volatile GPR save optimisation
>> 2/ Remove non-volatile GPR save optimisation
>> 3/ Implement in C everything else
>
> Hmm. I'll have a look but I would rather not go back and add the
> intermediate state I was hoping to avoid. I'll think about it and
> if it's not too difficult I will try to add something. I have an
> idea.
>
> With your nvregs performance test on ppc32, are you doing the
> nvgpr restore? The fast path should be able to avoid that.
I only added the SAVE_NVGPRS call in the syscall entry macro just after
the saving of volatile regs, and changed the trap from \trapno+1 to \trapno
Christophe
^ permalink raw reply
* Re: [DOC][PATCH v2] powerpc: Provide initial documentation for PAPR hcalls
From: Nicholas Piggin @ 2019-08-28 9:54 UTC (permalink / raw)
To: linuxppc-dev, Vaibhav Jain
Cc: Laurent Dufour, Aneesh Kumar K . V, Oliver O'Halloran,
msuchanek, David Gibson
In-Reply-To: <20190828082729.16695-1-vaibhav@linux.ibm.com>
Vaibhav Jain's on August 28, 2019 6:27 pm:
> This doc patch provides an initial description of the hcall op-codes
> that are used by Linux kernel running as a guest (LPAR) on top of
> PowerVM or any other sPAPR compliant hyper-visor (e.g qemu).
>
> Apart from documenting the hcalls the doc-patch also provides a
> rudimentary overview of how hcall ABI, how they are issued with the
> Linux kernel and how information/control flows between the guest and
> hypervisor.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log:
>
> v2:
> * Added a section on Register conventions to be followed when invoking
> hcalls. [Nick]
> * Updated section on HCALL ABI to clarify that byte ordering only
> matters to in/out values passed by a memory buffer. [Laurent]
> * Updated a code comment in 'exceptions-64s.S' describing hypercall
> register conventions to point it to 'papr_hcalls.rst' [Nick]
Thanks for that!
Acked-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply
* Re: [PATCH v1 2/2] PCI/AER: Update parameter descriptions to satisfy kernel-doc validator
From: Andy Shevchenko @ 2019-08-28 10:07 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Sam Bobroff, linux-pci, Oliver O'Halloran, Bjorn Helgaas,
linuxppc-dev
In-Reply-To: <71eb8108-61a7-2815-4082-75c21f8bbf03@linux.intel.com>
On Tue, Aug 27, 2019 at 10:06:54AM -0700, Kuppuswamy Sathyanarayanan wrote:
>
> On 8/27/19 8:18 AM, Andy Shevchenko wrote:
> > Kernel-doc validator complains:
> >
> > aer.c:207: warning: Function parameter or member 'str' not described in 'pcie_ecrc_get_policy'
> > aer.c:1209: warning: Function parameter or member 'irq' not described in 'aer_isr'
> > aer.c:1209: warning: Function parameter or member 'context' not described in 'aer_isr'
> > aer.c:1209: warning: Excess function parameter 'work' description in 'aer_isr'
> >
> > Fix the above accordingly.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com>
Thanks!
JFYI: Keep your tag on one line. Some bots require this IIRC (patchwork).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2 0/4] Disable compat cruft on ppc64le v2
From: Michal Suchanek @ 2019-08-28 10:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Hari Bathini,
Michal Suchanek, Joel Stanley, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, Allison Randal, Greg Kroah-Hartman, linux-kernel,
Eric W. Biederman, Andrew Donnellan, linux-fsdevel, Andrew Morton,
Alexander Viro
With endian switch disabled by default the ppc64le compat supports
ppc32le only which is something next to nobody has binaries for.
Less code means less bugs so drop the compat stuff.
I am not particularly sure about the best way to resolve the llseek
situation. I don't see anything in the syscal tables making it
32bit-only so I suppose it should be available on 64bit as well.
This is tested on ppc64le top of
https://patchwork.ozlabs.org/cover/1153556/
Changes in v2: saner CONFIG_COMPAT ifdefs
Thanks
Michal
Michal Suchanek (4):
fs: always build llseek.
powerpc: move common register copy functions from signal_32.c to
signal.c
powerpc/64: make buildable without CONFIG_COMPAT
powerpc/64: Disable COMPAT if littleendian.
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/syscall.h | 2 +
arch/powerpc/kernel/Makefile | 15 ++-
arch/powerpc/kernel/entry_64.S | 2 +
arch/powerpc/kernel/signal.c | 146 ++++++++++++++++++++++++++++-
arch/powerpc/kernel/signal_32.c | 140 ---------------------------
arch/powerpc/kernel/syscall_64.c | 5 +-
arch/powerpc/kernel/vdso.c | 4 +-
arch/powerpc/perf/callchain.c | 14 ++-
fs/read_write.c | 2 -
10 files changed, 177 insertions(+), 155 deletions(-)
--
2.22.0
^ permalink raw reply
* [PATCH v2 1/4] fs: always build llseek.
From: Michal Suchanek @ 2019-08-28 10:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Hari Bathini,
Michal Suchanek, Joel Stanley, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, Allison Randal, Greg Kroah-Hartman, linux-kernel,
Eric W. Biederman, Andrew Donnellan, linux-fsdevel, Andrew Morton,
Alexander Viro
In-Reply-To: <cover.1566987936.git.msuchanek@suse.de>
64bit !COMPAT does not build because the llseek syscall is in the tables.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
fs/read_write.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 5bbf587f5bc1..9db56931eb26 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -331,7 +331,6 @@ COMPAT_SYSCALL_DEFINE3(lseek, unsigned int, fd, compat_off_t, offset, unsigned i
}
#endif
-#if !defined(CONFIG_64BIT) || defined(CONFIG_COMPAT)
SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
unsigned long, offset_low, loff_t __user *, result,
unsigned int, whence)
@@ -360,7 +359,6 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
fdput_pos(f);
return retval;
}
-#endif
int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
{
--
2.22.0
^ permalink raw reply related
* [PATCH v2 2/4] powerpc: move common register copy functions from signal_32.c to signal.c
From: Michal Suchanek @ 2019-08-28 10:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Hari Bathini,
Michal Suchanek, Joel Stanley, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, Allison Randal, Greg Kroah-Hartman, linux-kernel,
Eric W. Biederman, Andrew Donnellan, linux-fsdevel, Andrew Morton,
Alexander Viro
In-Reply-To: <cover.1566987936.git.msuchanek@suse.de>
These functions are required for 64bit as well.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
arch/powerpc/kernel/signal.c | 141 ++++++++++++++++++++++++++++++++
arch/powerpc/kernel/signal_32.c | 140 -------------------------------
2 files changed, 141 insertions(+), 140 deletions(-)
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e6c30cee6abf..60436432399f 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -18,12 +18,153 @@
#include <linux/syscalls.h>
#include <asm/hw_breakpoint.h>
#include <linux/uaccess.h>
+#include <asm/switch_to.h>
#include <asm/unistd.h>
#include <asm/debug.h>
#include <asm/tm.h>
#include "signal.h"
+#ifdef CONFIG_VSX
+unsigned long copy_fpr_to_user(void __user *to,
+ struct task_struct *task)
+{
+ u64 buf[ELF_NFPREG];
+ int i;
+
+ /* save FPR copy to local buffer then write to the thread_struct */
+ for (i = 0; i < (ELF_NFPREG - 1) ; i++)
+ buf[i] = task->thread.TS_FPR(i);
+ buf[i] = task->thread.fp_state.fpscr;
+ return __copy_to_user(to, buf, ELF_NFPREG * sizeof(double));
+}
+
+unsigned long copy_fpr_from_user(struct task_struct *task,
+ void __user *from)
+{
+ u64 buf[ELF_NFPREG];
+ int i;
+
+ if (__copy_from_user(buf, from, ELF_NFPREG * sizeof(double)))
+ return 1;
+ for (i = 0; i < (ELF_NFPREG - 1) ; i++)
+ task->thread.TS_FPR(i) = buf[i];
+ task->thread.fp_state.fpscr = buf[i];
+
+ return 0;
+}
+
+unsigned long copy_vsx_to_user(void __user *to,
+ struct task_struct *task)
+{
+ u64 buf[ELF_NVSRHALFREG];
+ int i;
+
+ /* save FPR copy to local buffer then write to the thread_struct */
+ for (i = 0; i < ELF_NVSRHALFREG; i++)
+ buf[i] = task->thread.fp_state.fpr[i][TS_VSRLOWOFFSET];
+ return __copy_to_user(to, buf, ELF_NVSRHALFREG * sizeof(double));
+}
+
+unsigned long copy_vsx_from_user(struct task_struct *task,
+ void __user *from)
+{
+ u64 buf[ELF_NVSRHALFREG];
+ int i;
+
+ if (__copy_from_user(buf, from, ELF_NVSRHALFREG * sizeof(double)))
+ return 1;
+ for (i = 0; i < ELF_NVSRHALFREG ; i++)
+ task->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];
+ return 0;
+}
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+unsigned long copy_ckfpr_to_user(void __user *to,
+ struct task_struct *task)
+{
+ u64 buf[ELF_NFPREG];
+ int i;
+
+ /* save FPR copy to local buffer then write to the thread_struct */
+ for (i = 0; i < (ELF_NFPREG - 1) ; i++)
+ buf[i] = task->thread.TS_CKFPR(i);
+ buf[i] = task->thread.ckfp_state.fpscr;
+ return __copy_to_user(to, buf, ELF_NFPREG * sizeof(double));
+}
+
+unsigned long copy_ckfpr_from_user(struct task_struct *task,
+ void __user *from)
+{
+ u64 buf[ELF_NFPREG];
+ int i;
+
+ if (__copy_from_user(buf, from, ELF_NFPREG * sizeof(double)))
+ return 1;
+ for (i = 0; i < (ELF_NFPREG - 1) ; i++)
+ task->thread.TS_CKFPR(i) = buf[i];
+ task->thread.ckfp_state.fpscr = buf[i];
+
+ return 0;
+}
+
+unsigned long copy_ckvsx_to_user(void __user *to,
+ struct task_struct *task)
+{
+ u64 buf[ELF_NVSRHALFREG];
+ int i;
+
+ /* save FPR copy to local buffer then write to the thread_struct */
+ for (i = 0; i < ELF_NVSRHALFREG; i++)
+ buf[i] = task->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET];
+ return __copy_to_user(to, buf, ELF_NVSRHALFREG * sizeof(double));
+}
+
+unsigned long copy_ckvsx_from_user(struct task_struct *task,
+ void __user *from)
+{
+ u64 buf[ELF_NVSRHALFREG];
+ int i;
+
+ if (__copy_from_user(buf, from, ELF_NVSRHALFREG * sizeof(double)))
+ return 1;
+ for (i = 0; i < ELF_NVSRHALFREG ; i++)
+ task->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];
+ return 0;
+}
+#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+#else
+inline unsigned long copy_fpr_to_user(void __user *to,
+ struct task_struct *task)
+{
+ return __copy_to_user(to, task->thread.fp_state.fpr,
+ ELF_NFPREG * sizeof(double));
+}
+
+inline unsigned long copy_fpr_from_user(struct task_struct *task,
+ void __user *from)
+{
+ return __copy_from_user(task->thread.fp_state.fpr, from,
+ ELF_NFPREG * sizeof(double));
+}
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+inline unsigned long copy_ckfpr_to_user(void __user *to,
+ struct task_struct *task)
+{
+ return __copy_to_user(to, task->thread.ckfp_state.fpr,
+ ELF_NFPREG * sizeof(double));
+}
+
+inline unsigned long copy_ckfpr_from_user(struct task_struct *task,
+ void __user *from)
+{
+ return __copy_from_user(task->thread.ckfp_state.fpr, from,
+ ELF_NFPREG * sizeof(double));
+}
+#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+#endif
+
/* Log an error when sending an unhandled signal to a process. Controlled
* through debug.exception-trace sysctl.
*/
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 98600b276f76..c93c937ea568 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -235,146 +235,6 @@ struct rt_sigframe {
int abigap[56];
};
-#ifdef CONFIG_VSX
-unsigned long copy_fpr_to_user(void __user *to,
- struct task_struct *task)
-{
- u64 buf[ELF_NFPREG];
- int i;
-
- /* save FPR copy to local buffer then write to the thread_struct */
- for (i = 0; i < (ELF_NFPREG - 1) ; i++)
- buf[i] = task->thread.TS_FPR(i);
- buf[i] = task->thread.fp_state.fpscr;
- return __copy_to_user(to, buf, ELF_NFPREG * sizeof(double));
-}
-
-unsigned long copy_fpr_from_user(struct task_struct *task,
- void __user *from)
-{
- u64 buf[ELF_NFPREG];
- int i;
-
- if (__copy_from_user(buf, from, ELF_NFPREG * sizeof(double)))
- return 1;
- for (i = 0; i < (ELF_NFPREG - 1) ; i++)
- task->thread.TS_FPR(i) = buf[i];
- task->thread.fp_state.fpscr = buf[i];
-
- return 0;
-}
-
-unsigned long copy_vsx_to_user(void __user *to,
- struct task_struct *task)
-{
- u64 buf[ELF_NVSRHALFREG];
- int i;
-
- /* save FPR copy to local buffer then write to the thread_struct */
- for (i = 0; i < ELF_NVSRHALFREG; i++)
- buf[i] = task->thread.fp_state.fpr[i][TS_VSRLOWOFFSET];
- return __copy_to_user(to, buf, ELF_NVSRHALFREG * sizeof(double));
-}
-
-unsigned long copy_vsx_from_user(struct task_struct *task,
- void __user *from)
-{
- u64 buf[ELF_NVSRHALFREG];
- int i;
-
- if (__copy_from_user(buf, from, ELF_NVSRHALFREG * sizeof(double)))
- return 1;
- for (i = 0; i < ELF_NVSRHALFREG ; i++)
- task->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];
- return 0;
-}
-
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-unsigned long copy_ckfpr_to_user(void __user *to,
- struct task_struct *task)
-{
- u64 buf[ELF_NFPREG];
- int i;
-
- /* save FPR copy to local buffer then write to the thread_struct */
- for (i = 0; i < (ELF_NFPREG - 1) ; i++)
- buf[i] = task->thread.TS_CKFPR(i);
- buf[i] = task->thread.ckfp_state.fpscr;
- return __copy_to_user(to, buf, ELF_NFPREG * sizeof(double));
-}
-
-unsigned long copy_ckfpr_from_user(struct task_struct *task,
- void __user *from)
-{
- u64 buf[ELF_NFPREG];
- int i;
-
- if (__copy_from_user(buf, from, ELF_NFPREG * sizeof(double)))
- return 1;
- for (i = 0; i < (ELF_NFPREG - 1) ; i++)
- task->thread.TS_CKFPR(i) = buf[i];
- task->thread.ckfp_state.fpscr = buf[i];
-
- return 0;
-}
-
-unsigned long copy_ckvsx_to_user(void __user *to,
- struct task_struct *task)
-{
- u64 buf[ELF_NVSRHALFREG];
- int i;
-
- /* save FPR copy to local buffer then write to the thread_struct */
- for (i = 0; i < ELF_NVSRHALFREG; i++)
- buf[i] = task->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET];
- return __copy_to_user(to, buf, ELF_NVSRHALFREG * sizeof(double));
-}
-
-unsigned long copy_ckvsx_from_user(struct task_struct *task,
- void __user *from)
-{
- u64 buf[ELF_NVSRHALFREG];
- int i;
-
- if (__copy_from_user(buf, from, ELF_NVSRHALFREG * sizeof(double)))
- return 1;
- for (i = 0; i < ELF_NVSRHALFREG ; i++)
- task->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];
- return 0;
-}
-#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
-#else
-inline unsigned long copy_fpr_to_user(void __user *to,
- struct task_struct *task)
-{
- return __copy_to_user(to, task->thread.fp_state.fpr,
- ELF_NFPREG * sizeof(double));
-}
-
-inline unsigned long copy_fpr_from_user(struct task_struct *task,
- void __user *from)
-{
- return __copy_from_user(task->thread.fp_state.fpr, from,
- ELF_NFPREG * sizeof(double));
-}
-
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-inline unsigned long copy_ckfpr_to_user(void __user *to,
- struct task_struct *task)
-{
- return __copy_to_user(to, task->thread.ckfp_state.fpr,
- ELF_NFPREG * sizeof(double));
-}
-
-inline unsigned long copy_ckfpr_from_user(struct task_struct *task,
- void __user *from)
-{
- return __copy_from_user(task->thread.ckfp_state.fpr, from,
- ELF_NFPREG * sizeof(double));
-}
-#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
-#endif
-
/*
* Save the current user registers on the user stack.
* We only save the altivec/spe registers if the process has used
--
2.22.0
^ permalink raw reply related
* [PATCH v2 3/4] powerpc/64: make buildable without CONFIG_COMPAT
From: Michal Suchanek @ 2019-08-28 10:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Hari Bathini,
Michal Suchanek, Joel Stanley, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, Allison Randal, Greg Kroah-Hartman, linux-kernel,
Eric W. Biederman, Andrew Donnellan, linux-fsdevel, Andrew Morton,
Alexander Viro
In-Reply-To: <cover.1566987936.git.msuchanek@suse.de>
There are numerous references to 32bit functions in generic and 64bit
code so ifdef them out.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v2:
- fix 32bit ifdef condition in signal.c
- simplify the compat ifdef condition in vdso.c - 64bit is redundant
- simplify the compat ifdef condition in callchain.c - 64bit is redundant
---
arch/powerpc/include/asm/syscall.h | 2 ++
arch/powerpc/kernel/Makefile | 15 ++++++++++++---
arch/powerpc/kernel/entry_64.S | 2 ++
arch/powerpc/kernel/signal.c | 5 +++--
arch/powerpc/kernel/syscall_64.c | 5 +++--
arch/powerpc/kernel/vdso.c | 4 +++-
arch/powerpc/perf/callchain.c | 14 ++++++++++----
7 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index 38d62acfdce7..3ed3b75541a1 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -16,7 +16,9 @@
/* ftrace syscalls requires exporting the sys_call_table */
extern const unsigned long sys_call_table[];
+#ifdef CONFIG_COMPAT
extern const unsigned long compat_sys_call_table[];
+#endif
static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
{
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 1d646a94d96c..b0db365b83d8 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -44,16 +44,25 @@ CFLAGS_btext.o += -DDISABLE_BRANCH_PROFILING
endif
obj-y := cputable.o ptrace.o syscalls.o \
- irq.o align.o signal_32.o pmc.o vdso.o \
+ irq.o align.o pmc.o vdso.o \
process.o systbl.o idle.o \
signal.o sysfs.o cacheinfo.o time.o \
prom.o traps.o setup-common.o \
udbg.o misc.o io.o misc_$(BITS).o \
of_platform.o prom_parse.o
-obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \
- signal_64.o ptrace32.o \
+ifndef CONFIG_PPC64
+obj-y += signal_32.o
+else
+ifdef CONFIG_COMPAT
+obj-y += signal_32.o
+endif
+endif
+obj-$(CONFIG_PPC64) += setup_64.o signal_64.o \
paca.o nvram_64.o firmware.o \
syscall_64.o
+ifdef CONFIG_COMPAT
+obj-$(CONFIG_PPC64) += sys_ppc32.o ptrace32.o
+endif
obj-$(CONFIG_VDSO32) += vdso32/
obj-$(CONFIG_PPC_WATCHDOG) += watchdog.o
obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 2ec825a85f5b..a2dbf216f607 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -51,8 +51,10 @@
SYS_CALL_TABLE:
.tc sys_call_table[TC],sys_call_table
+#ifdef CONFIG_COMPAT
COMPAT_SYS_CALL_TABLE:
.tc compat_sys_call_table[TC],compat_sys_call_table
+#endif
/* This value is used to mark exception frames on the stack. */
exception_marker:
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 60436432399f..ffd045e9fb57 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -277,14 +277,15 @@ static void do_signal(struct task_struct *tsk)
rseq_signal_deliver(&ksig, tsk->thread.regs);
+#if !defined(CONFIG_PPC64) || defined(CONFIG_COMPAT)
if (is32) {
if (ksig.ka.sa.sa_flags & SA_SIGINFO)
ret = handle_rt_signal32(&ksig, oldset, tsk);
else
ret = handle_signal32(&ksig, oldset, tsk);
- } else {
+ } else
+#endif /* 32bit */
ret = handle_rt_signal64(&ksig, oldset, tsk);
- }
tsk->thread.regs->trap = 0;
signal_setup_done(ret, &ksig, test_thread_flag(TIF_SINGLESTEP));
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 98ed970796d5..3f48262b512d 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -100,6 +100,7 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
/* May be faster to do array_index_nospec? */
barrier_nospec();
+#ifdef CONFIG_COMPAT
if (unlikely(ti_flags & _TIF_32BIT)) {
f = (void *)compat_sys_call_table[r0];
@@ -110,9 +111,9 @@ long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
r7 &= 0x00000000ffffffffULL;
r8 &= 0x00000000ffffffffULL;
- } else {
+ } else
+#endif /* CONFIG_COMPAT */
f = (void *)sys_call_table[r0];
- }
return f(r3, r4, r5, r6, r7, r8);
}
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index d60598113a9f..a991b5d69010 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -667,7 +667,7 @@ static void __init vdso_setup_syscall_map(void)
{
unsigned int i;
extern unsigned long *sys_call_table;
-#ifdef CONFIG_PPC64
+#ifdef CONFIG_COMPAT
extern unsigned long *compat_sys_call_table;
#endif
extern unsigned long sys_ni_syscall;
@@ -678,9 +678,11 @@ static void __init vdso_setup_syscall_map(void)
if (sys_call_table[i] != sys_ni_syscall)
vdso_data->syscall_map_64[i >> 5] |=
0x80000000UL >> (i & 0x1f);
+#ifdef CONFIG_COMPAT
if (compat_sys_call_table[i] != sys_ni_syscall)
vdso_data->syscall_map_32[i >> 5] |=
0x80000000UL >> (i & 0x1f);
+#endif /* CONFIG_COMPAT */
#else /* CONFIG_PPC64 */
if (sys_call_table[i] != sys_ni_syscall)
vdso_data->syscall_map_32[i >> 5] |=
diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index c84bbd4298a0..b3dacc8bc98d 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -15,7 +15,7 @@
#include <asm/sigcontext.h>
#include <asm/ucontext.h>
#include <asm/vdso.h>
-#ifdef CONFIG_PPC64
+#ifdef CONFIG_COMPAT
#include "../kernel/ppc32.h"
#endif
#include <asm/pte-walk.h>
@@ -165,6 +165,7 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
return read_user_stack_slow(ptr, ret, 8);
}
+#ifdef CONFIG_COMPAT
static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
{
if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
@@ -180,6 +181,7 @@ static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
return read_user_stack_slow(ptr, ret, 4);
}
+#endif
static inline int valid_user_sp(unsigned long sp, int is_64)
{
@@ -341,6 +343,7 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
#endif /* CONFIG_PPC64 */
+#if !defined(CONFIG_PPC64) || defined(CONFIG_COMPAT)
/*
* Layout for non-RT signal frames
*/
@@ -482,12 +485,15 @@ static void perf_callchain_user_32(struct perf_callchain_entry_ctx *entry,
sp = next_sp;
}
}
+#endif /* 32bit */
void
perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
{
- if (current_is_64bit())
- perf_callchain_user_64(entry, regs);
- else
+#if !defined(CONFIG_PPC64) || defined(CONFIG_COMPAT)
+ if (!current_is_64bit())
perf_callchain_user_32(entry, regs);
+ else
+#endif
+ perf_callchain_user_64(entry, regs);
}
--
2.22.0
^ permalink raw reply related
* [PATCH v2 4/4] powerpc/64: Disable COMPAT if littleendian.
From: Michal Suchanek @ 2019-08-28 10:30 UTC (permalink / raw)
To: linuxppc-dev
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Hari Bathini,
Michal Suchanek, Joel Stanley, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, Allison Randal, Greg Kroah-Hartman, linux-kernel,
Eric W. Biederman, Andrew Donnellan, linux-fsdevel, Andrew Morton,
Alexander Viro
In-Reply-To: <cover.1566987936.git.msuchanek@suse.de>
ppc32le was never really a thing. Endian swap is already disabled by
default so this 32bit support is kind of useless on ppc64le.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
arch/powerpc/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 5bab0bb6b833..67cd1c4d1bae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -265,7 +265,7 @@ config PANIC_TIMEOUT
config COMPAT
bool
- default y if PPC64
+ default y if PPC64 && !CPU_LITTLE_ENDIAN
select COMPAT_BINFMT_ELF
select ARCH_WANT_OLD_COMPAT_IPC
select COMPAT_OLD_SIGACTION
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v2 0/4] Disable compat cruft on ppc64le v2
From: Nicholas Piggin @ 2019-08-28 10:57 UTC (permalink / raw)
To: linuxppc-dev, Michal Suchanek
Cc: David Hildenbrand, Dmitry V. Levin, Max Filippov, Paul Mackerras,
Breno Leitao, Michael Neuling, Firoz Khan, Allison Randal,
Joel Stanley, Steven Rostedt, Alexander Viro, Thomas Gleixner,
Hari Bathini, Greg Kroah-Hartman, linux-kernel, Eric W. Biederman,
Andrew Donnellan, linux-fsdevel, Andrew Morton
In-Reply-To: <cover.1566987936.git.msuchanek@suse.de>
Michal Suchanek's on August 28, 2019 8:30 pm:
> With endian switch disabled by default the ppc64le compat supports
> ppc32le only which is something next to nobody has binaries for.
>
> Less code means less bugs so drop the compat stuff.
Interesting patches, thanks for looking into it. I don't know much
about compat and wrong endian userspaces. I think sys_switch_endian
is enabled though, it's just a strange fast endian swap thing that
has been disabled by default.
The first patches look pretty good. Maybe for the last one it could
become a selectable option?
> I am not particularly sure about the best way to resolve the llseek
> situation. I don't see anything in the syscal tables making it
> 32bit-only so I suppose it should be available on 64bit as well.
It's for 32-bit userspace only. Can we just get rid of it, or is
there some old broken 64-bit BE userspace that tries to call it?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v6 06/12] powerpc/fsl_booke/32: implement KASLR infrastructure
From: Jason Yan @ 2019-08-28 11:03 UTC (permalink / raw)
To: Scott Wood
Cc: wangkefeng.wang, keescook, kernel-hardening, linux-kernel,
npiggin, jingxiangfeng, diana.craciun, paulus, zhaohongjiang,
thunder.leizhen, fanchengyang, linuxppc-dev, yebin10
In-Reply-To: <20190828045454.GB17757@home.buserror.net>
On 2019/8/28 12:54, Scott Wood wrote:
> On Fri, Aug 09, 2019 at 06:07:54PM +0800, Jason Yan wrote:
>> This patch add support to boot kernel from places other than KERNELBASE.
>> Since CONFIG_RELOCATABLE has already supported, what we need to do is
>> map or copy kernel to a proper place and relocate. Freescale Book-E
>> parts expect lowmem to be mapped by fixed TLB entries(TLB1). The TLB1
>> entries are not suitable to map the kernel directly in a randomized
>> region, so we chose to copy the kernel to a proper place and restart to
>> relocate.
>>
>> The offset of the kernel was not randomized yet(a fixed 64M is set). We
>> will randomize it in the next patch.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Cc: Diana Craciun <diana.craciun@nxp.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Tested-by: Diana Craciun <diana.craciun@nxp.com>
>> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/Kconfig | 11 ++++
>> arch/powerpc/kernel/Makefile | 1 +
>> arch/powerpc/kernel/early_32.c | 2 +-
>> arch/powerpc/kernel/fsl_booke_entry_mapping.S | 17 +++--
>> arch/powerpc/kernel/head_fsl_booke.S | 13 +++-
>> arch/powerpc/kernel/kaslr_booke.c | 62 +++++++++++++++++++
>> arch/powerpc/mm/mmu_decl.h | 7 +++
>> arch/powerpc/mm/nohash/fsl_booke.c | 7 ++-
>> 8 files changed, 105 insertions(+), 15 deletions(-)
>> create mode 100644 arch/powerpc/kernel/kaslr_booke.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 77f6ebf97113..710c12ef7159 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -548,6 +548,17 @@ config RELOCATABLE
>> setting can still be useful to bootwrappers that need to know the
>> load address of the kernel (eg. u-boot/mkimage).
>>
>> +config RANDOMIZE_BASE
>> + bool "Randomize the address of the kernel image"
>> + depends on (FSL_BOOKE && FLATMEM && PPC32)
>> + depends on RELOCATABLE
>> + help
>> + Randomizes the virtual address at which the kernel image is
>> + loaded, as a security feature that deters exploit attempts
>> + relying on knowledge of the location of kernel internals.
>> +
>> + If unsure, say N.
>> +
>
> Why is N the safe default (other than concerns about code maturity,
> though arm64 and mips don't seem to have updated this recommendation
> after several years)? On x86 this defaults to Y.
>
Actually I would like to set this default Y. I was just wondering if
people like this feature or not at the beginning so I had to be more
careful.
>> diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
>> index f4d3eaae54a9..641920d4f694 100644
>> --- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
>> +++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
>> @@ -155,23 +155,22 @@ skpinv: addi r6,r6,1 /* Increment */
>>
>> #if defined(ENTRY_MAPPING_BOOT_SETUP)
>>
>> -/* 6. Setup KERNELBASE mapping in TLB1[0] */
>> +/* 6. Setup kernstart_virt_addr mapping in TLB1[0] */
>> lis r6,0x1000 /* Set MAS0(TLBSEL) = TLB1(1), ESEL = 0 */
>> mtspr SPRN_MAS0,r6
>> lis r6,(MAS1_VALID|MAS1_IPROT)@h
>> ori r6,r6,(MAS1_TSIZE(BOOK3E_PAGESZ_64M))@l
>> mtspr SPRN_MAS1,r6
>> - lis r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, MAS2_M_IF_NEEDED)@h
>> - ori r6,r6,MAS2_VAL(PAGE_OFFSET, BOOK3E_PAGESZ_64M, MAS2_M_IF_NEEDED)@l
>> - mtspr SPRN_MAS2,r6
>> + lis r6,MAS2_EPN_MASK(BOOK3E_PAGESZ_64M)@h
>> + ori r6,r6,MAS2_EPN_MASK(BOOK3E_PAGESZ_64M)@l
>> + and r6,r6,r20
>> + ori r6,r6,MAS2_M_IF_NEEDED@l
>> + mtspr SPRN_MAS2,r6
>
> Please use tabs rather than spaces between the mnemonic and the
> arguments.
>
> It looks like that was the last user of MAS2_VAL so let's remove it.
>
OK.
>> diff --git a/arch/powerpc/kernel/kaslr_booke.c b/arch/powerpc/kernel/kaslr_booke.c
>> new file mode 100644
>> index 000000000000..f8dc60534ac1
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/kaslr_booke.c
>
> Shouldn't this go under arch/powerpc/mm/nohash?
>
>> +/*
>> + * To see if we need to relocate the kernel to a random offset
>> + * void *dt_ptr - address of the device tree
>> + * phys_addr_t size - size of the first memory block
>> + */
>> +notrace void __init kaslr_early_init(void *dt_ptr, phys_addr_t size)
>> +{
>> + unsigned long tlb_virt;
>> + phys_addr_t tlb_phys;
>> + unsigned long offset;
>> + unsigned long kernel_sz;
>> +
>> + kernel_sz = (unsigned long)_end - KERNELBASE;
>
> Why KERNELBASE and not kernstart_addr?
>
Did you mean kernstart_virt_addr? It should be kernstart_virt_addr.
>> +
>> + offset = kaslr_choose_location(dt_ptr, size, kernel_sz);
>> +
>> + if (offset == 0)
>> + return;
>> +
>> + kernstart_virt_addr += offset;
>> + kernstart_addr += offset;
>> +
>> + is_second_reloc = 1;
>> +
>> + if (offset >= SZ_64M) {
>> + tlb_virt = round_down(kernstart_virt_addr, SZ_64M);
>> + tlb_phys = round_down(kernstart_addr, SZ_64M);
>
> If kernstart_addr wasn't 64M-aligned before adding offset, then "offset
>> = SZ_64M" is not necessarily going to detect when you've crossed a
> mapping boundary.
> >> +
>> + /* Create kernel map to relocate in */
>> + create_tlb_entry(tlb_phys, tlb_virt, 1);
>> + }
>> +
>> + /* Copy the kernel to it's new location and run */
>> + memcpy((void *)kernstart_virt_addr, (void *)KERNELBASE, kernel_sz);
>> +
>> + reloc_kernel_entry(dt_ptr, kernstart_virt_addr);
>> +}
>
> After copying, call flush_icache_range() on the destination.
>
OK
>> diff --git a/arch/powerpc/mm/nohash/fsl_booke.c b/arch/powerpc/mm/nohash/fsl_booke.c
>> index 556e3cd52a35..2dc27cf88add 100644
>> --- a/arch/powerpc/mm/nohash/fsl_booke.c
>> +++ b/arch/powerpc/mm/nohash/fsl_booke.c
>> @@ -263,7 +263,8 @@ void setup_initial_memory_limit(phys_addr_t first_memblock_base,
>> int __initdata is_second_reloc;
>> notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
>> {
>> - unsigned long base = KERNELBASE;
>> + unsigned long base = kernstart_virt_addr;
>> + phys_addr_t size;
>>
>> kernstart_addr = start;
>> if (is_second_reloc) {
>> @@ -291,7 +292,7 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
>> start &= ~0x3ffffff;
>> base &= ~0x3ffffff;
>> virt_phys_offset = base - start;
>> - early_get_first_memblock_info(__va(dt_ptr), NULL);
>> + early_get_first_memblock_info(__va(dt_ptr), &size);
>> /*
>> * We now get the memstart_addr, then we should check if this
>> * address is the same as what the PAGE_OFFSET map to now. If
>> @@ -316,6 +317,8 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
>> /* We should never reach here */
>> panic("Relocation error");
>> }
>> +
>> + kaslr_early_init(__va(dt_ptr), size);
>
> Are you assuming that available memory starts at physical address zero?
> This isn't true of some partitioning scenarios, or in a kdump crash
> kernel.
>
I'm not assuming that but I haven't tested that case for now. I will
reconsider and test these scenarios and fix all bugs.
> -Scott
>
> .
>
^ permalink raw reply
* Re: [PATCH] powerpc/64: interrupt return in C
From: Nicholas Piggin @ 2019-08-28 11:56 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <33052597-ce23-780d-bb38-f50ecd78ff3a@c-s.fr>
Christophe Leroy's on August 28, 2019 7:28 pm:
>
>
> Le 28/08/2019 à 11:06, Nicholas Piggin a écrit :
>> This is a work in progress that goes on top of the syscalls in C patch.
>> It's not quite complete, 64e low level exit is not taken care of, and
>> the new return is hacked into the existing interrupt handlers pretty
>> quickly (e.g., full gprs handling is still ugly and could be cleaned),
>> but that code touches exception-64s.S which is under heavy modification
>> in parallel so I will rebase on top of that before polishing it properly.
>> I will also try to convert to more IS_ENABLED() for Christophe.
>
> I am flattered.
>
> I guess at the end you are not doing it for me but because it is on
> purpose in your code improvement process.
Heh :) Yeah I agree it's almost always nicer code. Sometimes I just
forget about it so I appreciate you pointing out improvements.
Thanks,
Nick
^ permalink raw reply
* [PATCH] sefltest/ima: support appended signatures (modsig)
From: Mimi Zohar @ 2019-08-28 12:39 UTC (permalink / raw)
To: linux-integrity
Cc: Dave Young, linuxppc-dev, Mimi Zohar, linux-kernel, Petr Vorel,
linux-kselftest, Jessica Yu, shuah, Thiago Jung Bauermann
Detect and allow appended signatures.
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
.../selftests/kexec/test_kexec_file_load.sh | 38 +++++++++++++++++++---
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kexec/test_kexec_file_load.sh b/tools/testing/selftests/kexec/test_kexec_file_load.sh
index fa7c24e8eefb..2ff600388c30 100755
--- a/tools/testing/selftests/kexec/test_kexec_file_load.sh
+++ b/tools/testing/selftests/kexec/test_kexec_file_load.sh
@@ -37,11 +37,20 @@ is_ima_sig_required()
# sequentially. As a result, a policy rule may be defined, but
# might not necessarily be used. This test assumes if a policy
# rule is specified, that is the intent.
+
+ # First check for appended signature (modsig), then xattr
if [ $ima_read_policy -eq 1 ]; then
check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
- "appraise_type=imasig"
+ "appraise_type=imasig|modsig"
ret=$?
- [ $ret -eq 1 ] && log_info "IMA signature required";
+ if [ $ret -eq 1 ]; then
+ log_info "IMA or appended(modsig) signature required"
+ else
+ check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
+ "appraise_type=imasig"
+ ret=$?
+ [ $ret -eq 1 ] && log_info "IMA signature required";
+ fi
fi
return $ret
}
@@ -84,6 +93,22 @@ check_for_imasig()
return $ret
}
+# Return 1 for appended signature (modsig) found and 0 for not found.
+check_for_modsig()
+{
+ local module_sig_string="~Module signature appended~"
+ local sig="$(tail --bytes $((${#module_sig_string} + 1)) $KERNEL_IMAGE)"
+ local ret=0
+
+ if [ "$sig" == "$module_sig_string" ]; then
+ ret=1
+ log_info "kexec kernel image modsig signed"
+ else
+ log_info "kexec kernel image not modsig signed"
+ fi
+ return $ret
+}
+
kexec_file_load_test()
{
local succeed_msg="kexec_file_load succeeded"
@@ -98,7 +123,8 @@ kexec_file_load_test()
# In secureboot mode with an architecture specific
# policy, make sure either an IMA or PE signature exists.
if [ $secureboot -eq 1 ] && [ $arch_policy -eq 1 ] && \
- [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ]; then
+ [ $ima_signed -eq 0 ] && [ $pe_signed -eq 0 ] \
+ && [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing sig)"
fi
@@ -107,7 +133,8 @@ kexec_file_load_test()
log_fail "$succeed_msg (missing PE sig)"
fi
- if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+ if [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ] \
+ && [ $ima_modsig -eq 0 ]; then
log_fail "$succeed_msg (missing IMA sig)"
fi
@@ -204,5 +231,8 @@ pe_signed=$?
check_for_imasig
ima_signed=$?
+check_for_modsig
+ima_modsig=$?
+
# Test loading the kernel image via kexec_file_load syscall
kexec_file_load_test
--
2.7.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox