| Message ID | 20221213140724.8612-3-quic_sibis@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix XPU violation during modem metadata authentication | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 13/12/2022 15:07, Sibi Sankar wrote: > Update the bindings to reflect the addition of the new modem metadata > carveout reference to the memory-region property. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 3 ++- > .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > index e4a7da8020f4..b1402bef0ebe 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml > @@ -95,6 +95,7 @@ properties: > items: > - description: MBA reserved region > - description: modem reserved region > + - description: metadata reserved region Which makes the third item now required, also for all out of tree DTS and other users of the bindings. Please write a bit more in commit msg why this is necessary (e.g. was it broken before?). I assume the driver does not break the ABI? Best regards, Krzysztof
On 12/14/22 01:11, Krzysztof Kozlowski wrote: > On 13/12/2022 15:07, Sibi Sankar wrote: >> Update the bindings to reflect the addition of the new modem metadata >> carveout reference to the memory-region property. >> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >> --- >> .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 3 ++- >> .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 3 ++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >> index e4a7da8020f4..b1402bef0ebe 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >> @@ -95,6 +95,7 @@ properties: >> items: >> - description: MBA reserved region >> - description: modem reserved region >> + - description: metadata reserved region > > Which makes the third item now required, also for all out of tree DTS > and other users of the bindings. Please write a bit more in commit msg > why this is necessary (e.g. was it broken before?). I assume the driver > does not break the ABI? I'll pad the commit msg with some of the additional info from patch 4. commit c44094eee32f "arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs from msm8996 on wards. The application processor accessing the dynamically allocated region after giving control to the modem results in a XPU violation. The recommended fix was to use a no-map carveout instead and memunmap before giving control to the modem. The future kernels that are paired with an older dtbs would crash during modem bootup since we would continue to use dma_alloc_attr. But all the other combinations (old kernel/new dtb) will continue to work. - Sibi > > Best regards, > Krzysztof >
On 14/12/2022 11:25, Sibi Sankar wrote: > > > On 12/14/22 01:11, Krzysztof Kozlowski wrote: >> On 13/12/2022 15:07, Sibi Sankar wrote: >>> Update the bindings to reflect the addition of the new modem metadata >>> carveout reference to the memory-region property. >>> >>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>> --- >>> .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 3 ++- >>> .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 3 ++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> index e4a7da8020f4..b1402bef0ebe 100644 >>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>> @@ -95,6 +95,7 @@ properties: >>> items: >>> - description: MBA reserved region >>> - description: modem reserved region >>> + - description: metadata reserved region >> >> Which makes the third item now required, also for all out of tree DTS >> and other users of the bindings. Please write a bit more in commit msg >> why this is necessary (e.g. was it broken before?). I assume the driver >> does not break the ABI? > > I'll pad the commit msg with some of the additional info from patch 4. > commit c44094eee32f "arm64: dma: Drop cache invalidation from > arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs > from msm8996 on wards. The application processor accessing the > dynamically allocated region after giving control to the modem results > in a XPU violation. The recommended fix was to use a no-map carveout > instead and memunmap before giving control to the modem. The future > kernels that are paired with an older dtbs would crash during modem Then it's an ABI break. > bootup since we would continue to use dma_alloc_attr. But all the other > combinations (old kernel/new dtb) will continue to work. Does it mean that old kernel with old DTB was working? If yes, then it's ABI break without clear benefits. Best regards, Krzysztof
On 12/14/22 17:00, Krzysztof Kozlowski wrote: > On 14/12/2022 11:25, Sibi Sankar wrote: >> >> >> On 12/14/22 01:11, Krzysztof Kozlowski wrote: >>> On 13/12/2022 15:07, Sibi Sankar wrote: >>>> Update the bindings to reflect the addition of the new modem metadata >>>> carveout reference to the memory-region property. >>>> >>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> >>>> --- >>>> .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 3 ++- >>>> .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 3 ++- >>>> 2 files changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>>> index e4a7da8020f4..b1402bef0ebe 100644 >>>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml >>>> @@ -95,6 +95,7 @@ properties: >>>> items: >>>> - description: MBA reserved region >>>> - description: modem reserved region >>>> + - description: metadata reserved region >>> >>> Which makes the third item now required, also for all out of tree DTS >>> and other users of the bindings. Please write a bit more in commit msg >>> why this is necessary (e.g. was it broken before?). I assume the driver >>> does not break the ABI? >> >> I'll pad the commit msg with some of the additional info from patch 4. >> commit c44094eee32f "arm64: dma: Drop cache invalidation from >> arch_dma_prep_coherent()" exposed a bug in the driver affecting SoCs >> from msm8996 on wards. The application processor accessing the >> dynamically allocated region after giving control to the modem results >> in a XPU violation. The recommended fix was to use a no-map carveout >> instead and memunmap before giving control to the modem. The future >> kernels that are paired with an older dtbs would crash during modem > > Then it's an ABI break. > >> bootup since we would continue to use dma_alloc_attr. But all the other >> combinations (old kernel/new dtb) will continue to work. > > Does it mean that old kernel with old DTB was working? If yes, then it's > ABI break without clear benefits. commit c44094eee32f is going to land regardless soon and will break modem on mainline and any other branches that picks up the patch. The suggested way to fix it (no-map carveout) requires this bindings change. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml index e4a7da8020f4..b1402bef0ebe 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml @@ -95,6 +95,7 @@ properties: items: - description: MBA reserved region - description: modem reserved region + - description: metadata reserved region firmware-name: $ref: /schemas/types.yaml#/definitions/string-array @@ -223,7 +224,7 @@ examples: <&rpmhpd SC7180_MSS>; power-domain-names = "cx", "mx", "mss"; - memory-region = <&mba_mem>, <&mpss_mem>; + memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>; qcom,qmp = <&aoss_qmp>; diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml index b4de0521a89d..005cb21732af 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml @@ -95,6 +95,7 @@ properties: items: - description: MBA reserved region - description: modem reserved region + - description: metadata reserved region firmware-name: $ref: /schemas/types.yaml#/definitions/string-array @@ -240,7 +241,7 @@ examples: <&rpmhpd SC7280_MSS>; power-domain-names = "cx", "mss"; - memory-region = <&mba_mem>, <&mpss_mem>; + memory-region = <&mba_mem>, <&mpss_mem>, <&mdata_mem>; qcom,qmp = <&aoss_qmp>;
Update the bindings to reflect the addition of the new modem metadata carveout reference to the memory-region property. Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- .../devicetree/bindings/remoteproc/qcom,sc7180-mss-pil.yaml | 3 ++- .../devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)