* RE: [PATCH] drm/amdgpu: Add a chunk ID for spm trace
From: He, Jacob @ 2020-02-20 10:19 UTC (permalink / raw)
To: Zhou, David(ChunMing), Koenig, Christian,
amd-gfx@lists.freedesktop.org
In-Reply-To: <MWHPR12MB1406B3418562F0692572C511B4130@MWHPR12MB1406.namprd12.prod.outlook.com>
[-- Attachment #1.1: Type: text/plain, Size: 10621 bytes --]
Looks like amdgpu_vm_reserve_vmid could work, let me have a try to update the RLC_SPM_VMID with pm4 packets in UMD.
Thanks
Jacob
From: Zhou, David(ChunMing)<mailto:David1.Zhou@amd.com>
Sent: Thursday, February 20, 2020 10:13 AM
To: Koenig, Christian<mailto:Christian.Koenig@amd.com>; He, Jacob<mailto:Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: RE: [PATCH] drm/amdgpu: Add a chunk ID for spm trace
[AMD Official Use Only - Internal Distribution Only]
Christian is right here, that will cause many problems for simply using VMID in kernel.
We already have an pair interface for RGP, I think you can use it instead of involving additional kernel change.
amdgpu_vm_reserve_vmid/ amdgpu_vm_unreserve_vmid.
-David
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
Sent: Wednesday, February 19, 2020 7:03 PM
To: He, Jacob <Jacob.He@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add a chunk ID for spm trace
Am 19.02.20 um 11:15 schrieb Jacob He:
> [WHY]
> When SPM trace enabled, SPM_VMID should be updated with the current
> vmid.
>
> [HOW]
> Add a chunk id, AMDGPU_CHUNK_ID_SPM_TRACE, so that UMD can tell us
> which job should update SPM_VMID.
> Right before a job is submitted to GPU, set the SPM_VMID accordingly.
>
> [Limitation]
> Running more than one SPM trace enabled processes simultaneously is
> not supported.
Well there are multiple problems with that patch.
First of all you need to better describe what SPM tracing is in the commit message.
Then the updating of mmRLC_SPM_MC_CNTL must be executed asynchronously on the ring. Otherwise we might corrupt an already executing SPM trace.
And you also need to make sure to disable the tracing again or otherwise we run into a bunch of trouble when the VMID is reused.
You also need to make sure that IBs using the SPM trace are serialized with each other, e.g. hack into amdgpu_ids.c file and make sure that only one VMID at a time can have that attribute.
Regards,
Christian.
>
> Change-Id: Ic932ef6ac9dbf244f03aaee90550e8ff3a675666
> Signed-off-by: Jacob He <jacob.he@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 +++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 10 +++++++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_rlc.h | 1 +
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 15 ++++++++++++++-
> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 3 ++-
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 3 ++-
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 15 ++++++++++++++-
> 8 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index f9fa6e104fef..3f32c4db5232 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -113,6 +113,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
> uint32_t uf_offset = 0;
> int i;
> int ret;
> + bool update_spm_vmid = false;
>
> if (cs->in.num_chunks == 0)
> return 0;
> @@ -221,6 +222,10 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
> case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL:
> break;
>
> + case AMDGPU_CHUNK_ID_SPM_TRACE:
> + update_spm_vmid = true;
> + break;
> +
> default:
> ret = -EINVAL;
> goto free_partial_kdata;
> @@ -231,6 +236,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
> if (ret)
> goto free_all_kdata;
>
> + p->job->need_update_spm_vmid = update_spm_vmid;
> +
> if (p->ctx->vram_lost_counter != p->job->vram_lost_counter) {
> ret = -ECANCELED;
> goto free_all_kdata;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index cae81914c821..36faab12b585 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -156,9 +156,13 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
> return -EINVAL;
> }
>
> - if (vm && !job->vmid) {
> - dev_err(adev->dev, "VM IB without ID\n");
> - return -EINVAL;
> + if (vm) {
> + if (!job->vmid) {
> + dev_err(adev->dev, "VM IB without ID\n");
> + return -EINVAL;
> + } else if (adev->gfx.rlc.funcs->update_spm_vmid && job->need_update_spm_vmid) {
> + adev->gfx.rlc.funcs->update_spm_vmid(adev, job->vmid);
> + }
> }
>
> alloc_size = ring->funcs->emit_frame_size + num_ibs * diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> index 2e2110dddb76..4582536961c7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> @@ -52,6 +52,7 @@ struct amdgpu_job {
> bool vm_needs_flush;
> uint64_t vm_pd_addr;
> unsigned vmid;
> + bool need_update_spm_vmid;
> unsigned pasid;
> uint32_t gds_base, gds_size;
> uint32_t gws_base, gws_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_rlc.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_rlc.h
> index d3d4707f2168..52509c254cbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_rlc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_rlc.h
> @@ -126,6 +126,7 @@ struct amdgpu_rlc_funcs {
> void (*stop)(struct amdgpu_device *adev);
> void (*reset)(struct amdgpu_device *adev);
> void (*start)(struct amdgpu_device *adev);
> + void (*update_spm_vmid)(struct amdgpu_device *adev, unsigned vmid);
> };
>
> struct amdgpu_rlc {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 5e9fb0976c6c..91eb788d6229 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4214,6 +4214,18 @@ static int gfx_v10_0_update_gfx_clock_gating(struct amdgpu_device *adev,
> return 0;
> }
>
> +static void gfx_v10_0_update_spm_vmid(struct amdgpu_device *adev,
> +unsigned vmid) {
> + u32 data;
> +
> + data = RREG32_SOC15(GC, 0, mmRLC_SPM_MC_CNTL);
> +
> + data &= ~RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK;
> + data |= (vmid & RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK) <<
> +RLC_SPM_MC_CNTL__RLC_SPM_VMID__SHIFT;
> +
> + WREG32_SOC15(GC, 0, mmRLC_SPM_MC_CNTL, data); }
> +
> static const struct amdgpu_rlc_funcs gfx_v10_0_rlc_funcs = {
> .is_rlc_enabled = gfx_v10_0_is_rlc_enabled,
> .set_safe_mode = gfx_v10_0_set_safe_mode, @@ -4224,7 +4236,8 @@
> static const struct amdgpu_rlc_funcs gfx_v10_0_rlc_funcs = {
> .resume = gfx_v10_0_rlc_resume,
> .stop = gfx_v10_0_rlc_stop,
> .reset = gfx_v10_0_rlc_reset,
> - .start = gfx_v10_0_rlc_start
> + .start = gfx_v10_0_rlc_start,
> + .update_spm_vmid = gfx_v10_0_update_spm_vmid
> };
>
> static int gfx_v10_0_set_powergating_state(void *handle, diff --git
> a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 8f20a5dd44fe..b24fc55cf13a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -4221,7 +4221,8 @@ static const struct amdgpu_rlc_funcs gfx_v7_0_rlc_funcs = {
> .resume = gfx_v7_0_rlc_resume,
> .stop = gfx_v7_0_rlc_stop,
> .reset = gfx_v7_0_rlc_reset,
> - .start = gfx_v7_0_rlc_start
> + .start = gfx_v7_0_rlc_start,
> + .update_spm_vmid = NULL
> };
>
> static int gfx_v7_0_early_init(void *handle) diff --git
> a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index fa245973de12..66640d2b6b37 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -5600,7 +5600,8 @@ static const struct amdgpu_rlc_funcs iceland_rlc_funcs = {
> .resume = gfx_v8_0_rlc_resume,
> .stop = gfx_v8_0_rlc_stop,
> .reset = gfx_v8_0_rlc_reset,
> - .start = gfx_v8_0_rlc_start
> + .start = gfx_v8_0_rlc_start,
> + .update_spm_vmid = NULL
> };
>
> static void gfx_v8_0_update_medium_grain_clock_gating(struct
> amdgpu_device *adev, diff --git
> a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 9b7ff783e9a5..df872f949f68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -4704,6 +4704,18 @@ static int gfx_v9_0_update_gfx_clock_gating(struct amdgpu_device *adev,
> return 0;
> }
>
> +static void gfx_v9_0_update_spm_vmid(struct amdgpu_device *adev,
> +unsigned vmid) {
> + u32 data;
> +
> + data = RREG32_SOC15(GC, 0, mmRLC_SPM_MC_CNTL);
> +
> + data &= ~RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK;
> + data |= (vmid & RLC_SPM_MC_CNTL__RLC_SPM_VMID_MASK) <<
> +RLC_SPM_MC_CNTL__RLC_SPM_VMID__SHIFT;
> +
> + WREG32_SOC15(GC, 0, mmRLC_SPM_MC_CNTL, data); }
> +
> static const struct amdgpu_rlc_funcs gfx_v9_0_rlc_funcs = {
> .is_rlc_enabled = gfx_v9_0_is_rlc_enabled,
> .set_safe_mode = gfx_v9_0_set_safe_mode, @@ -4715,7 +4727,8 @@
> static const struct amdgpu_rlc_funcs gfx_v9_0_rlc_funcs = {
> .resume = gfx_v9_0_rlc_resume,
> .stop = gfx_v9_0_rlc_stop,
> .reset = gfx_v9_0_rlc_reset,
> - .start = gfx_v9_0_rlc_start
> + .start = gfx_v9_0_rlc_start,
> + .update_spm_vmid = gfx_v9_0_update_spm_vmid
> };
>
> static int gfx_v9_0_set_powergating_state(void *handle,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cdavid1.zhou%40amd.com%7C354be34ff18e4f424f6708d7b52b43b0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637177069753914395&sdata=9rSL4kgPJweuZ4EJpdqtqTxyCVGEkmsg6aUzbtvGFrs%3D&reserved=0
[-- Attachment #1.2: Type: text/html, Size: 19894 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH 0/3] leds: add support for apa102c leds
From: Nicolas Belin @ 2020-02-20 10:19 UTC (permalink / raw)
To: Dan Murphy; +Cc: linux-kernel, linux-leds, jacek.anaszewski, pavel
In-Reply-To: <04642127-0e68-43b1-9b6c-0dbb56dc9bfe@ti.com>
Hi Dan,
Le mar. 18 févr. 2020 à 13:47, Dan Murphy <dmurphy@ti.com> a écrit :
>
> Hellp
>
> On 2/18/20 3:37 AM, Nicolas Belin wrote:
> > This patch series adds the driver and its related documentation
> > for the APA102C RGB Leds.
> >
> > Patch 1 adds the APA102C led manufacturer to the vendor-prefixes list.
> >
> > Patch 2 Documents the APA102C led driver.
> >
> > Patch 3 contains the actual driver code and modifications in the Kconfig
> > and the Makefile.
>
> Is this something that can benefit from the Multicolor framework patches?
>
> https://lore.kernel.org/patchwork/project/lkml/list/?series=427513
>
> Can you RFC the APA102C driver on top of the Multicolor FW to see how it
> blends?
Sure, the Multicolor framework will probably improve my driver !
I'll send you a new version once I have tested it.
>
> Dan
>
Thanks,
Nicolas
^ permalink raw reply
* Re: [CFT 5/8] net: dpaa2-mac: use resolved link config in mac_link_up()
From: Russell King - ARM Linux admin @ 2020-02-20 10:20 UTC (permalink / raw)
To: Ioana Ciornei
Cc: David S. Miller, netdev@vger.kernel.org, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
In-Reply-To: <DB8PR04MB6828ECB9945F747281C5796EE0110@DB8PR04MB6828.eurprd04.prod.outlook.com>
On Tue, Feb 18, 2020 at 10:42:41AM +0000, Ioana Ciornei wrote:
> > Subject: Re: [CFT 5/8] net: dpaa2-mac: use resolved link config in mac_link_up()
> >
> > It would really help if MAINTAINERS were updated with the correct information
> > for this driver:
> >
> > DPAA2 ETHERNET DRIVER
> > M: Ioana Radulescu <ruxandra.radulescu@nxp.com>
> >
> > This address bounces. Given what I find in the git history, is the correct person is
> > now:
> >
> > Ioana Ciornei <ioana.ciornei@nxp.com>
> >
> > Please submit a patch updating MAINTAINERS. Thanks.
>
> Sure thing. I'll update the MAINTAINERS file and list myself instead of Ioana Radulescu.
Any comments on the patch itself?
> > On Mon, Feb 17, 2020 at 05:24:16PM +0000, Russell King wrote:
> > > Convert the DPAA2 ethernet driver to use the finalised link parameters
> > > in mac_link_up() rather than the parameters in mac_config(), which are
> > > more suited to the needs of the DPAA2 MC firmware than those available
> > > via mac_config().
> > >
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 54
> > > +++++++++++-------- .../net/ethernet/freescale/dpaa2/dpaa2-mac.h |
> > > 1 +
> > > 2 files changed, 33 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > > index 3a75c5b58f95..3ee236c5fc37 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > > @@ -123,35 +123,16 @@ static void dpaa2_mac_config(struct phylink_config
> > *config, unsigned int mode,
> > > struct dpmac_link_state *dpmac_state = &mac->state;
> > > int err;
> > >
> > > - if (state->speed != SPEED_UNKNOWN)
> > > - dpmac_state->rate = state->speed;
> > > -
> > > - if (state->duplex != DUPLEX_UNKNOWN) {
> > > - if (!state->duplex)
> > > - dpmac_state->options |=
> > DPMAC_LINK_OPT_HALF_DUPLEX;
> > > - else
> > > - dpmac_state->options &=
> > ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > > - }
> > > -
> > > if (state->an_enabled)
> > > dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG;
> > > else
> > > dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG;
> > >
> > > - if (state->pause & MLO_PAUSE_RX)
> > > - dpmac_state->options |= DPMAC_LINK_OPT_PAUSE;
> > > - else
> > > - dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE;
> > > -
> > > - if (!!(state->pause & MLO_PAUSE_RX) ^ !!(state->pause &
> > MLO_PAUSE_TX))
> > > - dpmac_state->options |= DPMAC_LINK_OPT_ASYM_PAUSE;
> > > - else
> > > - dpmac_state->options &= ~DPMAC_LINK_OPT_ASYM_PAUSE;
> > > -
> > > err = dpmac_set_link_state(mac->mc_io, 0,
> > > mac->mc_dev->mc_handle, dpmac_state);
> > > if (err)
> > > - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n",
> > err);
> > > + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() =
> > %d\n",
> > > + __func__, err);
> > > }
> > >
> > > static void dpaa2_mac_link_up(struct phylink_config *config, @@
> > > -165,10 +146,37 @@ static void dpaa2_mac_link_up(struct phylink_config
> > *config,
> > > int err;
> > >
> > > dpmac_state->up = 1;
> > > +
> > > + if (mac->if_link_type == DPMAC_LINK_TYPE_PHY) {
> > > + /* If the DPMAC is configured for PHY mode, we need
> > > + * to pass the link parameters to the MC firmware.
> > > + */
> > > + dpmac_state->rate = speed;
> > > +
> > > + if (duplex == DUPLEX_HALF)
> > > + dpmac_state->options |=
> > DPMAC_LINK_OPT_HALF_DUPLEX;
> > > + else if (duplex == DUPLEX_FULL)
> > > + dpmac_state->options &=
> > ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > > +
> > > + /* This is lossy; the firmware really should take the pause
> > > + * enablement status rather than pause/asym pause status.
> > > + */
> > > + if (rx_pause)
> > > + dpmac_state->options |= DPMAC_LINK_OPT_PAUSE;
> > > + else
> > > + dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE;
> > > +
> > > + if (rx_pause ^ tx_pause)
> > > + dpmac_state->options |=
> > DPMAC_LINK_OPT_ASYM_PAUSE;
> > > + else
> > > + dpmac_state->options &=
> > ~DPMAC_LINK_OPT_ASYM_PAUSE;
> > > + }
> > > +
> > > err = dpmac_set_link_state(mac->mc_io, 0,
> > > mac->mc_dev->mc_handle, dpmac_state);
> > > if (err)
> > > - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n",
> > err);
> > > + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() =
> > %d\n",
> > > + __func__, err);
> > > }
> > >
> > > static void dpaa2_mac_link_down(struct phylink_config *config, @@
> > > -241,6 +249,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
> > > goto err_close_dpmac;
> > > }
> > >
> > > + mac->if_link_type = attr.link_type;
> > > +
> > > dpmac_node = dpaa2_mac_get_node(attr.id);
> > > if (!dpmac_node) {
> > > netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
> > diff
> > > --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
> > > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
> > > index 4da8079b9155..2130d9c7d40e 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
> > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h
> > > @@ -20,6 +20,7 @@ struct dpaa2_mac {
> > > struct phylink_config phylink_config;
> > > struct phylink *phylink;
> > > phy_interface_t if_mode;
> > > + enum dpmac_link_type if_link_type;
> > > };
> > >
> > > bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
> > > --
> > > 2.20.1
> > >
> > >
> >
> > --
> > RMK's Patch system:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> > mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Cioana.cior
> > nei%40nxp.com%7C09d0167191914135433808d7b45e15fd%7C686ea1d3bc2b4
> > c6fa92cd99c5c301635%7C0%7C0%7C637176188497544105&sdata=t0%2B
> > OzkoqRM180UHGBrW6FYAvHsIelx4CaP4oC3QcP1k%3D&reserved=0
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: Bug: Git: Clone: University Network: No Output on Terminal
From: Manish Devgan @ 2020-02-20 10:20 UTC (permalink / raw)
To: René Scharfe; +Cc: git, brian m. carlson, Junio C Hamano
In-Reply-To: <9ed26e7e-c19c-cdb2-0710-3b91bf31291b@web.de>
On Wed, Feb 19, 2020 at 12:54 AM René Scharfe <l.s.r@web.de> wrote:
> How about something like this?
>
> -- >8 --
> Subject: [PATCH] remote-curl: show progress for fetches over dumb HTTP
>
> Fetching over dumb HTTP transport doesn't show any progress, even with
> the option --progress. If the connection is slow or there is a lot of
> data to get then this can take a long time while the user is left to
> wonder if git got stuck.
>
> We don't know the number of objects to fetch at the outset, but we can
> count the ones we got. Show an open-ended progress indicator based on
> that number if the user asked for it.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> remote-curl.c | 1 +
> walker.c | 13 ++++++++++++-
> walker.h | 1 +
> 3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 8eb96152f5..e4cd321844 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1026,6 +1026,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
>
> walker = get_http_walker(url.buf);
> walker->get_verbosely = options.verbosity >= 3;
> + walker->get_progress = options.progress;
> walker->get_recover = 0;
> ret = walker_fetch(walker, nr_heads, targets, NULL, NULL);
> walker_free(walker);
> diff --git a/walker.c b/walker.c
> index bb010f7a2b..4984bf8b3d 100644
> --- a/walker.c
> +++ b/walker.c
> @@ -8,6 +8,7 @@
> #include "tag.h"
> #include "blob.h"
> #include "refs.h"
> +#include "progress.h"
>
> static struct object_id current_commit_oid;
>
> @@ -162,6 +163,11 @@ static int process(struct walker *walker, struct object *obj)
> static int loop(struct walker *walker)
> {
> struct object_list *elem;
> + struct progress *progress = NULL;
> + uint64_t nr = 0;
> +
> + if (walker->get_progress)
> + progress = start_delayed_progress(_("Fetching objects"), 0);
>
> while (process_queue) {
> struct object *obj = process_queue->item;
> @@ -176,15 +182,20 @@ static int loop(struct walker *walker)
> */
> if (! (obj->flags & TO_SCAN)) {
> if (walker->fetch(walker, obj->oid.hash)) {
> + stop_progress(&progress);
> report_missing(obj);
> return -1;
> }
> }
> if (!obj->type)
> parse_object(the_repository, &obj->oid);
> - if (process_object(walker, obj))
> + if (process_object(walker, obj)) {
> + stop_progress(&progress);
> return -1;
> + }
> + display_progress(progress, ++nr);
> }
> + stop_progress(&progress);
> return 0;
> }
>
> diff --git a/walker.h b/walker.h
> index 6d8ae00e5b..d40b016bab 100644
> --- a/walker.h
> +++ b/walker.h
> @@ -10,6 +10,7 @@ struct walker {
> int (*fetch)(struct walker *, unsigned char *sha1);
> void (*cleanup)(struct walker *);
> int get_verbosely;
> + int get_progress;
> int get_recover;
>
> int corrupt_object_found;
> --
> 2.25.1
This is great. But perhaps I would want the output to come
irrespective of --progress argument in case of dumb_http.
Thanks & Regards
Manish Devgan
^ permalink raw reply
* Re: [PATCH v2 09/21] btrfs: factor out create_chunk()
From: Naohiro Aota @ 2020-02-20 10:17 UTC (permalink / raw)
To: Josef Bacik
Cc: linux-btrfs, David Sterba, Chris Mason, Nikolay Borisov,
Damien Le Moal, Johannes Thumshirn, Hannes Reinecke, Anand Jain,
linux-fsdevel
In-Reply-To: <7514070d-b7a8-be1c-c23a-f01b9ee3c7ce@toxicpanda.com>
On Thu, Feb 13, 2020 at 11:24:57AM -0500, Josef Bacik wrote:
>On 2/12/20 2:20 AM, Naohiro Aota wrote:
>>Factor out create_chunk() from __btrfs_alloc_chunk(). This function finally
>>creates a chunk. There is no functional changes.
>>
>>Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>---
>> fs/btrfs/volumes.c | 130 ++++++++++++++++++++++++---------------------
>> 1 file changed, 70 insertions(+), 60 deletions(-)
>>
>>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>index 00085943e4dd..3e2e3896d72a 100644
>>--- a/fs/btrfs/volumes.c
>>+++ b/fs/btrfs/volumes.c
>>@@ -5052,90 +5052,53 @@ static int decide_stripe_size(struct btrfs_fs_devices *fs_devices,
>> }
>> }
>>-static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>- u64 start, u64 type)
>>+static int create_chunk(struct btrfs_trans_handle *trans,
>>+ struct alloc_chunk_ctl *ctl,
>>+ struct btrfs_device_info *devices_info)
>> {
>> struct btrfs_fs_info *info = trans->fs_info;
>>- struct btrfs_fs_devices *fs_devices = info->fs_devices;
>> struct map_lookup *map = NULL;
>> struct extent_map_tree *em_tree;
>> struct extent_map *em;
>>- struct btrfs_device_info *devices_info = NULL;
>>- struct alloc_chunk_ctl ctl;
>>+ u64 start = ctl->start;
>>+ u64 type = ctl->type;
>> int ret;
>> int i;
>> int j;
>>- if (!alloc_profile_is_valid(type, 0)) {
>>- ASSERT(0);
>>- return -EINVAL;
>>- }
>>-
>>- if (list_empty(&fs_devices->alloc_list)) {
>>- if (btrfs_test_opt(info, ENOSPC_DEBUG))
>>- btrfs_debug(info, "%s: no writable device", __func__);
>>- return -ENOSPC;
>>- }
>>-
>>- if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
>>- btrfs_err(info, "invalid chunk type 0x%llx requested", type);
>>- BUG();
>>- }
>>-
>>- ctl.start = start;
>>- ctl.type = type;
>>- init_alloc_chunk_ctl(fs_devices, &ctl);
>>-
>>- devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info),
>>- GFP_NOFS);
>>- if (!devices_info)
>>+ map = kmalloc(map_lookup_size(ctl->num_stripes), GFP_NOFS);
>>+ if (!map)
>> return -ENOMEM;
>>+ map->num_stripes = ctl->num_stripes;
>>- ret = gather_device_info(fs_devices, &ctl, devices_info);
>>- if (ret < 0)
>>- goto error;
>>-
>>- ret = decide_stripe_size(fs_devices, &ctl, devices_info);
>>- if (ret < 0)
>>- goto error;
>>-
>>- map = kmalloc(map_lookup_size(ctl.num_stripes), GFP_NOFS);
>>- if (!map) {
>>- ret = -ENOMEM;
>>- goto error;
>>- }
>>-
>>- map->num_stripes = ctl.num_stripes;
>>-
>>- for (i = 0; i < ctl.ndevs; ++i) {
>>- for (j = 0; j < ctl.dev_stripes; ++j) {
>>- int s = i * ctl.dev_stripes + j;
>>+ for (i = 0; i < ctl->ndevs; ++i) {
>>+ for (j = 0; j < ctl->dev_stripes; ++j) {
>>+ int s = i * ctl->dev_stripes + j;
>> map->stripes[s].dev = devices_info[i].dev;
>> map->stripes[s].physical = devices_info[i].dev_offset +
>>- j * ctl.stripe_size;
>>+ j * ctl->stripe_size;
>> }
>> }
>> map->stripe_len = BTRFS_STRIPE_LEN;
>> map->io_align = BTRFS_STRIPE_LEN;
>> map->io_width = BTRFS_STRIPE_LEN;
>> map->type = type;
>>- map->sub_stripes = ctl.sub_stripes;
>>+ map->sub_stripes = ctl->sub_stripes;
>>- trace_btrfs_chunk_alloc(info, map, start, ctl.chunk_size);
>>+ trace_btrfs_chunk_alloc(info, map, start, ctl->chunk_size);
>> em = alloc_extent_map();
>> if (!em) {
>> kfree(map);
>>- ret = -ENOMEM;
>>- goto error;
>>+ return -ENOMEM;
>> }
>> set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags);
>> em->map_lookup = map;
>> em->start = start;
>>- em->len = ctl.chunk_size;
>>+ em->len = ctl->chunk_size;
>> em->block_start = 0;
>> em->block_len = em->len;
>>- em->orig_block_len = ctl.stripe_size;
>>+ em->orig_block_len = ctl->stripe_size;
>> em_tree = &info->mapping_tree;
>> write_lock(&em_tree->lock);
>>@@ -5143,11 +5106,11 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>> if (ret) {
>> write_unlock(&em_tree->lock);
>> free_extent_map(em);
>>- goto error;
>>+ return ret;
>> }
>> write_unlock(&em_tree->lock);
>>- ret = btrfs_make_block_group(trans, 0, type, start, ctl.chunk_size);
>>+ ret = btrfs_make_block_group(trans, 0, type, start, ctl->chunk_size);
>> if (ret)
>> goto error_del_extent;
>>@@ -5155,20 +5118,19 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>> struct btrfs_device *dev = map->stripes[i].dev;
>> btrfs_device_set_bytes_used(dev,
>>- dev->bytes_used + ctl.stripe_size);
>>+ dev->bytes_used + ctl->stripe_size);
>> if (list_empty(&dev->post_commit_list))
>> list_add_tail(&dev->post_commit_list,
>> &trans->transaction->dev_update_list);
>> }
>>- atomic64_sub(ctl.stripe_size * map->num_stripes,
>>+ atomic64_sub(ctl->stripe_size * map->num_stripes,
>> &info->free_chunk_space);
>> free_extent_map(em);
>> check_raid56_incompat_flag(info, type);
>> check_raid1c34_incompat_flag(info, type);
>>- kfree(devices_info);
>> return 0;
>> error_del_extent:
>>@@ -5180,7 +5142,55 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>> free_extent_map(em);
>> /* One for the tree reference */
>> free_extent_map(em);
>>-error:
>>+
>>+ return ret;
>>+}
>>+
>>+static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>+ u64 start, u64 type)
>>+{
>>+ struct btrfs_fs_info *info = trans->fs_info;
>>+ struct btrfs_fs_devices *fs_devices = info->fs_devices;
>>+ struct btrfs_device_info *devices_info = NULL;
>>+ struct alloc_chunk_ctl ctl;
>>+ int ret;
>>+
>>+ if (!alloc_profile_is_valid(type, 0)) {
>>+ ASSERT(0);
>>+ return -EINVAL;
>>+ }
>>+
>>+ if (list_empty(&fs_devices->alloc_list)) {
>>+ if (btrfs_test_opt(info, ENOSPC_DEBUG))
>>+ btrfs_debug(info, "%s: no writable device", __func__);
>>+ return -ENOSPC;
>>+ }
>>+
>>+ if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
>>+ btrfs_err(info, "invalid chunk type 0x%llx requested", type);
>>+ BUG();
>>+ }
>
>This is superfluous, alloc_profile_is_valid() handles this check. Thanks,
>
>Josef
This checks if at least one block group type (data, metadata or
system) flag is set. OTOH, alloc_profile_is_valid() checks if profile
bits are valid.
Maybe, we can move this check into alloc_profile_is_valid()?
Thanks,
^ permalink raw reply
* Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()
From: Christian Brauner @ 2020-02-20 10:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: David S. Miller, Greg Kroah-Hartman, Linux Kernel Mailing List,
netdev, Pavel Machek, Jakub Kicinski, Eric Dumazet,
Stephen Hemminger, Linux PM
In-Reply-To: <CAJZ5v0hJwXH8Oc4spzDDemHhBVGKqtbrV2UG6-gmT-F0hA4ynA@mail.gmail.com>
On Thu, Feb 20, 2020 at 11:02:04AM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 18, 2020 at 5:30 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > Add a helper to change the owner of a device's power entries. This
> > needs to happen when the ownership of a device is changed, e.g. when
> > moving network devices between network namespaces.
> > This function will be used to correctly account for ownership changes,
> > e.g. when moving network devices between network namespaces.
> >
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v2 */
> > - "Rafael J. Wysocki" <rafael@kernel.org>:
> > - Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into
> > if (device_can_wakeup(dev)) check since the former can never be true if
> > the latter is false.
> >
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> > - Place (dev->power.wakeup && dev->power.wakeup->dev) check under
> > CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available
> > when this config option is set.
> >
> > /* v3 */
> > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> > - Add explicit uid/gid parameters.
> > ---
> > drivers/base/core.c | 4 ++++
> > drivers/base/power/power.h | 3 +++
> > drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index ec0d5e8cfd0f..efec2792f5d7 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > if (error)
> > goto out;
> >
> > + error = dpm_sysfs_change_owner(dev, kuid, kgid);
> > + if (error)
> > + goto out;
> > +
> > #ifdef CONFIG_BLOCK
> > if (sysfs_deprecated && dev->class == &block_class)
> > goto out;
> > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> > index 444f5c169a0b..54292cdd7808 100644
> > --- a/drivers/base/power/power.h
> > +++ b/drivers/base/power/power.h
> > @@ -74,6 +74,7 @@ extern int pm_qos_sysfs_add_flags(struct device *dev);
> > extern void pm_qos_sysfs_remove_flags(struct device *dev);
> > extern int pm_qos_sysfs_add_latency_tolerance(struct device *dev);
> > extern void pm_qos_sysfs_remove_latency_tolerance(struct device *dev);
> > +extern int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
> >
> > #else /* CONFIG_PM */
> >
> > @@ -88,6 +89,8 @@ static inline void pm_runtime_remove(struct device *dev) {}
> >
> > static inline int dpm_sysfs_add(struct device *dev) { return 0; }
> > static inline void dpm_sysfs_remove(struct device *dev) {}
> > +static inline int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid,
> > + kgid_t kgid) { return 0; }
> >
> > #endif
> >
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index d7d82db2e4bc..4e79afcd5ca8 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev)
> > return rc;
> > }
> >
> > +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > +{
> > + int rc;
> > +
> > + if (device_pm_not_required(dev))
> > + return 0;
> > +
> > + rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid);
> > + if (rc)
> > + return rc;
> > +
> > + if (pm_runtime_callbacks_present(dev)) {
> > + rc = sysfs_group_change_owner(
> > + &dev->kobj, &pm_runtime_attr_group, kuid, kgid);
> > + if (rc)
> > + return rc;
> > + }
> > + if (device_can_wakeup(dev)) {
> > + rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group,
> > + kuid, kgid);
> > + if (rc)
> > + return rc;
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > + if (dev->power.wakeup && dev->power.wakeup->dev) {
> > + rc = device_change_owner(dev->power.wakeup->dev, kuid,
> > + kgid);
> > + if (rc)
> > + return rc;
> > + }
> > +#endif
>
> First off, I don't particularly like #ifdefs in function bodies. In
> particular, there is a CONFIG_PM_SLEEP block in this file already and
> you could define a new function in there to carry out the above
> operations, and provide an empty stub of it for the "unset" case.
> Failing to do so is somewhat on the "rushing things in" side in my
> view.
How ifdefines are used is highly dependent on the subsystem; networking
ofen uses in-place ifdefines in some parts and not in others. That has
nothing to do with rushing things. I'm happy to change it to your
preferences. Thanks for pointing out your expectations. But please don't
assume bad intentions on my part because I'm not meeting them right
away. It often is the case that adding a helper that is called in one
place is not well-received.
>
> Second, the #ifdef should cover the entire if (device_can_wakeup(dev))
> {} block, because wakeup_sysfs_add() is only called if
> device_can_wakeup(dev) returns 'true' for the device in question (and
> arguably you could have checked that easily enough).
I've looked at the header definitions for device_can_wakeup() and with
and without CONFIG_PM_SLEEP it is defined as:
static inline bool device_can_wakeup(struct device *dev)
{
return dev->power.can_wakeup;
}
which to me looks like it would neet to be called in all cases.
I'll rework this to you preferences.
Thanks!
Christian
^ permalink raw reply
* Re: [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization
From: Jani Nikula @ 2020-02-20 10:21 UTC (permalink / raw)
To: Kees Cook, Joonas Lahtinen, Rodrigo Vivi
Cc: intel-gfx, Alexander Potapenko, Kees Cook, linux-kernel
In-Reply-To: <20200220062258.68854-1-keescook@chromium.org>
On Wed, 19 Feb 2020, Kees Cook <keescook@chromium.org> wrote:
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
>
> To avoid these problems, move such variables into the "case" where
> they're used or lift them up into the main function body.
>
> drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
> drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
> 12963 | unsigned int port_mask;
> | ^~~~~~~~~
>
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
> drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
> 474 | u32 dsparb, dsparb2, dsparb3;
> | ^~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
> drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
> 1997 | u32 dsparb, dsparb2, dsparb3;
> | ^~~~~~
>
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
If you look at i915/Makefile, you'll see that we don't shy away from
enabling lots of extra warnings, and we run our CI with -Werror to keep
it clean. It does not seem like -Wswitch-unreachable does me any good,
though... is it new?
BR,
Jani.
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 064dd99bbc49..c829cd26f99e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> WARN_ON(!connector_state->crtc);
>
> switch (encoder->type) {
> - unsigned int port_mask;
> case INTEL_OUTPUT_DDI:
> if (WARN_ON(!HAS_DDI(to_i915(dev))))
> break;
> /* else, fall through */
> case INTEL_OUTPUT_DP:
> case INTEL_OUTPUT_HDMI:
> - case INTEL_OUTPUT_EDP:
> + case INTEL_OUTPUT_EDP: {
> + unsigned int port_mask;
> +
> port_mask = 1 << encoder->port;
>
> /* the same port mustn't appear more than once */
> @@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>
> used_ports |= port_mask;
> break;
> + }
> case INTEL_OUTPUT_DP_MST:
> used_mst_ports |=
> 1 << encoder->port;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bd2d30ecc030..17d8833787c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
> struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
> enum pipe pipe = crtc->pipe;
> int sprite0_start, sprite1_start;
> + u32 dsparb, dsparb2, dsparb3;
>
> switch (pipe) {
> - u32 dsparb, dsparb2, dsparb3;
> case PIPE_A:
> dsparb = I915_READ(DSPARB);
> dsparb2 = I915_READ(DSPARB2);
> @@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> const struct vlv_fifo_state *fifo_state =
> &crtc_state->wm.vlv.fifo_state;
> int sprite0_start, sprite1_start, fifo_size;
> + u32 dsparb, dsparb2, dsparb3;
>
> if (!crtc_state->fifo_changed)
> return;
> @@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> spin_lock(&uncore->lock);
>
> switch (crtc->pipe) {
> - u32 dsparb, dsparb2, dsparb3;
> case PIPE_A:
> dsparb = intel_uncore_read_fw(uncore, DSPARB);
> dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);
>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH] drm/i915: Distribute switch variables for initialization
From: Jani Nikula @ 2020-02-20 10:21 UTC (permalink / raw)
To: Kees Cook, Joonas Lahtinen, Rodrigo Vivi
Cc: Alexander Potapenko, Kees Cook, intel-gfx, linux-kernel
In-Reply-To: <20200220062258.68854-1-keescook@chromium.org>
On Wed, 19 Feb 2020, Kees Cook <keescook@chromium.org> wrote:
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
>
> To avoid these problems, move such variables into the "case" where
> they're used or lift them up into the main function body.
>
> drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
> drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
> 12963 | unsigned int port_mask;
> | ^~~~~~~~~
>
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
> drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
> 474 | u32 dsparb, dsparb2, dsparb3;
> | ^~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
> drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
> 1997 | u32 dsparb, dsparb2, dsparb3;
> | ^~~~~~
>
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
If you look at i915/Makefile, you'll see that we don't shy away from
enabling lots of extra warnings, and we run our CI with -Werror to keep
it clean. It does not seem like -Wswitch-unreachable does me any good,
though... is it new?
BR,
Jani.
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 064dd99bbc49..c829cd26f99e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> WARN_ON(!connector_state->crtc);
>
> switch (encoder->type) {
> - unsigned int port_mask;
> case INTEL_OUTPUT_DDI:
> if (WARN_ON(!HAS_DDI(to_i915(dev))))
> break;
> /* else, fall through */
> case INTEL_OUTPUT_DP:
> case INTEL_OUTPUT_HDMI:
> - case INTEL_OUTPUT_EDP:
> + case INTEL_OUTPUT_EDP: {
> + unsigned int port_mask;
> +
> port_mask = 1 << encoder->port;
>
> /* the same port mustn't appear more than once */
> @@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>
> used_ports |= port_mask;
> break;
> + }
> case INTEL_OUTPUT_DP_MST:
> used_mst_ports |=
> 1 << encoder->port;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bd2d30ecc030..17d8833787c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
> struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
> enum pipe pipe = crtc->pipe;
> int sprite0_start, sprite1_start;
> + u32 dsparb, dsparb2, dsparb3;
>
> switch (pipe) {
> - u32 dsparb, dsparb2, dsparb3;
> case PIPE_A:
> dsparb = I915_READ(DSPARB);
> dsparb2 = I915_READ(DSPARB2);
> @@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> const struct vlv_fifo_state *fifo_state =
> &crtc_state->wm.vlv.fifo_state;
> int sprite0_start, sprite1_start, fifo_size;
> + u32 dsparb, dsparb2, dsparb3;
>
> if (!crtc_state->fifo_changed)
> return;
> @@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> spin_lock(&uncore->lock);
>
> switch (crtc->pipe) {
> - u32 dsparb, dsparb2, dsparb3;
> case PIPE_A:
> dsparb = intel_uncore_read_fw(uncore, DSPARB);
> dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);
>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [meta-ti] am437x-evm zeus image
From: Daniel Ammann @ 2020-02-20 10:21 UTC (permalink / raw)
To: Denys Dmytriyenko; +Cc: meta-ti
In-Reply-To: <20200219190052.GS720@beryl>
Hi Denys
Thanks for the answer.
My build configuration looks like this:
Build Configuration:
BB_VERSION = "1.44.0"
BUILD_SYS = "x86_64-linux"
NATIVELSBSTRING = "universal"
TARGET_SYS = "arm-poky-linux-gnueabi"
MACHINE = "am437x-evm"
DISTRO = "poky"
DISTRO_VERSION = "3.0.2"
TUNE_FEATURES = "arm armv7a vfp thumb neon callconvention-hard"
TARGET_FPU = "hard"
meta
meta-poky
meta-yocto-bsp = "zeus:5e1f52edb7a9f790fb6cb5d96502f3690267c1b1"
meta-oe
meta-networking
meta-python = "zeus:bb65c27a772723dfe2c15b5e1b27bcc1a1ed884c"
meta-ti = "zeus:19a73c96bd05a74711a36a61077e8a7b88b55c63"
See below for the error messgage.
Thanks and kind regards
Daniel
ERROR: ti-sgx-ddk-km-1.17.4948957-r3v do_compile: oe_runmake failed
ERROR: ti-sgx-ddk-km-1.17.4948957-r3v do_compile: Execution of '/home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v/temp/run.do_compile.19460' failed with exit code 1:
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/amba: Is a directory
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/avf: Is a directory
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/bcma: Is a directory
.
.
.
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/unaligned: Is a directory
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/usb: Is a directory
grep: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source/include/linux/wimax: Is a directory
../config/compiler.mk:96: host gcc
../config/compiler.mk:185: target arm-poky-linux-gnueabi-gcc -mno-thumb-interwork -marm -fuse-ld=bfd -fmacro-prefix-map=/home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v=/usr/src/debug/ti-sgx-ddk-km/1.17.4948957-r3v -fdebug-prefix-map=/home/daniel/workspace/reference/am335x-evm-poky
/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v=/usr/src/debug/ti-sgx-ddk-km/1.17.4948957-r3v -fdebug-prefix-map=/home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v/recipe-sysroot= -fdebug-prefix-map=/home/daniel/workspace/reference/am3
35x-evm-poky/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v/recipe-sysroot-native= -fdebug-prefix-map=/home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work-shared/am437x-evm/kernel-source=/usr/src/kernel
******* Multiarch build: no
******* Primary arch: target_armel
******* Secondary arch: none
../config/core.mk:513: $(KERNELDIR)/vmlinux does not exist. Kbuild may fail.
eurasiacon/build/linux2/toplevel.mk:230: eurasiacon/build/linux2/moduledefs/target_armel.mk: No such file or directory
make[1]: *** No rule to make target 'eurasiacon/build/linux2/moduledefs/target_armel.mk'. Stop.
make: *** [../config/core.mk:789: build] Error 2
WARNING: exit code 1 from a shell command.
ERROR: Logfile of failure stored in: /home/daniel/workspace/reference/am335x-evm-poky/build/tmp/work/am437x_evm-poky-linux-gnueabi/ti-sgx-ddk-km/1.17.4948957-r3v/temp/log.do_compile.19460
On 2/19/20 8:00 PM, Denys Dmytriyenko wrote:
> Daniel,
>
> You are not providing any useful info, such as error messages or logs.
>
> In general, yes, am437x-evm on Zeus is ready and has been building fine in
> many different configurations.
>
> Denys
>
>
> On Wed, Feb 19, 2020 at 11:27:55AM +0100, Daniel Ammann wrote:
>> Hi all
>>
>> I'm trying to build a zeus image for am437x-evm. How do I go about that? Is
>> meta-ti branch zeus ready yet?
>>
>> First, I tried a simple poky build with meta-openembedded and meta-ti, building
>> core-image-base. But compiling of ti-sgx-ddk-km fails.
>>
>> Then I followed the instructions in the arago wiki [1], but the build fails
>> with qtbase because qtbase doesn't run configure successfully.
>>
>> What is the recommended way to compile a development image? The goal is to compile
>> a qt5 application with qtwebengine that needs eglfs support.
>>
>> [1] http://arago-project.org/wiki/index.php/Setting_Up_Build_Environment
>>
>> Thanks and kind regards
>>
>> Daniel
--
bytes at work
Technoparkstrasse 7
CH-8406 Winterthur
Switzerland
phone: +41 52 550 50 67
^ permalink raw reply
* Re: [PATCH] mm: Fix possible PMD dirty bit lost in set_pmd_migration_entry()
From: William Kucharski @ 2020-02-20 10:22 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Zi Yan,
Kirill A . Shutemov, Andrea Arcangeli, Michal Hocko,
Vlastimil Babka
In-Reply-To: <20200220075220.2327056-1-ying.huang@intel.com>
> On Feb 20, 2020, at 12:52 AM, Huang, Ying <ying.huang@intel.com> wrote:
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Looks good to me.
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> ---
> mm/huge_memory.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 580098e115bd..b1e069e68189 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3060,8 +3060,7 @@ void set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw,
> return;
>
> flush_cache_range(vma, address, address + HPAGE_PMD_SIZE);
> - pmdval = *pvmw->pmd;
> - pmdp_invalidate(vma, address, pvmw->pmd);
> + pmdval = pmdp_invalidate(vma, address, pvmw->pmd);
> if (pmd_dirty(pmdval))
> set_page_dirty(page);
> entry = make_migration_entry(page, pmd_write(pmdval));
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
From: Andy Shevchenko @ 2020-02-20 10:22 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
Jiri Slaby, Linux LED Subsystem, Linux Kernel Mailing List,
Sascha Hauer, open list:SERIAL DRIVERS
In-Reply-To: <20200220074901.ohcrisjgd26555ya@pengutronix.de>
On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > >
> > > This function is in the same spirit as the other kstrto* functions and
> > > uses the same calling convention. It expects the input string to be in
> > > the format %u:%u and implements stricter parsing than sscanf as it
> > > returns an error on trailing data (other than the usual \n).
...
> > On top of that, why kstrtodev_t is so important? How many users are
> > already in the kernel to get an advantage out of it?
>
> Does it need to be important? It matches the other kstrto* functions and
> so it seemed more natural to me to put it near the other functions. I'm
> not aware of other potential users and surprised you seem to suggest
> this as a requirement.
Yes it does. The kstrtox() are quite generic, what you are proposing
is rather one particular case with blurry understanding how many users
will be out of it.
If you had told "look, we have 1234 users which may benefit out of
it", I would have given no comment against.
> > What to do with all other possible variants ("%d:%d", "%dx%d" and its
> > %u variant, etc)?
>
> I don't see how %d:%d is relevant, major and minor cannot be negative
> can they? I never saw 'x' as separator between major and minor. I
> considered shortly parsing %u, but given that (I think) this is an
> internal representation only I chose to not make it more visible than it
> already is.
See above, if we are going to make it generic, perhaps better to cover
more possible users, right?
Otherwise your change provokes pile of (replaced)
kstrto_resolution() /* %ux:%u */
kstrto_range() /* %d:%d */
kstrto_you_name_it()
> > Why simple_strto*() can't be used?
>
> I didn't really consider it, but looking in more detail I don't like it
> much. Without having tried it I think simple_strtoull accepts
> "1000000000000000000000000000000000000000000" returning some arbitrary
> value without an error indication.
So what? User has a lot of possibilities to shoot into the foot.
Since you interpret this as device major:minor, not founding a device
will be first level of error, next one when your code will try to do
something out of it. It shouldn't be a problem of kstrtox generic
helpers.
> And given that I was asked for strict
> parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
> is a step backwards. Also simple_strtoul() has "This function is obsolete.
> Please use kstrtoul instead." in its docstring which seems to apply to
> the other simple_strto*() functions, too.
I specifically fixed a doc string to approve its use in the precisely
cases you have here.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [Xen-devel] [PATCH V3] x86/altp2m: Hypercall to set altp2m view visibility
From: Jan Beulich @ 2020-02-20 10:23 UTC (permalink / raw)
To: Alexandru Stefan ISAILA
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Jun Nakajima, xen-devel@lists.xenproject.org,
Roger Pau Monné
In-Reply-To: <1b42e2e2-1559-9014-1022-e5d5bf65eaa6@bitdefender.com>
On 20.02.2020 10:59, Alexandru Stefan ISAILA wrote:
> On 19.02.2020 19:00, Jan Beulich wrote:
>> On 19.02.2020 10:18, Alexandru Stefan ISAILA wrote:
>>> @@ -4835,6 +4836,23 @@ static int do_altp2m_op(
>>> break;
>>> }
>>>
>>> + case HVMOP_altp2m_set_visibility:
>>> + {
>>> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>>> +
>>> + if ( a.u.set_visibility.pad )
>>> + rc = -EINVAL;
>>> + else if ( !altp2m_active(d) )
>>> + rc = -EOPNOTSUPP;
>>> + else if ( a.u.set_visibility.visible )
>>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>>> + d->arch.altp2m_eptp[altp2m_idx];
>>> + else
>>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>>> + mfn_x(INVALID_MFN);
>>
>> Don't you need to bounds check the index before its use?
>
> Unless we want a index out of bounds from the user. Sorry for not having
> that, I will add a "altp2m_eptp[array_index_nospec(altp2m_idx,
> MAX_EPTP)]" in place for the next version.
>
>> And
>> shouldn't you return an error also for in-range ones which
>> aren't actually valid?
> That is a good thing. Maybe -EINVAL could fit this?
Sure.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [PATCH v2 1/2] docs: Convert qemu-cpu-models.texi to rST
From: Kashyap Chamarthy @ 2020-02-20 10:23 UTC (permalink / raw)
To: Peter Maydell
Cc: armbru, Daniel P. Berrange, QEMU Developers, Eduardo Habkost
In-Reply-To: <CAFEAcA_M3s7U_Brx4iZcWKbrNesn5z33C2Cz+jr1PxCNGTvaXg@mail.gmail.com>
On Wed, Feb 19, 2020 at 05:57:16PM +0000, Peter Maydell wrote:
> On Wed, 19 Feb 2020 at 16:40, Kashyap Chamarthy <kchamart@redhat.com> wrote:
> > Peter, how are you able to generate those *.7 `nroff` man pages? When I
> > do 'make sphinxbuild' from my build dir ($HOME/buld/qemu), I only see:
>
> Just run 'make'.
I did that, but I was missing the "--enable-docs" with my `configure`
invocation (which I didn't post in full on the list, but only on IRC
yesterday). Thanks, Markus, for spotting that.
$> cd ~/build/qemu
$> ~/src/qemu//configure --target-list=x86_64-softmmu \
--enable-debug --enable-docs
$> make -j8
That did the trick:
# From the build dir
$> find -name \*.7
./docs/interop/qemu-ga-ref.7
./docs/interop/qemu-qmp-ref.7
./docs/system/qemu-block-drivers.7
./docs/system/qemu-cpu-models.7
> As with the existing texinfo manpages, there
> is no specific makefile target for "build the manpages and
> nothing else".
Yeah, noticed that from the Makefile.
All sorted now, thanks!
> thanks
> -- PMM
>
--
/kashyap
^ permalink raw reply
* Re: [PATCH kvmtool v2] Add emulation for CFI compatible flash memory
From: Alexandru Elisei @ 2020-02-20 10:24 UTC (permalink / raw)
To: Andre Przywara
Cc: Raphael Gault, Sami Mujawar, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20200219172639.6c98334b@donnerap.cambridge.arm.com>
Hi,
On 2/19/20 5:26 PM, Andre Przywara wrote:
> On Mon, 17 Feb 2020 17:20:43 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> I guess the device hasn't been tested with Linux. This is what I'm getting when
>> trying to boot a Linux guest using the command:
> It was actually developed with a Linux guest, because that's more verbatim and easier to debug.
>
> And I just tested this again with Linux and it worked for me:
The flash image you provided is 2 MB. The flash image that I used is 10 MB (it
shows in the log that I sent). I guess you ran a different test.
> [ 2.164992] physmap-flash 20000.flash: physmap platform flash device: [mem 0x00020000-0x0021ffff]
> [ 2.166539] 20000.flash: Found 2 x16 devices at 0x0 in 32-bit bank. Manufacturer ID 0x000000 Chip ID 0x00ffff
> ...
> # mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NORFLASH
> mtd.size = 2097152 (2M)
> mtd.erasesize = 65536 (64K)
> mtd.writesize = 1
> mtd.oobsize = 0
> regions = 1
>
> I think what you are seeing are problems when you give a non-power-of-2 sized flash image. The current patch does not really support this (since it's hardly a thing in the real world). I originally wanted to expand any "uneven" size to the next power-of-2, but this doesn't work easily with mmap.
I would expect that if kvmtool allows the user to specify a non-power-of-2 flash
image size, then it should know how to deal with it and not present a broken
device to a linux guest if that size is forbidden by the spec. Or it is allowed by
the specification and kvmtool doesn't know how to deal with it?
Instead of expanding the file provided by the user to fit a bigger flash, how
about you use the highest power of two size that is smaller than the flash size?
> So I now changed the code to downgrade, so you get 8MB with any file ranging from [8MB, 16MB(, for instance.
> That fixed the Linux problems with those files for me.
>
>> $ ./lkvm run -c4 -m4096 -k /path/to/kernel -d /path/to/disk -p root="/dev/vda2" -F
>> flash.img
>>
>> [ 0.659167] physmap-flash 2000000.flash: physmap platform flash device: [mem
>> 0x02000000-0x029fffff]
>> [ 0.660444] Number of erase regions: 1
>> [ 0.661036] Primary Vendor Command Set: 0001 (Intel/Sharp Extended)
>> [ 0.661688] Primary Algorithm Table at 0031
>> [ 0.662168] Alternative Vendor Command Set: 0000 (None)
>> [ 0.662711] No Alternate Algorithm Table
>> [ 0.663120] Vcc Minimum: 4.5 V
>> [ 0.663450] Vcc Maximum: 5.5 V
>> [ 0.663779] No Vpp line
>> [ 0.664039] Typical byte/word write timeout: 2 µs
>> [ 0.664590] Maximum byte/word write timeout: 2 µs
>> [ 0.665240] Typical full buffer write timeout: 2 µs
>> [ 0.665775] Maximum full buffer write timeout: 2 µs
>> [ 0.666373] Typical block erase timeout: 2 ms
>> [ 0.666828] Maximum block erase timeout: 2 ms
>> [ 0.667282] Chip erase not supported
>> [ 0.667659] Device size: 0x800000 bytes (8 MiB)
>> [ 0.668137] Flash Device Interface description: 0x0006
>> [ 0.668697] - Unknown
>> [ 0.668963] Max. bytes in buffer write: 0x40
>> [ 0.669407] Number of Erase Block Regions: 1
>> [ 0.669865] Erase Region #0: BlockSize 0x8000 bytes, 160 blocks
>> [ 0.672299] 2000000.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
>> Manufacturer ID 0x000000 Chip ID 0x00ffff
>> [ 0.681328] NOR chip too large to fit in mapping. Attempting to cope...
>> [ 0.682046] Intel/Sharp Extended Query Table at 0x0031
>> [ 0.682645] Using buffer write method
>> [ 0.683031] Sum of regions (a00000) != total size of set of interleaved chips
>> (1000000)
>> [ 0.683854] gen_probe: No supported Vendor Command Set found
>> [ 0.684441] physmap-flash 2000000.flash: map_probe failed
>>
>> I also defined DEBUG_CFI in drivers/mtd/chips/cfi_probe.c.
>>
>> The Flash Device Interface description that we provide is wrong, it should 0x05.
>> More details below.
>>
>> On 2/7/20 12:19 PM, Andre Przywara wrote:
>>> From: Raphael Gault <raphael.gault@arm.com>
>>>
>>> The EDK II UEFI firmware implementation requires some storage for the EFI
>>> variables, which is typically some flash storage.
>>> Since this is already supported on the EDK II side, we add a CFI flash
>>> emulation to kvmtool.
>>> This is backed by a file, specified via the --flash or -F command line
>>> option. Any flash writes done by the guest will immediately be reflected
>>> into this file (kvmtool mmap's the file).
>>>
>>> This implements a CFI flash using the "Intel/Sharp extended command
>>> set", as specified in:
>>> - JEDEC JESD68.01
>>> - JEDEC JEP137B
>>> - Intel Application Note 646
>>> Some gaps in those specs have been filled by looking at real devices and
>>> other implementations (QEMU, Linux kernel driver).
>>>
>>> At the moment this relies on DT to advertise the base address of the
>>> flash memory (mapped into the MMIO address space) and is only enabled
>>> for ARM/ARM64. The emulation itself is architecture agnostic, though.
>>>
>>> This is one missing piece toward a working UEFI boot with kvmtool on
>>> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>>
>>> Signed-off-by: Raphael Gault <raphael.gault@arm.com>
>>> [Andre: rewriting and fixing]
>>> Signed-off-by: Andre Przywra <andre.przywara@arm.com>
>>> ---
>>> Hi,
>>>
>>> an update addressing Will's comments. I added coarse grained locking
>>> to the MMIO handler, to prevent concurrent vCPU accesses from messing up
>>> the internal CFI flash state machine.
>>> I also folded the actual flash array read access into the MMIO handler
>>> and fixed the other small issues.
>>>
>>> Cheers,
>>> Andre
>>>
>>> Makefile | 6 +
>>> arm/include/arm-common/kvm-arch.h | 3 +
>>> builtin-run.c | 2 +
>>> hw/cfi_flash.c | 546 ++++++++++++++++++++++++++++++
>>> include/kvm/kvm-config.h | 1 +
>>> include/kvm/util.h | 5 +
>>> 6 files changed, 563 insertions(+)
>>> create mode 100644 hw/cfi_flash.c
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 3862112c..7ed6fb5e 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm)
>>> CFLAGS += -march=armv7-a
>>>
>>> ARCH_WANT_LIBFDT := y
>>> + ARCH_HAS_FLASH_MEM := y
>>> endif
>>>
>>> # ARM64
>>> @@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64)
>>> ARCH_INCLUDE += -Iarm/aarch64/include
>>>
>>> ARCH_WANT_LIBFDT := y
>>> + ARCH_HAS_FLASH_MEM := y
>>> endif
>>>
>>> ifeq ($(ARCH),mips)
>>> @@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
>>> endif
>>> endif
>>>
>>> +ifeq (y,$(ARCH_HAS_FLASH_MEM))
>>> + OBJS += hw/cfi_flash.o
>>> +endif
>>> +
>>> ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
>>> CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB
>>> LIBS_DYNOPT += -lz
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index b9d486d5..2bb085f4 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -21,6 +21,9 @@
>>> #define ARM_GIC_DIST_SIZE 0x10000
>>> #define ARM_GIC_CPUI_SIZE 0x20000
>>>
>>> +#define ARM_FLASH_MMIO_BASE 0x2000000 /* 32 MB */
>>> +#define KVM_FLASH_MMIO_BASE ARM_FLASH_MMIO_BASE
>>> +
>>> #define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
>>> #define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>> #define ARM_PCI_CFG_SIZE (1ULL << 24)
>>> diff --git a/builtin-run.c b/builtin-run.c
>>> index f8dc6c72..df8c6741 100644
>>> --- a/builtin-run.c
>>> +++ b/builtin-run.c
>>> @@ -138,6 +138,8 @@ void kvm_run_set_wrapper_sandbox(void)
>>> "Kernel command line arguments"), \
>>> OPT_STRING('f', "firmware", &(cfg)->firmware_filename, "firmware",\
>>> "Firmware image to boot in virtual machine"), \
>>> + OPT_STRING('F', "flash", &(cfg)->flash_filename, "flash",\
>>> + "Flash image to present to virtual machine"), \
>>> \
>>> OPT_GROUP("Networking options:"), \
>>> OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params", \
>>> diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
>>> new file mode 100644
>>> index 00000000..d7c0e7e8
>>> --- /dev/null
>>> +++ b/hw/cfi_flash.c
>>> @@ -0,0 +1,546 @@
>>> +#include <stdbool.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/err.h>
>>> +#include <linux/sizes.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "kvm/kvm.h"
>>> +#include "kvm/kvm-arch.h"
>>> +#include "kvm/devices.h"
>>> +#include "kvm/fdt.h"
>>> +#include "kvm/mutex.h"
>>> +#include "kvm/util.h"
>>> +
>>> +/* The EDK2 driver hardcodes two 16-bit chips on a 32-bit bus. */
>>> +#define CFI_NR_FLASH_CHIPS 2
>>> +
>>> +/* We always emulate a 32 bit bus width. */
>>> +#define CFI_BUS_WIDTH 4
>>> +
>>> +/* The *effective* size of an erase block (over all chips) */
>>> +#define FLASH_BLOCK_SIZE SZ_64K
>>> +
>>> +#define PROGRAM_BUFF_SIZE_BITS 7
>>> +#define PROGRAM_BUFF_SIZE (1U << PROGRAM_BUFF_SIZE_BITS)
>>> +
>>> +/* CFI commands */
>>> +#define CFI_CMD_LOCK_BLOCK 0x01
>>> +#define CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP 0x10
>>> +#define CFI_CMD_BLOCK_ERASE_SETUP 0x20
>>> +#define CFI_CMD_WORD_PROGRAM_SETUP 0x40
>>> +#define CFI_CMD_CLEAR_STATUS_REGISTER 0x50
>>> +#define CFI_CMD_LOCK_BLOCK_SETUP 0x60
>>> +#define CFI_CMD_READ_STATUS_REGISTER 0x70
>>> +#define CFI_CMD_READ_JEDEC 0x90
>>> +#define CFI_CMD_READ_CFI_QUERY 0x98
>>> +#define CFI_CMD_BUFFERED_PROGRAM_CONFIRM 0xd0
>>> +#define CFI_CMD_BLOCK_ERASE_CONFIRM 0xd0
>>> +#define CFI_CMD_UNLOCK_BLOCK 0xd0
>>> +#define CFI_CMD_BUFFERED_PROGRAM_SETUP 0xe8
>>> +#define CFI_CMD_READ_ARRAY 0xff
>>> +
>>> +/*
>>> + * CFI query table contents, as far as it is constant.
>>> + */
>>> +#define CFI_GEOM_OFFSET 0x27
>>> +static u8 cfi_query_table[] = {
>>> + /* offset 0x10: CFI query identification string */
>>> + 'Q', 'R', 'Y', /* ID string */
>>> + 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>>> + 0x31, 0x00, /* address of primary extended query table */
>>> + 0x00, 0x00, /* alternative command set: unused */
>>> + 0x00, 0x00, /* address of alternative extended query table*/
>>> + /* offset 0x1b: system interface information */
>>> + 0x45, /* minimum Vcc voltage: 4.5V */
>>> + 0x55, /* maximum Vcc voltage: 5.5V */
>>> + 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>>> + 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>>> + 0x01, /* timeout for single word program: 2 us */
>>> + 0x01, /* timeout for multi-byte program: 2 us */
>>> + 0x01, /* timeout for block erase: 2 ms */
>>> + 0x00, /* timeout for full chip erase: not supported */
>>> + 0x00, /* max timeout for single word program: 1x */
>>> + 0x00, /* max timeout for mulit-byte program: 1x */
>>> + 0x00, /* max timeout for block erase: 1x */
>>> + 0x00, /* max timeout for chip erase: not supported */
>>> + /* offset 0x27: flash geometry information */
>>> + 0x00, /* size in power-of-2 bytes, filled later */
>>> + 0x06, 0x00, /* interface description: 32 and 16 bits */
>> I don't think this is correct. From Intel StrataFlash Embedded Memory (P30)
>> Family, table 34:
>>
>> ""n" such that n+1 specifies the bit field that represents the flash device width
>> capabilities as described in the table".
> Yeah, seems to be correct, but it looks this Intel Strata document is the only place which details this encoding (which looks like being retrofit somehow).
> And I didn't really use this document, because it's a manufacturer data sheet and not a specification.
The device is in the list of specification you provided in the commit message. I
think it would make the reviewers' life a lot easier if you posted all the
documentation that you used, and drop documentation that you didn't. Where did you
get the value 0x06 from? That was the only document from where I could infer what
it means, maybe I didn't dig deep enough.
> I will change it to 0x5, but for the records Linux worked even with 0x6 for me.
I would say that in this case working != correct, because Linux 5.6-rc2 defines
0x05 as a x32/x16 interface, and 0x06 is undefined in the file that I mentioned in
the previous reply. Did you check to see if the Linux driver recognized that
interface type? Maybe it changed between versions. I also tried 0x00,0x00 for the
interface description and Linux also worked.
Either way, I followed the trail of breadcrumbs starting at the comment for the
define and I found this in Common Flash Memory Interface Specification release
2.0, Appendix:
"Note: April 2000 -x16/x32 devices will be represented by hex value 0005h as
requested by Intel in order to make them more software friendly. Changes will be
made to the CFI drivers so that a bit-wise switch is created to represent
different data widths. [..] For example, if we take the description for an x16/x32
device (0005h) and we convert that to binary we get 0101b. If we add one to this
value we get a bit pattern that looks like this: 0110b. This bit-wise switch
indicates that the device a x16/x32 device".
I guess the reason for the inconsistencies is that at some point it used to be
different, but it was changed because Intel requested it.
>> If you want to advertise 32 and 16 bit write capabilities, it should be 5 because
>> 5+1=6. This is also the value that the Linux kernel checks for (see
>> include/linux/mtd/cfi.h, define CFI_INTERFACE_X16_BY_X32_ASYNC"). 6 actually means
>> 32, 16 and 8 bit accesses.
>>
>> This begs another question: why do we support both 16 and 32 bit accesses instead
>> of supporting only 32 bit?
> Because we can, there is no reason to restrict this. I feel like we should be as capable as possible, especially since it's trivial to emulate.
Makes sense.
>
>>> + PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>>> + /* number of multi-byte writes */
>> Shouldn't the comment be maximum number of bytes in the write buffer?
> Yes, possibly.
>
>>> + 0x01, /* one erase block region */
>>> + 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>>> + /* offset 0x31: Intel primary algorithm extended query table */
>>> + 'P', 'R', 'I',
>>> + '1', '0', /* version 1.0 */
>>> + 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>>> + 0x00, /* no functions after suspend */
>>> + 0x01, 0x00, /* only lock bit supported */
>>> + 0x50, /* best Vcc value: 5.0V */
>>> + 0x00, /* best Vpp value: 0.0V (unused) */
>>> + 0x01, /* number of protection register fields */
>>> + 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>>> +};
>> As an aside, I found it impossible to review the cfi_query_table array in its
>> current form. This is how I wrote the array so I could read it. I also took the
>> liberty to remove the offset when indexing the array, making read_cfi less error
>> prone, in my opinion:
> Please don't post elaborate code sequences as a comment, especially not if it gets mangled (Thunderbird is annoyingly bad in this respect).
I use Thunderbird and it showed fine for me, and I have sent large diffs before in
replies and I got no complaints. I use the settings from
Documentation/process/email-clients.rst.
Thanks,
Alex
> I think I would have got what you mean by showing just one line ;-)
>
> Cheers,
> Andre
>
>> diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
>> index d7c0e7e80d69..65a90e288be8 100644
>> --- a/hw/cfi_flash.c
>> +++ b/hw/cfi_flash.c
>> @@ -46,45 +46,43 @@
>> */
>> #define CFI_GEOM_OFFSET 0x27
>> static u8 cfi_query_table[] = {
>> - /* offset 0x10: CFI query identification string */
>> - 'Q', 'R', 'Y', /* ID string */
>> - 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> - 0x31, 0x00, /* address of primary extended query table */
>> - 0x00, 0x00, /* alternative command set: unused */
>> - 0x00, 0x00, /* address of alternative extended query table*/
>> - /* offset 0x1b: system interface information */
>> - 0x45, /* minimum Vcc voltage: 4.5V */
>> - 0x55, /* maximum Vcc voltage: 5.5V */
>> - 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> - 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> - 0x01, /* timeout for single word program: 2 us */
>> - 0x01, /* timeout for multi-byte program: 2 us */
>> - 0x01, /* timeout for block erase: 2 ms */
>> - 0x00, /* timeout for full chip erase: not supported */
>> - 0x00, /* max timeout for single word program: 1x */
>> - 0x00, /* max timeout for mulit-byte program: 1x */
>> - 0x00, /* max timeout for block erase: 1x */
>> - 0x00, /* max timeout for chip erase: not supported */
>> - /* offset 0x27: flash geometry information */
>> - 0x00, /* size in power-of-2 bytes, filled later */
>> - 0x06, 0x00, /* interface description: 32 and 16 bits */
>> - PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> + [0x10] = 'Q', 'R', 'Y', /* ID string */
>> + [0x13] = 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> + [0x15] = 0x31, 0x00, /* address of primary extended query table */
>> + [0x17] = 0x00, 0x00, /* alternative command set: unused */
>> + [0x19] = 0x00, 0x00, /* address of alternative extended query table*/
>> + /* System interface information */
>> + [0x1b] = 0x45, /* minimum Vcc voltage: 4.5V */
>> + [0x1c] = 0x55, /* maximum Vcc voltage: 5.5V */
>> + [0x1d] = 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> + [0x1e] = 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> + [0x1f] = 0x01, /* timeout for single word program: 2 us */
>> + [0x20] = 0x01, /* timeout for multi-byte program: 2 us */
>> + [0x21] = 0x01, /* timeout for block erase: 2 ms */
>> + [0x22] = 0x00, /* timeout for full chip erase: not supported */
>> + [0x23] = 0x00, /* max timeout for single word program: 1x */
>> + [0x24] = 0x00, /* max timeout for mulit-byte program: 1x */
>> + [0x25] = 0x00, /* max timeout for block erase: 1x */
>> + [0x26] = 0x00, /* max timeout for chip erase: not supported */
>> + /* Flash geometry information */
>> + [0x27] = 0x00, /* size in power-of-2 bytes, filled later */
>> + [0x28] = 0x06, 0x00, /* interface description: 32 and 16 bits */
>> + [0x2a] = PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> /* number of multi-byte writes */
>> - 0x01, /* one erase block region */
>> - 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> - /* offset 0x31: Intel primary algorithm extended query table */
>> - 'P', 'R', 'I',
>> - '1', '0', /* version 1.0 */
>> - 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>> - 0x00, /* no functions after suspend */
>> - 0x01, 0x00, /* only lock bit supported */
>> - 0x50, /* best Vcc value: 5.0V */
>> - 0x00, /* best Vpp value: 0.0V (unused) */
>> - 0x01, /* number of protection register fields */
>> - 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>> + [0x2c] = 0x01, /* one erase block region */
>> + [0x2d] = 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> + /* Intel primary algorithm extended query table */
>> + [0x31] = 'P', 'R', 'I', /* ID string */
>> + [0x34] = '1', '0', /* version 1.0 */
>> + [0x36] = 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock &
>> pm-read */
>> + [0x40] = 0x00, /* no functions after suspend */
>> + [0x41] = 0x01, 0x00, /* only lock bit supported */
>> ...skipping...
>> + [0x10] = 'Q', 'R', 'Y', /* ID string */
>> + [0x13] = 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> + [0x15] = 0x31, 0x00, /* address of primary extended query table */
>> + [0x17] = 0x00, 0x00, /* alternative command set: unused */
>> + [0x19] = 0x00, 0x00, /* address of alternative extended query table*/
>> + /* System interface information */
>> + [0x1b] = 0x45, /* minimum Vcc voltage: 4.5V */
>> + [0x1c] = 0x55, /* maximum Vcc voltage: 5.5V */
>> + [0x1d] = 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> + [0x1e] = 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> + [0x1f] = 0x01, /* timeout for single word program: 2 us */
>> + [0x20] = 0x01, /* timeout for multi-byte program: 2 us */
>> + [0x21] = 0x01, /* timeout for block erase: 2 ms */
>> + [0x22] = 0x00, /* timeout for full chip erase: not supported */
>> + [0x23] = 0x00, /* max timeout for single word program: 1x */
>> + [0x24] = 0x00, /* max timeout for mulit-byte program: 1x */
>> + [0x25] = 0x00, /* max timeout for block erase: 1x */
>> + [0x26] = 0x00, /* max timeout for chip erase: not supported */
>> + /* Flash geometry information */
>> + [0x27] = 0x00, /* size in power-of-2 bytes, filled later */
>> + [0x28] = 0x06, 0x00, /* interface description: 32 and 16 bits */
>> + [0x2a] = PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> /* number of multi-byte writes */
>> - 0x01, /* one erase block region */
>> - 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> - /* offset 0x31: Intel primary algorithm extended query table */
>> - 'P', 'R', 'I',
>> - '1', '0', /* version 1.0 */
>> - 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>> - 0x00, /* no functions after suspend */
>> - 0x01, 0x00, /* only lock bit supported */
>> - 0x50, /* best Vcc value: 5.0V */
>> - 0x00, /* best Vpp value: 0.0V (unused) */
>> - 0x01, /* number of protection register fields */
>> - 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>> + [0x2c] = 0x01, /* one erase block region */
>> + [0x2d] = 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> + /* Intel primary algorithm extended query table */
>> + [0x31] = 'P', 'R', 'I', /* ID string */
>> + [0x34] = '1', '0', /* version 1.0 */
>> + [0x36] = 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock &
>> pm-read */
>> + [0x40] = 0x00, /* no functions after suspend */
>> + [0x41] = 0x01, 0x00, /* only lock bit supported */
>> + [0x43] = 0x50, /* best Vcc value: 5.0V */
>> + [0x43] = 0x00, /* best Vpp value: 0.0V (unused) */
>> + [0x44] = 0x01, /* number of protection register fields */
>> + [0x45] = 0x00, 0x00, 0x00, 0x00,/* protection field 1 description */
>> };
>>
>> -
>> /*
>> * Those states represent a subset of the CFI flash state machine.
>> */
>> @@ -141,10 +139,7 @@ static int nr_erase_blocks(struct cfi_flash_device *sfdev)
>> */
>> static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>> {
>> - if (addr < 0x10) /* CFI information starts at 0x10 */
>> - return 0;
>> -
>> - if (addr - 0x10 > sizeof(cfi_query_table)) {
>> + if (addr > sizeof(cfi_query_table)) {
>> pr_debug("CFI query read access beyond the end of table");
>> return 0;
>> }
>> @@ -163,7 +158,7 @@ static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>> return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) >> 8;
>> }
>>
>> - return cfi_query_table[addr - 0x10];
>> + return cfi_query_table[addr];
>> }
>>
>> static bool block_is_locked(struct cfi_flash_device *sfdev, u64 addr)
>>
>> Thanks,
>> Alex
>>> +
>>> +
>>> +/*
>>> + * Those states represent a subset of the CFI flash state machine.
>>> + */
>>> +enum cfi_flash_state {
>>> + READY,
>>> + LOCK_SETUP,
>>> + WP_SETUP,
>>> + BP_SETUP,
>>> + BP_LOAD,
>>> + ERASE_SETUP,
>>> +};
>>> +
>>> +/*
>>> + * The device can be in several **Read** modes.
>>> + * We don't implement the asynchronous burst mode.
>>> + */
>>> +enum cfi_read_mode {
>>> + READ_ARRAY,
>>> + READ_STATUS,
>>> + READ_DEVICE_ID,
>>> + READ_QUERY,
>>> +};
>>> +
>>> +struct cfi_flash_device {
>>> + struct device_header dev_hdr;
>>> + /* Protects the CFI state machine variables in this data structure. */
>>> + struct mutex mutex;
>>> + u64 base_addr;
>>> + u32 size;
>>> +
>>> + void *flash_memory;
>>> + u8 program_buffer[PROGRAM_BUFF_SIZE * 4];
>>> + unsigned long *lock_bm;
>>> + u64 last_address;
>>> + unsigned int buff_written;
>>> + unsigned int program_length;
>>> +
>>> + enum cfi_flash_state state;
>>> + enum cfi_read_mode read_mode;
>>> + u16 rcr;
>>> + u8 sr;
>>> +};
>>> +
>>> +static int nr_erase_blocks(struct cfi_flash_device *sfdev)
>>> +{
>>> + return sfdev->size / FLASH_BLOCK_SIZE;
>>> +}
>>> +
>>> +/*
>>> + * CFI queries always deal with one byte of information, possibly mirrored
>>> + * to other bytes on the bus. This is dealt with in the callers.
>>> + * The address provided is the one for 8-bit addressing, and would need to
>>> + * be adjusted for wider accesses.
>>> + */
>>> +static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + if (addr < 0x10) /* CFI information starts at 0x10 */
>>> + return 0;
>>> +
>>> + if (addr - 0x10 > sizeof(cfi_query_table)) {
>>> + pr_debug("CFI query read access beyond the end of table");
>>> + return 0;
>>> + }
>>> +
>>> + /* Fixup dynamic information in the geometry part of the table. */
>>> + switch (addr) {
>>> + case CFI_GEOM_OFFSET: /* device size in bytes, power of two */
>>> + return pow2_size(sfdev->size / CFI_NR_FLASH_CHIPS);
>>> + case CFI_GEOM_OFFSET + 6: /* number of erase blocks, minus one */
>>> + return (nr_erase_blocks(sfdev) - 1) & 0xff;
>>> + case CFI_GEOM_OFFSET + 7:
>>> + return (nr_erase_blocks(sfdev) - 1) >> 8;
>>> + case CFI_GEOM_OFFSET + 8: /* erase block size, in units of 256 */
>>> + return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) & 0xff;
>>> + case CFI_GEOM_OFFSET + 9:
>>> + return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) >> 8;
>>> + }
>>> +
>>> + return cfi_query_table[addr - 0x10];
>>> +}
>>> +
>>> +static bool block_is_locked(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + int block_nr = addr / FLASH_BLOCK_SIZE;
>>> +
>>> + return test_bit(block_nr, sfdev->lock_bm);
>>> +}
>>> +
>>> +#define DEV_ID_MASK 0x7ff
>>> +static u16 read_dev_id(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + switch ((addr & DEV_ID_MASK) / CFI_BUS_WIDTH) {
>>> + case 0x0: /* vendor ID */
>>> + return 0x0000;
>>> + case 0x1: /* device ID */
>>> + return 0xffff;
>>> + case 0x2:
>>> + return block_is_locked(sfdev, addr & ~DEV_ID_MASK);
>>> + case 0x5:
>>> + return sfdev->rcr;
>>> + default: /* Ignore the other entries. */
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static void lock_block(struct cfi_flash_device *sfdev, u64 addr, bool lock)
>>> +{
>>> + int block_nr = addr / FLASH_BLOCK_SIZE;
>>> +
>>> + if (lock)
>>> + set_bit(block_nr, sfdev->lock_bm);
>>> + else
>>> + clear_bit(block_nr, sfdev->lock_bm);
>>> +}
>>> +
>>> +static void word_program(struct cfi_flash_device *sfdev,
>>> + u64 addr, void *data, int len)
>>> +{
>>> + if (block_is_locked(sfdev, addr)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> +
>>> + memcpy(sfdev->flash_memory + addr, data, len);
>>> +}
>>> +
>>> +/* Reset the program buffer state to prepare for follow-up writes. */
>>> +static void buffer_setup(struct cfi_flash_device *sfdev)
>>> +{
>>> + memset(sfdev->program_buffer, 0, sizeof(sfdev->program_buffer));
>>> + sfdev->last_address = ~0ULL;
>>> + sfdev->buff_written = 0;
>>> +}
>>> +
>>> +static bool buffer_program(struct cfi_flash_device *sfdev,
>>> + u64 addr, void *buffer, int len)
>>> +{
>>> + unsigned int buf_addr;
>>> +
>>> + if (sfdev->buff_written >= sfdev->program_length)
>>> + return false;
>>> +
>>> + /*
>>> + * The first word written into the buffer after the setup command
>>> + * happens to be the base address for the buffer.
>>> + * All subsequent writes need to be within this address and this
>>> + * address plus the buffer size, so keep this value around.
>>> + */
>>> + if (sfdev->last_address == ~0ULL)
>>> + sfdev->last_address = addr;
>>> +
>>> + if (addr < sfdev->last_address)
>>> + return false;
>>> + buf_addr = addr - sfdev->last_address;
>>> + if (buf_addr >= PROGRAM_BUFF_SIZE)
>>> + return false;
>>> +
>>> + memcpy(sfdev->program_buffer + buf_addr, buffer, len);
>>> + sfdev->buff_written++;
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static void buffer_confirm(struct cfi_flash_device *sfdev)
>>> +{
>>> + if (block_is_locked(sfdev, sfdev->last_address)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> + memcpy(sfdev->flash_memory + sfdev->last_address,
>>> + sfdev->program_buffer,
>>> + sfdev->buff_written * sizeof(u32));
>>> +}
>>> +
>>> +static void block_erase_confirm(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + if (block_is_locked(sfdev, addr)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> +
>>> + memset(sfdev->flash_memory + addr, 0xFF, FLASH_BLOCK_SIZE);
>>> +}
>>> +
>>> +static void cfi_flash_mmio(struct kvm_cpu *vcpu,
>>> + u64 addr, u8 *data, u32 len, u8 is_write,
>>> + void *context)
>>> +{
>>> + struct cfi_flash_device *sfdev = context;
>>> + u64 faddr = addr - sfdev->base_addr;
>>> + u32 value;
>>> +
>>> + if (!is_write) {
>>> + u16 cfi_value = 0;
>>> +
>>> + mutex_lock(&sfdev->mutex);
>>> +
>>> + switch (sfdev->read_mode) {
>>> + case READ_ARRAY:
>>> + /* just copy the requested bytes from the array */
>>> + memcpy(data, sfdev->flash_memory + faddr, len);
>>> + goto out_unlock;
>>> + case READ_STATUS:
>>> + cfi_value = sfdev->sr;
>>> + break;
>>> + case READ_DEVICE_ID:
>>> + cfi_value = read_dev_id(sfdev, faddr);
>>> + break;
>>> + case READ_QUERY:
>>> + cfi_value = read_cfi(sfdev, faddr / CFI_BUS_WIDTH);
>>> + break;
>>> + }
>>> + switch (len) {
>>> + case 1:
>>> + *data = cfi_value;
>>> + break;
>>> + case 8: memset(data + 4, 0, 4);
>>> + /* fall-through */
>>> + case 4:
>>> + if (CFI_NR_FLASH_CHIPS == 2)
>>> + memcpy(data + 2, &cfi_value, 2);
>>> + else
>>> + memset(data + 2, 0, 2);
>>> + /* fall-through */
>>> + case 2:
>>> + memcpy(data, &cfi_value, 2);
>>> + break;
>>> + default:
>>> + pr_debug("CFI flash: illegal access length %d for read mode %d",
>>> + len, sfdev->read_mode);
>>> + break;
>>> + }
>>> +
>>> + goto out_unlock;
>>> + }
>>> +
>>> + if (len > 4) {
>>> + pr_info("CFI flash: MMIO %d-bit write access not supported",
>>> + len * 8);
>>> + return;
>>> + }
>>> +
>>> + memcpy(&value, data, len);
>>> +
>>> + mutex_lock(&sfdev->mutex);
>>> +
>>> + switch (sfdev->state) {
>>> + case READY: /* handled below */
>>> + break;
>>> +
>>> + case LOCK_SETUP:
>>> + switch (value & 0xff) {
>>> + case CFI_CMD_LOCK_BLOCK:
>>> + lock_block(sfdev, faddr, true);
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_UNLOCK_BLOCK:
>>> + lock_block(sfdev, faddr, false);
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + default:
>>> + sfdev->sr |= 0x30;
>>> + break;
>>> + }
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case WP_SETUP:
>>> + word_program(sfdev, faddr, data, len);
>>> + sfdev->read_mode = READ_STATUS;
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case BP_LOAD:
>>> + if (buffer_program(sfdev, faddr, data, len))
>>> + goto out_unlock;
>>> +
>>> + if ((value & 0xFF) == CFI_CMD_BUFFERED_PROGRAM_CONFIRM) {
>>> + buffer_confirm(sfdev);
>>> + sfdev->read_mode = READ_STATUS;
>>> + } else {
>>> + pr_debug("CFI flash: BP_LOAD: expected CONFIRM(0xd0), got 0x%x @ 0x%llx",
>>> + value, faddr);
>>> + sfdev->sr |= 0x10;
>>> + }
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case BP_SETUP:
>>> + sfdev->program_length = (value & 0xffff) + 1;
>>> + if (sfdev->program_length > PROGRAM_BUFF_SIZE / 4)
>>> + sfdev->program_length = PROGRAM_BUFF_SIZE / 4;
>>> + sfdev->state = BP_LOAD;
>>> + sfdev->read_mode = READ_STATUS;
>>> + goto out_unlock;
>>> +
>>> + case ERASE_SETUP:
>>> + if ((value & 0xff) == CFI_CMD_BLOCK_ERASE_CONFIRM)
>>> + block_erase_confirm(sfdev, faddr);
>>> + else
>>> + sfdev->sr |= 0x30;
>>> +
>>> + sfdev->state = READY;
>>> + sfdev->read_mode = READ_STATUS;
>>> + goto out_unlock;
>>> + }
>>> +
>>> + /* write commands in READY state */
>>> + switch (value & 0xFF) {
>>> + case CFI_CMD_READ_JEDEC:
>>> + sfdev->read_mode = READ_DEVICE_ID;
>>> + break;
>>> + case CFI_CMD_READ_STATUS_REGISTER:
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_READ_CFI_QUERY:
>>> + sfdev->read_mode = READ_QUERY;
>>> + break;
>>> + case CFI_CMD_CLEAR_STATUS_REGISTER:
>>> + sfdev->sr = 0x80;
>>> + break;
>>> + case CFI_CMD_WORD_PROGRAM_SETUP:
>>> + case CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP:
>>> + sfdev->state = WP_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_LOCK_BLOCK_SETUP:
>>> + sfdev->state = LOCK_SETUP;
>>> + break;
>>> + case CFI_CMD_BLOCK_ERASE_SETUP:
>>> + sfdev->state = ERASE_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_BUFFERED_PROGRAM_SETUP:
>>> + buffer_setup(sfdev);
>>> + sfdev->state = BP_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_BUFFERED_PROGRAM_CONFIRM:
>>> + pr_debug("CFI flash: unexpected confirm command 0xD0");
>>> + break;
>>> + default:
>>> + pr_debug("CFI flash: unknown command 0x%x", value);
>>> + /* fall through */
>>> + case CFI_CMD_READ_ARRAY:
>>> + sfdev->read_mode = READ_ARRAY;
>>> + break;
>>> + }
>>> +
>>> +out_unlock:
>>> + mutex_unlock(&sfdev->mutex);
>>> +}
>>> +
>>> +#ifdef CONFIG_HAS_LIBFDT
>>> +static void generate_cfi_flash_fdt_node(void *fdt,
>>> + struct device_header *dev_hdr,
>>> + void (*generate_irq_prop)(void *fdt,
>>> + u8 irq,
>>> + enum irq_type))
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> + u64 reg_prop[2];
>>> +
>>> + sfdev = container_of(dev_hdr, struct cfi_flash_device, dev_hdr);
>>> + reg_prop[0] = cpu_to_fdt64(sfdev->base_addr);
>>> + reg_prop[1] = cpu_to_fdt64(sfdev->size);
>>> +
>>> + _FDT(fdt_begin_node(fdt, "flash"));
>>> + _FDT(fdt_property_cell(fdt, "bank-width", CFI_BUS_WIDTH));
>>> + _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
>>> + _FDT(fdt_property_cell(fdt, "#size-cells", 0x1));
>>> + _FDT(fdt_property_string(fdt, "compatible", "cfi-flash"));
>>> + _FDT(fdt_property_string(fdt, "label", "System-firmware"));
>>> + _FDT(fdt_property(fdt, "reg", ®_prop, sizeof(reg_prop)));
>>> + _FDT(fdt_end_node(fdt));
>>> +}
>>> +#else
>>> +#define generate_cfi_flash_fdt_node NULL
>>> +#endif
>>> +
>>> +static struct cfi_flash_device *create_flash_device_file(struct kvm *kvm,
>>> + const char *filename)
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> + struct stat statbuf;
>>> + unsigned int value;
>>> + int ret;
>>> + int fd;
>>> +
>>> + fd = open(filename, O_RDWR);
>>> + if (fd < 0)
>>> + return ERR_PTR(-errno);
>>> + if (fstat(fd, &statbuf) < 0) {
>>> + close(fd);
>>> + return ERR_PTR(-errno);
>>> + }
>>> +
>>> + sfdev = malloc(sizeof(struct cfi_flash_device));
>>> + if (!sfdev) {
>>> + close(fd);
>>> + return ERR_PTR(-ENOMEM);
>>> + }
>>> +
>>> + sfdev->size = (statbuf.st_size + 4095) & ~0xfffUL;
>>> + sfdev->flash_memory = mmap(NULL, statbuf.st_size,
>>> + PROT_READ | PROT_WRITE, MAP_SHARED,
>>> + fd, 0);
>>> + if (sfdev->flash_memory == MAP_FAILED) {
>>> + close(fd);
>>> + free(sfdev);
>>> + return ERR_PTR(-errno);
>>> + }
>>> + sfdev->base_addr = KVM_FLASH_MMIO_BASE;
>>> + sfdev->state = READY;
>>> + sfdev->read_mode = READ_ARRAY;
>>> + sfdev->sr = 0x80;
>>> + sfdev->rcr = 0xbfcf;
>>> +
>>> + value = roundup(nr_erase_blocks(sfdev), BITS_PER_LONG) / 8;
>>> + sfdev->lock_bm = malloc(value);
>>> + memset(sfdev->lock_bm, 0, value);
>>> +
>>> + sfdev->dev_hdr.bus_type = DEVICE_BUS_MMIO;
>>> + sfdev->dev_hdr.data = generate_cfi_flash_fdt_node;
>>> + mutex_init(&sfdev->mutex);
>>> + ret = device__register(&sfdev->dev_hdr);
>>> + if (ret) {
>>> + free(sfdev->flash_memory);
>>> + free(sfdev);
>>> + return ERR_PTR(ret);
>>> + }
>>> +
>>> + ret = kvm__register_mmio(kvm,
>>> + sfdev->base_addr, sfdev->size,
>>> + false, cfi_flash_mmio, sfdev);
>>> + if (ret) {
>>> + device__unregister(&sfdev->dev_hdr);
>>> + free(sfdev->flash_memory);
>>> + free(sfdev);
>>> + return ERR_PTR(ret);
>>> + }
>>> +
>>> + return sfdev;
>>> +}
>>> +
>>> +static int flash__init(struct kvm *kvm)
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> +
>>> + if (!kvm->cfg.flash_filename)
>>> + return 0;
>>> +
>>> + sfdev = create_flash_device_file(kvm, kvm->cfg.flash_filename);
>>> + if (IS_ERR(sfdev))
>>> + return PTR_ERR(sfdev);
>>> +
>>> + return 0;
>>> +}
>>> +dev_init(flash__init);
>>> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
>>> index a052b0bc..f4a8b831 100644
>>> --- a/include/kvm/kvm-config.h
>>> +++ b/include/kvm/kvm-config.h
>>> @@ -35,6 +35,7 @@ struct kvm_config {
>>> const char *vmlinux_filename;
>>> const char *initrd_filename;
>>> const char *firmware_filename;
>>> + const char *flash_filename;
>>> const char *console;
>>> const char *dev;
>>> const char *network;
>>> diff --git a/include/kvm/util.h b/include/kvm/util.h
>>> index 4ca7aa93..5c37f0b7 100644
>>> --- a/include/kvm/util.h
>>> +++ b/include/kvm/util.h
>>> @@ -104,6 +104,11 @@ static inline unsigned long roundup_pow_of_two(unsigned long x)
>>> return x ? 1UL << fls_long(x - 1) : 0;
>>> }
>>>
>>> +static inline int pow2_size(unsigned long x)
>>> +{
>>> + return (sizeof(x) * 8) - __builtin_clzl(x - 1);
>>> +}
>>> +
>>> struct kvm;
>>> void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size);
>>> void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 size);
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply
* Re: [PATCH kvmtool v2] Add emulation for CFI compatible flash memory
From: Alexandru Elisei @ 2020-02-20 10:24 UTC (permalink / raw)
To: Andre Przywara
Cc: Raphael Gault, Julien Thierry, Sami Mujawar, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20200219172639.6c98334b@donnerap.cambridge.arm.com>
Hi,
On 2/19/20 5:26 PM, Andre Przywara wrote:
> On Mon, 17 Feb 2020 17:20:43 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> I guess the device hasn't been tested with Linux. This is what I'm getting when
>> trying to boot a Linux guest using the command:
> It was actually developed with a Linux guest, because that's more verbatim and easier to debug.
>
> And I just tested this again with Linux and it worked for me:
The flash image you provided is 2 MB. The flash image that I used is 10 MB (it
shows in the log that I sent). I guess you ran a different test.
> [ 2.164992] physmap-flash 20000.flash: physmap platform flash device: [mem 0x00020000-0x0021ffff]
> [ 2.166539] 20000.flash: Found 2 x16 devices at 0x0 in 32-bit bank. Manufacturer ID 0x000000 Chip ID 0x00ffff
> ...
> # mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NORFLASH
> mtd.size = 2097152 (2M)
> mtd.erasesize = 65536 (64K)
> mtd.writesize = 1
> mtd.oobsize = 0
> regions = 1
>
> I think what you are seeing are problems when you give a non-power-of-2 sized flash image. The current patch does not really support this (since it's hardly a thing in the real world). I originally wanted to expand any "uneven" size to the next power-of-2, but this doesn't work easily with mmap.
I would expect that if kvmtool allows the user to specify a non-power-of-2 flash
image size, then it should know how to deal with it and not present a broken
device to a linux guest if that size is forbidden by the spec. Or it is allowed by
the specification and kvmtool doesn't know how to deal with it?
Instead of expanding the file provided by the user to fit a bigger flash, how
about you use the highest power of two size that is smaller than the flash size?
> So I now changed the code to downgrade, so you get 8MB with any file ranging from [8MB, 16MB(, for instance.
> That fixed the Linux problems with those files for me.
>
>> $ ./lkvm run -c4 -m4096 -k /path/to/kernel -d /path/to/disk -p root="/dev/vda2" -F
>> flash.img
>>
>> [ 0.659167] physmap-flash 2000000.flash: physmap platform flash device: [mem
>> 0x02000000-0x029fffff]
>> [ 0.660444] Number of erase regions: 1
>> [ 0.661036] Primary Vendor Command Set: 0001 (Intel/Sharp Extended)
>> [ 0.661688] Primary Algorithm Table at 0031
>> [ 0.662168] Alternative Vendor Command Set: 0000 (None)
>> [ 0.662711] No Alternate Algorithm Table
>> [ 0.663120] Vcc Minimum: 4.5 V
>> [ 0.663450] Vcc Maximum: 5.5 V
>> [ 0.663779] No Vpp line
>> [ 0.664039] Typical byte/word write timeout: 2 µs
>> [ 0.664590] Maximum byte/word write timeout: 2 µs
>> [ 0.665240] Typical full buffer write timeout: 2 µs
>> [ 0.665775] Maximum full buffer write timeout: 2 µs
>> [ 0.666373] Typical block erase timeout: 2 ms
>> [ 0.666828] Maximum block erase timeout: 2 ms
>> [ 0.667282] Chip erase not supported
>> [ 0.667659] Device size: 0x800000 bytes (8 MiB)
>> [ 0.668137] Flash Device Interface description: 0x0006
>> [ 0.668697] - Unknown
>> [ 0.668963] Max. bytes in buffer write: 0x40
>> [ 0.669407] Number of Erase Block Regions: 1
>> [ 0.669865] Erase Region #0: BlockSize 0x8000 bytes, 160 blocks
>> [ 0.672299] 2000000.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
>> Manufacturer ID 0x000000 Chip ID 0x00ffff
>> [ 0.681328] NOR chip too large to fit in mapping. Attempting to cope...
>> [ 0.682046] Intel/Sharp Extended Query Table at 0x0031
>> [ 0.682645] Using buffer write method
>> [ 0.683031] Sum of regions (a00000) != total size of set of interleaved chips
>> (1000000)
>> [ 0.683854] gen_probe: No supported Vendor Command Set found
>> [ 0.684441] physmap-flash 2000000.flash: map_probe failed
>>
>> I also defined DEBUG_CFI in drivers/mtd/chips/cfi_probe.c.
>>
>> The Flash Device Interface description that we provide is wrong, it should 0x05.
>> More details below.
>>
>> On 2/7/20 12:19 PM, Andre Przywara wrote:
>>> From: Raphael Gault <raphael.gault@arm.com>
>>>
>>> The EDK II UEFI firmware implementation requires some storage for the EFI
>>> variables, which is typically some flash storage.
>>> Since this is already supported on the EDK II side, we add a CFI flash
>>> emulation to kvmtool.
>>> This is backed by a file, specified via the --flash or -F command line
>>> option. Any flash writes done by the guest will immediately be reflected
>>> into this file (kvmtool mmap's the file).
>>>
>>> This implements a CFI flash using the "Intel/Sharp extended command
>>> set", as specified in:
>>> - JEDEC JESD68.01
>>> - JEDEC JEP137B
>>> - Intel Application Note 646
>>> Some gaps in those specs have been filled by looking at real devices and
>>> other implementations (QEMU, Linux kernel driver).
>>>
>>> At the moment this relies on DT to advertise the base address of the
>>> flash memory (mapped into the MMIO address space) and is only enabled
>>> for ARM/ARM64. The emulation itself is architecture agnostic, though.
>>>
>>> This is one missing piece toward a working UEFI boot with kvmtool on
>>> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>>
>>> Signed-off-by: Raphael Gault <raphael.gault@arm.com>
>>> [Andre: rewriting and fixing]
>>> Signed-off-by: Andre Przywra <andre.przywara@arm.com>
>>> ---
>>> Hi,
>>>
>>> an update addressing Will's comments. I added coarse grained locking
>>> to the MMIO handler, to prevent concurrent vCPU accesses from messing up
>>> the internal CFI flash state machine.
>>> I also folded the actual flash array read access into the MMIO handler
>>> and fixed the other small issues.
>>>
>>> Cheers,
>>> Andre
>>>
>>> Makefile | 6 +
>>> arm/include/arm-common/kvm-arch.h | 3 +
>>> builtin-run.c | 2 +
>>> hw/cfi_flash.c | 546 ++++++++++++++++++++++++++++++
>>> include/kvm/kvm-config.h | 1 +
>>> include/kvm/util.h | 5 +
>>> 6 files changed, 563 insertions(+)
>>> create mode 100644 hw/cfi_flash.c
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 3862112c..7ed6fb5e 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm)
>>> CFLAGS += -march=armv7-a
>>>
>>> ARCH_WANT_LIBFDT := y
>>> + ARCH_HAS_FLASH_MEM := y
>>> endif
>>>
>>> # ARM64
>>> @@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64)
>>> ARCH_INCLUDE += -Iarm/aarch64/include
>>>
>>> ARCH_WANT_LIBFDT := y
>>> + ARCH_HAS_FLASH_MEM := y
>>> endif
>>>
>>> ifeq ($(ARCH),mips)
>>> @@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
>>> endif
>>> endif
>>>
>>> +ifeq (y,$(ARCH_HAS_FLASH_MEM))
>>> + OBJS += hw/cfi_flash.o
>>> +endif
>>> +
>>> ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
>>> CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB
>>> LIBS_DYNOPT += -lz
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index b9d486d5..2bb085f4 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -21,6 +21,9 @@
>>> #define ARM_GIC_DIST_SIZE 0x10000
>>> #define ARM_GIC_CPUI_SIZE 0x20000
>>>
>>> +#define ARM_FLASH_MMIO_BASE 0x2000000 /* 32 MB */
>>> +#define KVM_FLASH_MMIO_BASE ARM_FLASH_MMIO_BASE
>>> +
>>> #define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
>>> #define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>> #define ARM_PCI_CFG_SIZE (1ULL << 24)
>>> diff --git a/builtin-run.c b/builtin-run.c
>>> index f8dc6c72..df8c6741 100644
>>> --- a/builtin-run.c
>>> +++ b/builtin-run.c
>>> @@ -138,6 +138,8 @@ void kvm_run_set_wrapper_sandbox(void)
>>> "Kernel command line arguments"), \
>>> OPT_STRING('f', "firmware", &(cfg)->firmware_filename, "firmware",\
>>> "Firmware image to boot in virtual machine"), \
>>> + OPT_STRING('F', "flash", &(cfg)->flash_filename, "flash",\
>>> + "Flash image to present to virtual machine"), \
>>> \
>>> OPT_GROUP("Networking options:"), \
>>> OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params", \
>>> diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
>>> new file mode 100644
>>> index 00000000..d7c0e7e8
>>> --- /dev/null
>>> +++ b/hw/cfi_flash.c
>>> @@ -0,0 +1,546 @@
>>> +#include <stdbool.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/err.h>
>>> +#include <linux/sizes.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "kvm/kvm.h"
>>> +#include "kvm/kvm-arch.h"
>>> +#include "kvm/devices.h"
>>> +#include "kvm/fdt.h"
>>> +#include "kvm/mutex.h"
>>> +#include "kvm/util.h"
>>> +
>>> +/* The EDK2 driver hardcodes two 16-bit chips on a 32-bit bus. */
>>> +#define CFI_NR_FLASH_CHIPS 2
>>> +
>>> +/* We always emulate a 32 bit bus width. */
>>> +#define CFI_BUS_WIDTH 4
>>> +
>>> +/* The *effective* size of an erase block (over all chips) */
>>> +#define FLASH_BLOCK_SIZE SZ_64K
>>> +
>>> +#define PROGRAM_BUFF_SIZE_BITS 7
>>> +#define PROGRAM_BUFF_SIZE (1U << PROGRAM_BUFF_SIZE_BITS)
>>> +
>>> +/* CFI commands */
>>> +#define CFI_CMD_LOCK_BLOCK 0x01
>>> +#define CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP 0x10
>>> +#define CFI_CMD_BLOCK_ERASE_SETUP 0x20
>>> +#define CFI_CMD_WORD_PROGRAM_SETUP 0x40
>>> +#define CFI_CMD_CLEAR_STATUS_REGISTER 0x50
>>> +#define CFI_CMD_LOCK_BLOCK_SETUP 0x60
>>> +#define CFI_CMD_READ_STATUS_REGISTER 0x70
>>> +#define CFI_CMD_READ_JEDEC 0x90
>>> +#define CFI_CMD_READ_CFI_QUERY 0x98
>>> +#define CFI_CMD_BUFFERED_PROGRAM_CONFIRM 0xd0
>>> +#define CFI_CMD_BLOCK_ERASE_CONFIRM 0xd0
>>> +#define CFI_CMD_UNLOCK_BLOCK 0xd0
>>> +#define CFI_CMD_BUFFERED_PROGRAM_SETUP 0xe8
>>> +#define CFI_CMD_READ_ARRAY 0xff
>>> +
>>> +/*
>>> + * CFI query table contents, as far as it is constant.
>>> + */
>>> +#define CFI_GEOM_OFFSET 0x27
>>> +static u8 cfi_query_table[] = {
>>> + /* offset 0x10: CFI query identification string */
>>> + 'Q', 'R', 'Y', /* ID string */
>>> + 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>>> + 0x31, 0x00, /* address of primary extended query table */
>>> + 0x00, 0x00, /* alternative command set: unused */
>>> + 0x00, 0x00, /* address of alternative extended query table*/
>>> + /* offset 0x1b: system interface information */
>>> + 0x45, /* minimum Vcc voltage: 4.5V */
>>> + 0x55, /* maximum Vcc voltage: 5.5V */
>>> + 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>>> + 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>>> + 0x01, /* timeout for single word program: 2 us */
>>> + 0x01, /* timeout for multi-byte program: 2 us */
>>> + 0x01, /* timeout for block erase: 2 ms */
>>> + 0x00, /* timeout for full chip erase: not supported */
>>> + 0x00, /* max timeout for single word program: 1x */
>>> + 0x00, /* max timeout for mulit-byte program: 1x */
>>> + 0x00, /* max timeout for block erase: 1x */
>>> + 0x00, /* max timeout for chip erase: not supported */
>>> + /* offset 0x27: flash geometry information */
>>> + 0x00, /* size in power-of-2 bytes, filled later */
>>> + 0x06, 0x00, /* interface description: 32 and 16 bits */
>> I don't think this is correct. From Intel StrataFlash Embedded Memory (P30)
>> Family, table 34:
>>
>> ""n" such that n+1 specifies the bit field that represents the flash device width
>> capabilities as described in the table".
> Yeah, seems to be correct, but it looks this Intel Strata document is the only place which details this encoding (which looks like being retrofit somehow).
> And I didn't really use this document, because it's a manufacturer data sheet and not a specification.
The device is in the list of specification you provided in the commit message. I
think it would make the reviewers' life a lot easier if you posted all the
documentation that you used, and drop documentation that you didn't. Where did you
get the value 0x06 from? That was the only document from where I could infer what
it means, maybe I didn't dig deep enough.
> I will change it to 0x5, but for the records Linux worked even with 0x6 for me.
I would say that in this case working != correct, because Linux 5.6-rc2 defines
0x05 as a x32/x16 interface, and 0x06 is undefined in the file that I mentioned in
the previous reply. Did you check to see if the Linux driver recognized that
interface type? Maybe it changed between versions. I also tried 0x00,0x00 for the
interface description and Linux also worked.
Either way, I followed the trail of breadcrumbs starting at the comment for the
define and I found this in Common Flash Memory Interface Specification release
2.0, Appendix:
"Note: April 2000 -x16/x32 devices will be represented by hex value 0005h as
requested by Intel in order to make them more software friendly. Changes will be
made to the CFI drivers so that a bit-wise switch is created to represent
different data widths. [..] For example, if we take the description for an x16/x32
device (0005h) and we convert that to binary we get 0101b. If we add one to this
value we get a bit pattern that looks like this: 0110b. This bit-wise switch
indicates that the device a x16/x32 device".
I guess the reason for the inconsistencies is that at some point it used to be
different, but it was changed because Intel requested it.
>> If you want to advertise 32 and 16 bit write capabilities, it should be 5 because
>> 5+1=6. This is also the value that the Linux kernel checks for (see
>> include/linux/mtd/cfi.h, define CFI_INTERFACE_X16_BY_X32_ASYNC"). 6 actually means
>> 32, 16 and 8 bit accesses.
>>
>> This begs another question: why do we support both 16 and 32 bit accesses instead
>> of supporting only 32 bit?
> Because we can, there is no reason to restrict this. I feel like we should be as capable as possible, especially since it's trivial to emulate.
Makes sense.
>
>>> + PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>>> + /* number of multi-byte writes */
>> Shouldn't the comment be maximum number of bytes in the write buffer?
> Yes, possibly.
>
>>> + 0x01, /* one erase block region */
>>> + 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>>> + /* offset 0x31: Intel primary algorithm extended query table */
>>> + 'P', 'R', 'I',
>>> + '1', '0', /* version 1.0 */
>>> + 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>>> + 0x00, /* no functions after suspend */
>>> + 0x01, 0x00, /* only lock bit supported */
>>> + 0x50, /* best Vcc value: 5.0V */
>>> + 0x00, /* best Vpp value: 0.0V (unused) */
>>> + 0x01, /* number of protection register fields */
>>> + 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>>> +};
>> As an aside, I found it impossible to review the cfi_query_table array in its
>> current form. This is how I wrote the array so I could read it. I also took the
>> liberty to remove the offset when indexing the array, making read_cfi less error
>> prone, in my opinion:
> Please don't post elaborate code sequences as a comment, especially not if it gets mangled (Thunderbird is annoyingly bad in this respect).
I use Thunderbird and it showed fine for me, and I have sent large diffs before in
replies and I got no complaints. I use the settings from
Documentation/process/email-clients.rst.
Thanks,
Alex
> I think I would have got what you mean by showing just one line ;-)
>
> Cheers,
> Andre
>
>> diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
>> index d7c0e7e80d69..65a90e288be8 100644
>> --- a/hw/cfi_flash.c
>> +++ b/hw/cfi_flash.c
>> @@ -46,45 +46,43 @@
>> */
>> #define CFI_GEOM_OFFSET 0x27
>> static u8 cfi_query_table[] = {
>> - /* offset 0x10: CFI query identification string */
>> - 'Q', 'R', 'Y', /* ID string */
>> - 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> - 0x31, 0x00, /* address of primary extended query table */
>> - 0x00, 0x00, /* alternative command set: unused */
>> - 0x00, 0x00, /* address of alternative extended query table*/
>> - /* offset 0x1b: system interface information */
>> - 0x45, /* minimum Vcc voltage: 4.5V */
>> - 0x55, /* maximum Vcc voltage: 5.5V */
>> - 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> - 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> - 0x01, /* timeout for single word program: 2 us */
>> - 0x01, /* timeout for multi-byte program: 2 us */
>> - 0x01, /* timeout for block erase: 2 ms */
>> - 0x00, /* timeout for full chip erase: not supported */
>> - 0x00, /* max timeout for single word program: 1x */
>> - 0x00, /* max timeout for mulit-byte program: 1x */
>> - 0x00, /* max timeout for block erase: 1x */
>> - 0x00, /* max timeout for chip erase: not supported */
>> - /* offset 0x27: flash geometry information */
>> - 0x00, /* size in power-of-2 bytes, filled later */
>> - 0x06, 0x00, /* interface description: 32 and 16 bits */
>> - PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> + [0x10] = 'Q', 'R', 'Y', /* ID string */
>> + [0x13] = 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> + [0x15] = 0x31, 0x00, /* address of primary extended query table */
>> + [0x17] = 0x00, 0x00, /* alternative command set: unused */
>> + [0x19] = 0x00, 0x00, /* address of alternative extended query table*/
>> + /* System interface information */
>> + [0x1b] = 0x45, /* minimum Vcc voltage: 4.5V */
>> + [0x1c] = 0x55, /* maximum Vcc voltage: 5.5V */
>> + [0x1d] = 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> + [0x1e] = 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> + [0x1f] = 0x01, /* timeout for single word program: 2 us */
>> + [0x20] = 0x01, /* timeout for multi-byte program: 2 us */
>> + [0x21] = 0x01, /* timeout for block erase: 2 ms */
>> + [0x22] = 0x00, /* timeout for full chip erase: not supported */
>> + [0x23] = 0x00, /* max timeout for single word program: 1x */
>> + [0x24] = 0x00, /* max timeout for mulit-byte program: 1x */
>> + [0x25] = 0x00, /* max timeout for block erase: 1x */
>> + [0x26] = 0x00, /* max timeout for chip erase: not supported */
>> + /* Flash geometry information */
>> + [0x27] = 0x00, /* size in power-of-2 bytes, filled later */
>> + [0x28] = 0x06, 0x00, /* interface description: 32 and 16 bits */
>> + [0x2a] = PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> /* number of multi-byte writes */
>> - 0x01, /* one erase block region */
>> - 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> - /* offset 0x31: Intel primary algorithm extended query table */
>> - 'P', 'R', 'I',
>> - '1', '0', /* version 1.0 */
>> - 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>> - 0x00, /* no functions after suspend */
>> - 0x01, 0x00, /* only lock bit supported */
>> - 0x50, /* best Vcc value: 5.0V */
>> - 0x00, /* best Vpp value: 0.0V (unused) */
>> - 0x01, /* number of protection register fields */
>> - 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>> + [0x2c] = 0x01, /* one erase block region */
>> + [0x2d] = 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> + /* Intel primary algorithm extended query table */
>> + [0x31] = 'P', 'R', 'I', /* ID string */
>> + [0x34] = '1', '0', /* version 1.0 */
>> + [0x36] = 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock &
>> pm-read */
>> + [0x40] = 0x00, /* no functions after suspend */
>> + [0x41] = 0x01, 0x00, /* only lock bit supported */
>> ...skipping...
>> + [0x10] = 'Q', 'R', 'Y', /* ID string */
>> + [0x13] = 0x01, 0x00, /* primary command set: Intel/Sharp extended */
>> + [0x15] = 0x31, 0x00, /* address of primary extended query table */
>> + [0x17] = 0x00, 0x00, /* alternative command set: unused */
>> + [0x19] = 0x00, 0x00, /* address of alternative extended query table*/
>> + /* System interface information */
>> + [0x1b] = 0x45, /* minimum Vcc voltage: 4.5V */
>> + [0x1c] = 0x55, /* maximum Vcc voltage: 5.5V */
>> + [0x1d] = 0x00, /* minimum Vpp voltage: 0.0V (unused) */
>> + [0x1e] = 0x00, /* maximum Vpp voltage: 0.0V *(unused) */
>> + [0x1f] = 0x01, /* timeout for single word program: 2 us */
>> + [0x20] = 0x01, /* timeout for multi-byte program: 2 us */
>> + [0x21] = 0x01, /* timeout for block erase: 2 ms */
>> + [0x22] = 0x00, /* timeout for full chip erase: not supported */
>> + [0x23] = 0x00, /* max timeout for single word program: 1x */
>> + [0x24] = 0x00, /* max timeout for mulit-byte program: 1x */
>> + [0x25] = 0x00, /* max timeout for block erase: 1x */
>> + [0x26] = 0x00, /* max timeout for chip erase: not supported */
>> + /* Flash geometry information */
>> + [0x27] = 0x00, /* size in power-of-2 bytes, filled later */
>> + [0x28] = 0x06, 0x00, /* interface description: 32 and 16 bits */
>> + [0x2a] = PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
>> /* number of multi-byte writes */
>> - 0x01, /* one erase block region */
>> - 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> - /* offset 0x31: Intel primary algorithm extended query table */
>> - 'P', 'R', 'I',
>> - '1', '0', /* version 1.0 */
>> - 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
>> - 0x00, /* no functions after suspend */
>> - 0x01, 0x00, /* only lock bit supported */
>> - 0x50, /* best Vcc value: 5.0V */
>> - 0x00, /* best Vpp value: 0.0V (unused) */
>> - 0x01, /* number of protection register fields */
>> - 0x00, 0x00, 0x00, 0x00, /* protection field 1 description */
>> + [0x2c] = 0x01, /* one erase block region */
>> + [0x2d] = 0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
>> + /* Intel primary algorithm extended query table */
>> + [0x31] = 'P', 'R', 'I', /* ID string */
>> + [0x34] = '1', '0', /* version 1.0 */
>> + [0x36] = 0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock &
>> pm-read */
>> + [0x40] = 0x00, /* no functions after suspend */
>> + [0x41] = 0x01, 0x00, /* only lock bit supported */
>> + [0x43] = 0x50, /* best Vcc value: 5.0V */
>> + [0x43] = 0x00, /* best Vpp value: 0.0V (unused) */
>> + [0x44] = 0x01, /* number of protection register fields */
>> + [0x45] = 0x00, 0x00, 0x00, 0x00,/* protection field 1 description */
>> };
>>
>> -
>> /*
>> * Those states represent a subset of the CFI flash state machine.
>> */
>> @@ -141,10 +139,7 @@ static int nr_erase_blocks(struct cfi_flash_device *sfdev)
>> */
>> static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>> {
>> - if (addr < 0x10) /* CFI information starts at 0x10 */
>> - return 0;
>> -
>> - if (addr - 0x10 > sizeof(cfi_query_table)) {
>> + if (addr > sizeof(cfi_query_table)) {
>> pr_debug("CFI query read access beyond the end of table");
>> return 0;
>> }
>> @@ -163,7 +158,7 @@ static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>> return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) >> 8;
>> }
>>
>> - return cfi_query_table[addr - 0x10];
>> + return cfi_query_table[addr];
>> }
>>
>> static bool block_is_locked(struct cfi_flash_device *sfdev, u64 addr)
>>
>> Thanks,
>> Alex
>>> +
>>> +
>>> +/*
>>> + * Those states represent a subset of the CFI flash state machine.
>>> + */
>>> +enum cfi_flash_state {
>>> + READY,
>>> + LOCK_SETUP,
>>> + WP_SETUP,
>>> + BP_SETUP,
>>> + BP_LOAD,
>>> + ERASE_SETUP,
>>> +};
>>> +
>>> +/*
>>> + * The device can be in several **Read** modes.
>>> + * We don't implement the asynchronous burst mode.
>>> + */
>>> +enum cfi_read_mode {
>>> + READ_ARRAY,
>>> + READ_STATUS,
>>> + READ_DEVICE_ID,
>>> + READ_QUERY,
>>> +};
>>> +
>>> +struct cfi_flash_device {
>>> + struct device_header dev_hdr;
>>> + /* Protects the CFI state machine variables in this data structure. */
>>> + struct mutex mutex;
>>> + u64 base_addr;
>>> + u32 size;
>>> +
>>> + void *flash_memory;
>>> + u8 program_buffer[PROGRAM_BUFF_SIZE * 4];
>>> + unsigned long *lock_bm;
>>> + u64 last_address;
>>> + unsigned int buff_written;
>>> + unsigned int program_length;
>>> +
>>> + enum cfi_flash_state state;
>>> + enum cfi_read_mode read_mode;
>>> + u16 rcr;
>>> + u8 sr;
>>> +};
>>> +
>>> +static int nr_erase_blocks(struct cfi_flash_device *sfdev)
>>> +{
>>> + return sfdev->size / FLASH_BLOCK_SIZE;
>>> +}
>>> +
>>> +/*
>>> + * CFI queries always deal with one byte of information, possibly mirrored
>>> + * to other bytes on the bus. This is dealt with in the callers.
>>> + * The address provided is the one for 8-bit addressing, and would need to
>>> + * be adjusted for wider accesses.
>>> + */
>>> +static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + if (addr < 0x10) /* CFI information starts at 0x10 */
>>> + return 0;
>>> +
>>> + if (addr - 0x10 > sizeof(cfi_query_table)) {
>>> + pr_debug("CFI query read access beyond the end of table");
>>> + return 0;
>>> + }
>>> +
>>> + /* Fixup dynamic information in the geometry part of the table. */
>>> + switch (addr) {
>>> + case CFI_GEOM_OFFSET: /* device size in bytes, power of two */
>>> + return pow2_size(sfdev->size / CFI_NR_FLASH_CHIPS);
>>> + case CFI_GEOM_OFFSET + 6: /* number of erase blocks, minus one */
>>> + return (nr_erase_blocks(sfdev) - 1) & 0xff;
>>> + case CFI_GEOM_OFFSET + 7:
>>> + return (nr_erase_blocks(sfdev) - 1) >> 8;
>>> + case CFI_GEOM_OFFSET + 8: /* erase block size, in units of 256 */
>>> + return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) & 0xff;
>>> + case CFI_GEOM_OFFSET + 9:
>>> + return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) >> 8;
>>> + }
>>> +
>>> + return cfi_query_table[addr - 0x10];
>>> +}
>>> +
>>> +static bool block_is_locked(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + int block_nr = addr / FLASH_BLOCK_SIZE;
>>> +
>>> + return test_bit(block_nr, sfdev->lock_bm);
>>> +}
>>> +
>>> +#define DEV_ID_MASK 0x7ff
>>> +static u16 read_dev_id(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + switch ((addr & DEV_ID_MASK) / CFI_BUS_WIDTH) {
>>> + case 0x0: /* vendor ID */
>>> + return 0x0000;
>>> + case 0x1: /* device ID */
>>> + return 0xffff;
>>> + case 0x2:
>>> + return block_is_locked(sfdev, addr & ~DEV_ID_MASK);
>>> + case 0x5:
>>> + return sfdev->rcr;
>>> + default: /* Ignore the other entries. */
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static void lock_block(struct cfi_flash_device *sfdev, u64 addr, bool lock)
>>> +{
>>> + int block_nr = addr / FLASH_BLOCK_SIZE;
>>> +
>>> + if (lock)
>>> + set_bit(block_nr, sfdev->lock_bm);
>>> + else
>>> + clear_bit(block_nr, sfdev->lock_bm);
>>> +}
>>> +
>>> +static void word_program(struct cfi_flash_device *sfdev,
>>> + u64 addr, void *data, int len)
>>> +{
>>> + if (block_is_locked(sfdev, addr)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> +
>>> + memcpy(sfdev->flash_memory + addr, data, len);
>>> +}
>>> +
>>> +/* Reset the program buffer state to prepare for follow-up writes. */
>>> +static void buffer_setup(struct cfi_flash_device *sfdev)
>>> +{
>>> + memset(sfdev->program_buffer, 0, sizeof(sfdev->program_buffer));
>>> + sfdev->last_address = ~0ULL;
>>> + sfdev->buff_written = 0;
>>> +}
>>> +
>>> +static bool buffer_program(struct cfi_flash_device *sfdev,
>>> + u64 addr, void *buffer, int len)
>>> +{
>>> + unsigned int buf_addr;
>>> +
>>> + if (sfdev->buff_written >= sfdev->program_length)
>>> + return false;
>>> +
>>> + /*
>>> + * The first word written into the buffer after the setup command
>>> + * happens to be the base address for the buffer.
>>> + * All subsequent writes need to be within this address and this
>>> + * address plus the buffer size, so keep this value around.
>>> + */
>>> + if (sfdev->last_address == ~0ULL)
>>> + sfdev->last_address = addr;
>>> +
>>> + if (addr < sfdev->last_address)
>>> + return false;
>>> + buf_addr = addr - sfdev->last_address;
>>> + if (buf_addr >= PROGRAM_BUFF_SIZE)
>>> + return false;
>>> +
>>> + memcpy(sfdev->program_buffer + buf_addr, buffer, len);
>>> + sfdev->buff_written++;
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static void buffer_confirm(struct cfi_flash_device *sfdev)
>>> +{
>>> + if (block_is_locked(sfdev, sfdev->last_address)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> + memcpy(sfdev->flash_memory + sfdev->last_address,
>>> + sfdev->program_buffer,
>>> + sfdev->buff_written * sizeof(u32));
>>> +}
>>> +
>>> +static void block_erase_confirm(struct cfi_flash_device *sfdev, u64 addr)
>>> +{
>>> + if (block_is_locked(sfdev, addr)) {
>>> + sfdev->sr |= 0x12;
>>> + return;
>>> + }
>>> +
>>> + memset(sfdev->flash_memory + addr, 0xFF, FLASH_BLOCK_SIZE);
>>> +}
>>> +
>>> +static void cfi_flash_mmio(struct kvm_cpu *vcpu,
>>> + u64 addr, u8 *data, u32 len, u8 is_write,
>>> + void *context)
>>> +{
>>> + struct cfi_flash_device *sfdev = context;
>>> + u64 faddr = addr - sfdev->base_addr;
>>> + u32 value;
>>> +
>>> + if (!is_write) {
>>> + u16 cfi_value = 0;
>>> +
>>> + mutex_lock(&sfdev->mutex);
>>> +
>>> + switch (sfdev->read_mode) {
>>> + case READ_ARRAY:
>>> + /* just copy the requested bytes from the array */
>>> + memcpy(data, sfdev->flash_memory + faddr, len);
>>> + goto out_unlock;
>>> + case READ_STATUS:
>>> + cfi_value = sfdev->sr;
>>> + break;
>>> + case READ_DEVICE_ID:
>>> + cfi_value = read_dev_id(sfdev, faddr);
>>> + break;
>>> + case READ_QUERY:
>>> + cfi_value = read_cfi(sfdev, faddr / CFI_BUS_WIDTH);
>>> + break;
>>> + }
>>> + switch (len) {
>>> + case 1:
>>> + *data = cfi_value;
>>> + break;
>>> + case 8: memset(data + 4, 0, 4);
>>> + /* fall-through */
>>> + case 4:
>>> + if (CFI_NR_FLASH_CHIPS == 2)
>>> + memcpy(data + 2, &cfi_value, 2);
>>> + else
>>> + memset(data + 2, 0, 2);
>>> + /* fall-through */
>>> + case 2:
>>> + memcpy(data, &cfi_value, 2);
>>> + break;
>>> + default:
>>> + pr_debug("CFI flash: illegal access length %d for read mode %d",
>>> + len, sfdev->read_mode);
>>> + break;
>>> + }
>>> +
>>> + goto out_unlock;
>>> + }
>>> +
>>> + if (len > 4) {
>>> + pr_info("CFI flash: MMIO %d-bit write access not supported",
>>> + len * 8);
>>> + return;
>>> + }
>>> +
>>> + memcpy(&value, data, len);
>>> +
>>> + mutex_lock(&sfdev->mutex);
>>> +
>>> + switch (sfdev->state) {
>>> + case READY: /* handled below */
>>> + break;
>>> +
>>> + case LOCK_SETUP:
>>> + switch (value & 0xff) {
>>> + case CFI_CMD_LOCK_BLOCK:
>>> + lock_block(sfdev, faddr, true);
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_UNLOCK_BLOCK:
>>> + lock_block(sfdev, faddr, false);
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + default:
>>> + sfdev->sr |= 0x30;
>>> + break;
>>> + }
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case WP_SETUP:
>>> + word_program(sfdev, faddr, data, len);
>>> + sfdev->read_mode = READ_STATUS;
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case BP_LOAD:
>>> + if (buffer_program(sfdev, faddr, data, len))
>>> + goto out_unlock;
>>> +
>>> + if ((value & 0xFF) == CFI_CMD_BUFFERED_PROGRAM_CONFIRM) {
>>> + buffer_confirm(sfdev);
>>> + sfdev->read_mode = READ_STATUS;
>>> + } else {
>>> + pr_debug("CFI flash: BP_LOAD: expected CONFIRM(0xd0), got 0x%x @ 0x%llx",
>>> + value, faddr);
>>> + sfdev->sr |= 0x10;
>>> + }
>>> + sfdev->state = READY;
>>> + goto out_unlock;
>>> +
>>> + case BP_SETUP:
>>> + sfdev->program_length = (value & 0xffff) + 1;
>>> + if (sfdev->program_length > PROGRAM_BUFF_SIZE / 4)
>>> + sfdev->program_length = PROGRAM_BUFF_SIZE / 4;
>>> + sfdev->state = BP_LOAD;
>>> + sfdev->read_mode = READ_STATUS;
>>> + goto out_unlock;
>>> +
>>> + case ERASE_SETUP:
>>> + if ((value & 0xff) == CFI_CMD_BLOCK_ERASE_CONFIRM)
>>> + block_erase_confirm(sfdev, faddr);
>>> + else
>>> + sfdev->sr |= 0x30;
>>> +
>>> + sfdev->state = READY;
>>> + sfdev->read_mode = READ_STATUS;
>>> + goto out_unlock;
>>> + }
>>> +
>>> + /* write commands in READY state */
>>> + switch (value & 0xFF) {
>>> + case CFI_CMD_READ_JEDEC:
>>> + sfdev->read_mode = READ_DEVICE_ID;
>>> + break;
>>> + case CFI_CMD_READ_STATUS_REGISTER:
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_READ_CFI_QUERY:
>>> + sfdev->read_mode = READ_QUERY;
>>> + break;
>>> + case CFI_CMD_CLEAR_STATUS_REGISTER:
>>> + sfdev->sr = 0x80;
>>> + break;
>>> + case CFI_CMD_WORD_PROGRAM_SETUP:
>>> + case CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP:
>>> + sfdev->state = WP_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_LOCK_BLOCK_SETUP:
>>> + sfdev->state = LOCK_SETUP;
>>> + break;
>>> + case CFI_CMD_BLOCK_ERASE_SETUP:
>>> + sfdev->state = ERASE_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_BUFFERED_PROGRAM_SETUP:
>>> + buffer_setup(sfdev);
>>> + sfdev->state = BP_SETUP;
>>> + sfdev->read_mode = READ_STATUS;
>>> + break;
>>> + case CFI_CMD_BUFFERED_PROGRAM_CONFIRM:
>>> + pr_debug("CFI flash: unexpected confirm command 0xD0");
>>> + break;
>>> + default:
>>> + pr_debug("CFI flash: unknown command 0x%x", value);
>>> + /* fall through */
>>> + case CFI_CMD_READ_ARRAY:
>>> + sfdev->read_mode = READ_ARRAY;
>>> + break;
>>> + }
>>> +
>>> +out_unlock:
>>> + mutex_unlock(&sfdev->mutex);
>>> +}
>>> +
>>> +#ifdef CONFIG_HAS_LIBFDT
>>> +static void generate_cfi_flash_fdt_node(void *fdt,
>>> + struct device_header *dev_hdr,
>>> + void (*generate_irq_prop)(void *fdt,
>>> + u8 irq,
>>> + enum irq_type))
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> + u64 reg_prop[2];
>>> +
>>> + sfdev = container_of(dev_hdr, struct cfi_flash_device, dev_hdr);
>>> + reg_prop[0] = cpu_to_fdt64(sfdev->base_addr);
>>> + reg_prop[1] = cpu_to_fdt64(sfdev->size);
>>> +
>>> + _FDT(fdt_begin_node(fdt, "flash"));
>>> + _FDT(fdt_property_cell(fdt, "bank-width", CFI_BUS_WIDTH));
>>> + _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
>>> + _FDT(fdt_property_cell(fdt, "#size-cells", 0x1));
>>> + _FDT(fdt_property_string(fdt, "compatible", "cfi-flash"));
>>> + _FDT(fdt_property_string(fdt, "label", "System-firmware"));
>>> + _FDT(fdt_property(fdt, "reg", ®_prop, sizeof(reg_prop)));
>>> + _FDT(fdt_end_node(fdt));
>>> +}
>>> +#else
>>> +#define generate_cfi_flash_fdt_node NULL
>>> +#endif
>>> +
>>> +static struct cfi_flash_device *create_flash_device_file(struct kvm *kvm,
>>> + const char *filename)
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> + struct stat statbuf;
>>> + unsigned int value;
>>> + int ret;
>>> + int fd;
>>> +
>>> + fd = open(filename, O_RDWR);
>>> + if (fd < 0)
>>> + return ERR_PTR(-errno);
>>> + if (fstat(fd, &statbuf) < 0) {
>>> + close(fd);
>>> + return ERR_PTR(-errno);
>>> + }
>>> +
>>> + sfdev = malloc(sizeof(struct cfi_flash_device));
>>> + if (!sfdev) {
>>> + close(fd);
>>> + return ERR_PTR(-ENOMEM);
>>> + }
>>> +
>>> + sfdev->size = (statbuf.st_size + 4095) & ~0xfffUL;
>>> + sfdev->flash_memory = mmap(NULL, statbuf.st_size,
>>> + PROT_READ | PROT_WRITE, MAP_SHARED,
>>> + fd, 0);
>>> + if (sfdev->flash_memory == MAP_FAILED) {
>>> + close(fd);
>>> + free(sfdev);
>>> + return ERR_PTR(-errno);
>>> + }
>>> + sfdev->base_addr = KVM_FLASH_MMIO_BASE;
>>> + sfdev->state = READY;
>>> + sfdev->read_mode = READ_ARRAY;
>>> + sfdev->sr = 0x80;
>>> + sfdev->rcr = 0xbfcf;
>>> +
>>> + value = roundup(nr_erase_blocks(sfdev), BITS_PER_LONG) / 8;
>>> + sfdev->lock_bm = malloc(value);
>>> + memset(sfdev->lock_bm, 0, value);
>>> +
>>> + sfdev->dev_hdr.bus_type = DEVICE_BUS_MMIO;
>>> + sfdev->dev_hdr.data = generate_cfi_flash_fdt_node;
>>> + mutex_init(&sfdev->mutex);
>>> + ret = device__register(&sfdev->dev_hdr);
>>> + if (ret) {
>>> + free(sfdev->flash_memory);
>>> + free(sfdev);
>>> + return ERR_PTR(ret);
>>> + }
>>> +
>>> + ret = kvm__register_mmio(kvm,
>>> + sfdev->base_addr, sfdev->size,
>>> + false, cfi_flash_mmio, sfdev);
>>> + if (ret) {
>>> + device__unregister(&sfdev->dev_hdr);
>>> + free(sfdev->flash_memory);
>>> + free(sfdev);
>>> + return ERR_PTR(ret);
>>> + }
>>> +
>>> + return sfdev;
>>> +}
>>> +
>>> +static int flash__init(struct kvm *kvm)
>>> +{
>>> + struct cfi_flash_device *sfdev;
>>> +
>>> + if (!kvm->cfg.flash_filename)
>>> + return 0;
>>> +
>>> + sfdev = create_flash_device_file(kvm, kvm->cfg.flash_filename);
>>> + if (IS_ERR(sfdev))
>>> + return PTR_ERR(sfdev);
>>> +
>>> + return 0;
>>> +}
>>> +dev_init(flash__init);
>>> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
>>> index a052b0bc..f4a8b831 100644
>>> --- a/include/kvm/kvm-config.h
>>> +++ b/include/kvm/kvm-config.h
>>> @@ -35,6 +35,7 @@ struct kvm_config {
>>> const char *vmlinux_filename;
>>> const char *initrd_filename;
>>> const char *firmware_filename;
>>> + const char *flash_filename;
>>> const char *console;
>>> const char *dev;
>>> const char *network;
>>> diff --git a/include/kvm/util.h b/include/kvm/util.h
>>> index 4ca7aa93..5c37f0b7 100644
>>> --- a/include/kvm/util.h
>>> +++ b/include/kvm/util.h
>>> @@ -104,6 +104,11 @@ static inline unsigned long roundup_pow_of_two(unsigned long x)
>>> return x ? 1UL << fls_long(x - 1) : 0;
>>> }
>>>
>>> +static inline int pow2_size(unsigned long x)
>>> +{
>>> + return (sizeof(x) * 8) - __builtin_clzl(x - 1);
>>> +}
>>> +
>>> struct kvm;
>>> void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size);
>>> void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 size);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Hard Disk consumes lots of power in s2idle
From: Kai-Heng Feng @ 2020-02-20 10:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Srinivas Pandruvada, Linux PM,
open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers),
open list, Kent Lin, Tejun Heo
In-Reply-To: <CAJZ5v0jXvo0ceNMp=kstTi24Ne7F-ZGMcD0T0TSMpcZZWsJsUA@mail.gmail.com>
> On Feb 20, 2020, at 18:12, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 20, 2020 at 9:08 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>>
>> Hi Srinivas,
>>
>>> On Feb 20, 2020, at 02:36, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
>>>
>>> Hi Kai,
>>>
>>> On Wed, 2020-02-19 at 22:22 +0800, Kai-Heng Feng wrote:
>>>> Hi Srinivas,
>>>>
>>>> Your previous work to support DEVSLP works well on SATA SSDs, so I am
>>>> asking you the issue I am facing:
>>>> Once a laptop has a HDD installed, the power consumption during
>>>> S2Idle increases ~0.4W, which is quite a lot.
>>>> However, HDDs don't seem to support DEVSLP, so I wonder if you know
>>>> to do proper power management for HDDs?
>>> What is the default here
>>> cat /sys/power/mem_sleep
>>> s2idle or deep?
>>
>> It defaults to s2idle.
>>
>>>
>>> Please follow debug steps here:
>>> https://01.org/blogs/qwang59/2018/how-achieve-s0ix-states-linux
>>>
>>> We need to check whether you get any PC10 residency or not.
>>
>> Yes it reaches PC10. It doesn't reach SLP_S0 though.
>> The real number on S2Idle power consumption:
>> No HDD: ~1.4W
>> One HDD: ~1.8W
>>
>> If the SoC doesn't hit PC10 the number should be significantly higher.
>> That's why I think the issue is the power management on HDD itself.
>
> I'm assuming that you mean a non-SSD device here.
Yes, it's spinning rust here.
>
> That would be handled via ata_port_suspend() I gather and whatever
> that does should do the right thing.
>
> Do you think that the disk doesn't spin down or it spins down, but the
> logic stays on?
The spin sound is audible, so I am certain the HDD spins down during S2Idle.
How do I know if the logic is on or off?
Kai-Heng
^ permalink raw reply
* [Ocfs2-devel] [QUESTION] An NULL pointer dereference problem which was caused when dereference journal head
From: wangyan @ 2020-02-20 10:25 UTC (permalink / raw)
To: ocfs2-devel
In-Reply-To: <20200220100610.GC13232@quack2.suse.cz>
Thanks a lot!
I will send a patch to help you fix this later.
Thanks,
Yan Wang
On 2020/2/20 18:06, Jan Kara wrote:
> On Thu 20-02-20 16:48:56, wangyan wrote:
>> Hi all,
>>
>> I met an null pointer dereference problem, the running environment:
>> kernel version: 4.19
>> A cluster with two nodes, 5 luns mounted on two nodes, and do some
>> file operations like dd/fallocate/truncate/rm on every lun with storage
>> network disconnection.
>>
>> The fallocate operation on dm-23-45 caused an null pointer dereference.
>>
>> The information of NULL pointer dereference as follows:
>> [577992.878282] JBD2: Error -5 detected when updating journal superblock for dm-23-45.
>> [577992.878290] Aborting journal on device dm-23-45.
>> ...
>> [577992.890778] JBD2: Error -5 detected when updating journal superblock for dm-24-46.
>> [577992.890908] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890916] (fallocate,88392,52):ocfs2_extend_trans:474 ERROR: status = -30
>> [577992.890918] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890920] (fallocate,88392,52):ocfs2_rotate_tree_right:2500 ERROR: status = -30
>> [577992.890922] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890924] (fallocate,88392,52):ocfs2_do_insert_extent:4382 ERROR: status = -30
>> [577992.890928] (fallocate,88392,52):ocfs2_insert_extent:4842 ERROR: status = -30
>> [577992.890928] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890930] (fallocate,88392,52):ocfs2_add_clusters_in_btree:4947 ERROR: status = -30
>> [577992.890933] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890939] __journal_remove_journal_head: freeing b_committed_data
>> [577992.890949] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
>> [577992.890950] Mem abort info:
>> [577992.890951] ESR = 0x96000004
>> [577992.890952] Exception class = DABT (current EL), IL = 32 bits
>> [577992.890952] SET = 0, FnV = 0
>> [577992.890953] EA = 0, S1PTW = 0
>> [577992.890954] Data abort info:
>> [577992.890955] ISV = 0, ISS = 0x00000004
>> [577992.890956] CM = 0, WnR = 0
>> [577992.890958] user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000f8da07a9
>> [577992.890960] [0000000000000020] pgd=0000000000000000
>> [577992.890964] Internal error: Oops: 96000004 [#1] SMP
>> [577992.890965] Process fallocate (pid: 88392, stack limit = 0x00000000013db2fd)
>> [577992.890968] CPU: 52 PID: 88392 Comm: fallocate Kdump: loaded Tainted: G W OE 4.19.36 #1
>> [577992.890969] Hardware name: Huawei TaiShan 2280 V2/BC82AMDD, BIOS 0.98 08/25/2019
>> [577992.890971] pstate: 60400009 (nZCv daif +PAN -UAO)
>> [577992.891054] pc : _ocfs2_free_suballoc_bits+0x63c/0x968 [ocfs2]
>> [577992.891082] lr : _ocfs2_free_suballoc_bits+0x618/0x968 [ocfs2]
>> [577992.891084] sp : ffff0000c8e2b810
>> [577992.891085] x29: ffff0000c8e2b820 x28: 0000000000000000
>> [577992.891087] x27: 00000000000006f3 x26: ffffa07957b02e70
>> [577992.891089] x25: ffff807c59d50000 x24: 00000000000006f2
>> [577992.891091] x23: 0000000000000001 x22: ffff807bd39abc30
>> [577992.891093] x21: ffff0000811d9000 x20: ffffa07535d6a000
>> [577992.891097] x19: ffff000001681638 x18: ffffffffffffffff
>> [577992.891098] x17: 0000000000000000 x16: ffff000080a03df0
>> [577992.891100] x15: ffff0000811d9708 x14: 203d207375746174
>> [577992.891101] x13: 73203a524f525245 x12: 20373439343a6565
>> [577992.891103] x11: 0000000000000038 x10: 0101010101010101
>> [577992.891106] x9 : ffffa07c68a85d70 x8 : 7f7f7f7f7f7f7f7f
>> [577992.891109] x7 : 0000000000000000 x6 : 0000000000000080
>> [577992.891110] x5 : 0000000000000000 x4 : 0000000000000002
>> [577992.891112] x3 : ffff000001713390 x2 : 2ff90f88b1c22f00
>> [577992.891114] x1 : ffff807bd39abc30 x0 : 0000000000000000
>> [577992.891116] Call trace:
>> [577992.891139] _ocfs2_free_suballoc_bits+0x63c/0x968 [ocfs2]
>> [577992.891162] _ocfs2_free_clusters+0x100/0x290 [ocfs2]
>> [577992.891185] ocfs2_free_clusters+0x50/0x68 [ocfs2]
>> [577992.891206] ocfs2_add_clusters_in_btree+0x198/0x5e0 [ocfs2]
>> [577992.891227] ocfs2_add_inode_data+0x94/0xc8 [ocfs2]
>> [577992.891248] ocfs2_extend_allocation+0x1bc/0x7a8 [ocfs2]
>> [577992.891269] ocfs2_allocate_extents+0x14c/0x338 [ocfs2]
>> [577992.891290] __ocfs2_change_file_space+0x3f8/0x610 [ocfs2]
>> [577992.891309] ocfs2_fallocate+0xe4/0x128 [ocfs2]
>> [577992.891316] vfs_fallocate+0x11c/0x250
>> [577992.891317] ksys_fallocate+0x54/0x88
>> [577992.891319] __arm64_sys_fallocate+0x28/0x38
>> [577992.891323] el0_svc_common+0x78/0x130
>> [577992.891325] el0_svc_handler+0x38/0x78
>> [577992.891327] el0_svc+0x8/0xc
>>
>> My analysis process as follows:
>> ocfs2_fallocate
>> __ocfs2_change_file_space
>> ocfs2_allocate_extents
>> ocfs2_extend_allocation
>> ocfs2_add_inode_data
>> ocfs2_add_clusters_in_btree
>> ocfs2_insert_extent
>> ocfs2_do_insert_extent
>> ocfs2_rotate_tree_right
>> ocfs2_extend_rotate_transaction
>> ocfs2_extend_trans
>> jbd2_journal_restart
>> jbd2__journal_restart
>> /* handle->h_transaction is NULL,
>> * is_handle_aborted(handle) is true
>> */
>> handle->h_transaction = NULL;
>> start_this_handle
>> return -EROFS;
>> ocfs2_free_clusters
>> _ocfs2_free_clusters
>> _ocfs2_free_suballoc_bits
>> ocfs2_block_group_clear_bits
>> ocfs2_journal_access_gd
>> __ocfs2_journal_access
>> jbd2_journal_get_undo_access
>> /* I think jbd2_write_access_granted() will
>> * return true, because do_get_write_access()
>> * will return -EROFS.
>> */
>> if (jbd2_write_access_granted(...)) return 0;
>> do_get_write_access
>> /* handle->h_transaction is NULL, it will
>> * return -EROFS here, so do_get_write_access()
>> * was not called.
>> */
>> if (is_handle_aborted(handle)) return -EROFS;
>> /* bh2jh(group_bh) is NULL, caused NULL
>> pointer dereference */
>> undo_bg = (struct ocfs2_group_desc *)
>> bh2jh(group_bh)->b_committed_data;
>>
>> I found the log "__journal_remove_journal_head: freeing b_committed_data"
>> before causing NULL pointer dereference which will be printed when remove
>> journal head.
>>
>> So, I think bh2jh(group_bh) is removed after ocfs2_journal_access_gd() and
>> before call "bh2jh(group_bh)->b_committed_data", I don't know if this
>> condition will happen and if the analysis result is right?
>
> Thanks for the analysis! Yes, what you write seems to be possible because
> if handle->h_transaction == NULL, then jbd2_write_access_granted() does not
> really guarantee that journal_head will stay around, not even speaking of
> its b_committed_data.
>
> I think a proper fix is to move is_handle_aborted() check from
> do_get_write_access() into jbd2_journal_get_undo_access() and
> jbd2_journal_get_write_access() before the call to
> jbd2_write_access_granted().
>
> Will you send a patch to fix this or should I do it?
>
> Honza
>
^ permalink raw reply
* Re: [PATCH 3/3] drivers: leds: add support for apa102c leds
From: Nicolas Belin @ 2020-02-20 10:25 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-kernel, linux-leds, pavel, dmurphy
In-Reply-To: <00d63872-0856-602a-e24b-4e27300d9254@gmail.com>
Hi Jacek,
Thanks for you feedback.
I am going to use multicolor framework as Dan mentioned, and fix the
issues you pointed out.
Regards,
Nicolas
Le mar. 18 févr. 2020 à 22:13, Jacek Anaszewski
<jacek.anaszewski@gmail.com> a écrit :
>
> Hi Nicolas,
>
> On 2/18/20 10:37 AM, Nicolas Belin wrote:
> > Initilial commit in order to support the apa102c RGB leds.
> >
> > Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> > ---
> > drivers/leds/Kconfig | 11 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-apa102c.c | 268 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 280 insertions(+)
> > create mode 100644 drivers/leds/leds-apa102c.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index d82f1dea3711..4fafeaaf6ee8 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -69,6 +69,17 @@ config LEDS_AN30259A
> > To compile this driver as a module, choose M here: the module
> > will be called leds-an30259a.
> >
> > +config LEDS_APA102C
> > + tristate "LED Support for Shiji APA102C"
> > + depends on LEDS_CLASS
> > + depends on SPI
> > + help
> > + This option enables support for the Shiji Lighthing APA102C RGB full color
> > + LEDs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called leds-apa102c.
> > +
> > config LEDS_APU
> > tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
> > depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index d7e1107753fb..ab17f90347cb 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
> > # LED Platform Drivers
> > obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o
> > obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o
> > +obj-$(CONFIG_LEDS_APA102C) += leds-apa102c.o
> > obj-$(CONFIG_LEDS_APU) += leds-apu.o
> > obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
> > obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> > diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
> > new file mode 100644
> > index 000000000000..e7abe3f5b7c2
> > --- /dev/null
> > +++ b/drivers/leds/leds-apa102c.c
> > @@ -0,0 +1,268 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (C) 2020 BayLibre, SAS
> > + * Author: Nicolas Belin <nbelin@baylibre.com>
> > + */
>
> Please use "//" comment style for all the above lines.
>
> > +
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +#include <uapi/linux/uleds.h>
> > +
> > +/*
> > + * APA102C SPI protocol description:
> > + * +------+----------------------------------------+------+
> > + * |START | DATA FIELD: | END |
> > + * |FRAME | N LED FRAMES |FRAME |
> > + * +------+------+------+------+------+-----+------+------+
> > + * | 0*32 | LED1 | LED2 | LED3 | LED4 | --- | LEDN | 1*32 |
> > + * +------+------+------+------+------+-----+------+------+
> > + *
> > + * +-----------------------------------+
> > + * |START FRAME 32bits |
> > + * +--------+--------+--------+--------+
> > + * |00000000|00000000|00000000|00000000|
> > + * +--------+--------+--------+--------+
> > + *
> > + * +------------------------------------+
> > + * |LED FRAME 32bits |
> > + * +---+-----+--------+--------+--------+
> > + * |111|LUMA | BLUE | GREEN | RED |
> > + * +---+-----+--------+--------+--------+
> > + * |3b |5bits| 8bits | 8bits | 8bits |
> > + * +---+-----+--------+--------+--------+
> > + * |MSB LSB|MSB LSB|MSB LSB|MSB LSB|
> > + * +---+-----+--------+--------+--------+
> > + *
> > + * +-----------------------------------+
> > + * |END FRAME 32bits |
> > + * +--------+--------+--------+--------+
> > + * |11111111|11111111|11111111|11111111|
> > + * +--------+--------+--------+--------+
> > + */
> > +
> > +/* apa102c default settings */
> > +#define CR_MAX_BRIGHTNESS GENMASK(7, 0)
> > +#define LM_MAX_BRIGHTNESS GENMASK(4, 0)
> > +#define CH_NUM 4
> > +#define START_BYTE 0
> > +#define END_BYTE GENMASK(7, 0)
> > +#define LED_FRAME_HEADER GENMASK(7, 5)
> > +
> > +enum led_channels {
> > + RED,
> > + GREEN,
> > + BLUE,
> > + LUMA,
> > +};
> > +
> > +struct apa102c_led {
> > + char name[LED_MAX_NAME_SIZE];
> > + struct apa102c *priv;
> > + struct led_classdev ldev;
> > + u8 brightness;
>
> Please drop this one, struct led_classdev already holds brightness
> value.
>
> > +};
> > +
> > +struct apa102c {
> > + size_t led_count;
> > + struct device *dev;
> > + struct mutex lock;
> > + struct spi_device *spi;
> > + u8 *buf;
> > + struct apa102c_led leds[];
> > +};
> > +
> > +static int apa102c_sync(struct apa102c *priv)
> > +{
> > + int ret;
> > + size_t i;
> > + size_t bytes = 0;
> > +
> > + for (i = 0; i < 4; i++)
> > + priv->buf[bytes++] = START_BYTE;
> > +
> > + for (i = 0; i < priv->led_count; i++) {
> > + priv->buf[bytes++] = LED_FRAME_HEADER |
> > + priv->leds[i * CH_NUM + LUMA].brightness;
> > + priv->buf[bytes++] = priv->leds[i * CH_NUM + BLUE].brightness;
> > + priv->buf[bytes++] = priv->leds[i * CH_NUM + GREEN].brightness;
> > + priv->buf[bytes++] = priv->leds[i * CH_NUM + RED].brightness;
>
> This is odd. You create separate LED class device for each color anyway,
> so this seems pointless. We have pending LED multi color framework patch
> set, as Dan mentioned, so you could try to use it. If you want to have
> the patch set accepted quicker then just set brightness for one LED at
> a time. You will be able to add LED multicolor class support later when
> it will be ready.
>
> > + }
> > +
> > + for (i = 0; i < 4; i++)
> > + priv->buf[bytes++] = END_BYTE;
> > +
> > + ret = spi_write(priv->spi, priv->buf, bytes);
> > +
> > + return ret;
> > +}
> > +
> > +static int apa102c_set_sync(struct led_classdev *ldev,
> > + enum led_brightness brightness)
> > +{
> > + int ret;
> > + struct apa102c_led *led = container_of(ldev,
> > + struct apa102c_led,
> > + ldev);
> > +
> > + dev_dbg(led->priv->dev, "Set brightness of %s to %d\n",
> > + led->name, brightness);
> > +
> > + mutex_lock(&led->priv->lock);
> > + led->brightness = (u8)brightness;
> > + ret = apa102c_sync(led->priv);
> > + mutex_unlock(&led->priv->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int apa102c_probe_dt(struct apa102c *priv)
> > +{
> > + u32 i = 0;
> > + int j = 0;
> > + struct apa102c_led *led;
> > + struct fwnode_handle *child;
> > + struct device_node *np;
> > + int ret;
> > + int use_index;
> > + const char *str;
> > + static const char * const rgb_name[] = {"red",
> > + "green",
> > + "blue",
> > + "luma"};
>
> We have LED_COLOR_ID* definitions in dt-bindings/leds/common.h
> for red, green and blue. And regarding "luma" - what is specificity
> of that one? If neither of existing definitions fits for it then
> you are welcome to submit a patch adding LED_COLOR_ID_LUMA.
>
> > +
> > + device_for_each_child_node(priv->dev, child) {
> > + np = to_of_node(child);
> > +
> > + ret = fwnode_property_read_u32(child, "reg", &i);
> > + if (ret)
> > + return ret;
> > +
> > + if (i >= priv->led_count)
> > + return -EINVAL;
> > +
> > + /* use the index to create the name if the label is not set */
> > + use_index = fwnode_property_read_string(child, "label", &str);
> > +
> > + /* for each physical LED, 4 LEDs are created representing
> > + * the 4 components: red, green, blue and global luma.
> > + */
> > + for (j = 0; j < CH_NUM; j++) {
> > + led = &priv->leds[i * CH_NUM + j];
> > +
> > + if (use_index)
> > + snprintf(led->name, sizeof(led->name),
> > + "apa102c:%s:%d", rgb_name[j], i);
> > + else
> > + snprintf(led->name, sizeof(led->name),
> > + "apa102c:%s:%s", rgb_name[j], str);
>
> LED core already handles LED name composition. Please refer to existing
> LED class drivers that use devm_led_classdev_register_ext() API and use
> it in your driver.
>
> > +
> > + fwnode_property_read_string(child,
> > + "linux,default-trigger",
> > + &led->ldev.default_trigger);
> > +
> > + led->priv = priv;
> > + led->ldev.name = led->name;
> > + if (j == LUMA) {
> > + led->ldev.brightness = led->brightness
>
> What do you want to achieve here?
>
> > + = LM_MAX_BRIGHTNESS;
> > + led->ldev.max_brightness = LM_MAX_BRIGHTNESS;
> > + } else {
> > + led->ldev.brightness = led->brightness
> > + = 0;
> > + led->ldev.max_brightness = CR_MAX_BRIGHTNESS;
> > + }
> > +
> > + led->ldev.brightness_set_blocking = apa102c_set_sync;
> > +
> > + ret = devm_led_classdev_register(priv->dev, &led->ldev);
>
> As mentioned above - new *ext API will make your life easier.
>
> > + if (ret) {
> > + dev_err(priv->dev,
> > + "failed to register LED %s, err %d",
> > + led->name, ret);
> > + fwnode_handle_put(child);
> > + return ret;
> > + }
> > +
> > + led->ldev.dev->of_node = np;
> > +
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int apa102c_probe(struct spi_device *spi)
> > +{
> > + struct apa102c *priv;
> > + size_t led_count;
> > + int ret;
> > +
> > + led_count = device_get_child_node_count(&spi->dev);
> > + if (!led_count) {
> > + dev_err(&spi->dev, "No LEDs defined in device tree!");
> > + return -ENODEV;
> > + }
> > +
> > + priv = devm_kzalloc(&spi->dev,
> > + struct_size(priv, leds, led_count * CH_NUM),
> > + GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->buf = devm_kzalloc(&spi->dev, led_count * CH_NUM + 8, GFP_KERNEL);
> > + if (!priv->buf)
> > + return -ENOMEM;
> > +
> > + mutex_init(&priv->lock);
> > + priv->led_count = led_count;
> > + priv->dev = &spi->dev;
> > + priv->spi = spi;
> > +
> > + ret = apa102c_probe_dt(priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Set the LEDs with default values at start */
> > + apa102c_sync(priv);
> > + if (ret)
> > + return ret;
> > +
> > + spi_set_drvdata(spi, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int apa102c_remove(struct spi_device *spi)
> > +{
> > + struct apa102c *priv = spi_get_drvdata(spi);
> > +
> > + mutex_destroy(&priv->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id apa102c_dt_ids[] = {
> > + { .compatible = "shiji,apa102c", },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, apa102c_dt_ids);
> > +
> > +static struct spi_driver apa102c_driver = {
> > + .probe = apa102c_probe,
> > + .remove = apa102c_remove,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = apa102c_dt_ids,
> > + },
> > +};
> > +
> > +module_spi_driver(apa102c_driver);
> > +
> > +MODULE_AUTHOR("Nicolas Belin <nbelin@baylibre.com>");
> > +MODULE_DESCRIPTION("apa102c LED driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:apa102c");
> >
>
> --
> Best regards,
> Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH V3 2/7] dt-bindings: gpio: Add binding for Versal gpio
From: Bartosz Golaszewski @ 2020-02-20 10:25 UTC (permalink / raw)
To: Srinivas Neeli
Cc: Linus Walleij, Michal Simek, shubhrajyoti.datta, linux-gpio,
arm-soc, LKML, git
In-Reply-To: <1581942793-19468-3-git-send-email-srinivas.neeli@xilinx.com>
pon., 17 lut 2020 o 13:33 Srinivas Neeli <srinivas.neeli@xilinx.com> napisał(a):
>
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>
> Add binding for Versal binding.
Please add a short note on Versal here as well - don't worry about
duplicating commit messages: when I git blame the line and go to see
the commit that introduced it, I want to get the whole picture.
Bart
^ permalink raw reply
* Re: [PATCH V3 2/7] dt-bindings: gpio: Add binding for Versal gpio
From: Bartosz Golaszewski @ 2020-02-20 10:25 UTC (permalink / raw)
To: Srinivas Neeli
Cc: Linus Walleij, shubhrajyoti.datta, Michal Simek, LKML, linux-gpio,
git, arm-soc
In-Reply-To: <1581942793-19468-3-git-send-email-srinivas.neeli@xilinx.com>
pon., 17 lut 2020 o 13:33 Srinivas Neeli <srinivas.neeli@xilinx.com> napisał(a):
>
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>
> Add binding for Versal binding.
Please add a short note on Versal here as well - don't worry about
duplicating commit messages: when I git blame the line and go to see
the commit that introduced it, I want to get the whole picture.
Bart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 11/13] p11-kit: upgrade 0.23.18.1 -> 0.23.20
From: Richard Purdie @ 2020-02-20 10:26 UTC (permalink / raw)
To: Alexander Kanavin, openembedded-core
In-Reply-To: <20200219194752.7967-11-alex.kanavin@gmail.com>
On Wed, 2020-02-19 at 20:47 +0100, Alexander Kanavin wrote:
> Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
> ---
> .../p11-kit/{p11-kit_0.23.18.1.bb => p11-kit_0.23.20.bb} | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> rename meta/recipes-support/p11-kit/{p11-kit_0.23.18.1.bb => p11-kit_0.23.20.bb} (93%)
>
> diff --git a/meta/recipes-support/p11-kit/p11-kit_0.23.18.1.bb b/meta/recipes-support/p11-kit/p11-kit_0.23.20.bb
> similarity index 93%
> rename from meta/recipes-support/p11-kit/p11-kit_0.23.18.1.bb
> rename to meta/recipes-support/p11-kit/p11-kit_0.23.20.bb
> index 19895ec269..26833ef431 100644
> --- a/meta/recipes-support/p11-kit/p11-kit_0.23.18.1.bb
> +++ b/meta/recipes-support/p11-kit/p11-kit_0.23.20.bb
> @@ -9,7 +9,7 @@ DEPENDS = "libtasn1 libtasn1-native libffi"
> DEPENDS_append = "${@' glib-2.0' if d.getVar('GTKDOC_ENABLED') == 'True' else ''}"
>
> SRC_URI = "git://github.com/p11-glue/p11-kit"
> -SRCREV = "b0ebe7555c291808db29377ba79cb8326301f0a6"
> +SRCREV = "762cdaa2cd5c5ec09cc844f9a6bdc551c7f6c8ed"
> S = "${WORKDIR}/git"
>
> PACKAGECONFIG ??= ""
This version adds bash-completion which is triggering in some builds
and isn't deterministic.
https://autobuilder.yoctoproject.org/typhoon/#/builders/108/builds/291
Cheers,
Richard
^ permalink raw reply
* [Buildroot] [PATCH v3 1/2] support/scripts/pkg-stats: add support for CVE reporting
From: Titouan Christophe @ 2020-02-20 10:26 UTC (permalink / raw)
To: buildroot
In-Reply-To: <CAAXf6LV8MSXPGrQW2GUhJcR4pjupw2EHFb7H6x4R0OkbrYFiMQ@mail.gmail.com>
Hello Thomas^2 and all,
On 2/19/20 9:33 PM, Thomas De Schampheleire wrote:
> El mi?., 19 feb. 2020 a las 19:49, Thomas Petazzoni
> (<thomas.petazzoni@bootlin.com>) escribi?:
>>
>> Hello Titouan,
>>
>> On Sat, 15 Feb 2020 13:44:16 +0100
>> Titouan Christophe <titouan.christophe@railnova.eu> wrote:
>>
>>> This commit extends the pkg-stats script to grab information about the
>>> CVEs affecting the Buildroot packages.
>>
>> Here the script consumes too much memory. On my 4 GB RAM server, the
>> script gets killed by the OOM killer. It goes like this:
>>
[--SNIP--]
>>
>> So Python needs more than 4.2 GB of virtual memory, and 3.6 GB of
>> resident memory. To me, it feels like there is something wrong going on
>> with the NVD files.
I tried to evaluate how much memory the NVD JSON files actually use when
loaded as Python objects. To do that, I used the function given here:
https://goshippo.com/blog/measure-real-size-any-python-object/.
I used the file for the year 2018 as example. This file weights 10MB in
compressed form, and 254MB when uncompressed. I then call the function
get_size on json.load(gzip.GzipFile("nvdcve-1.0-2018.json.gz"))
In Python 2.7, the total size used is as high as 1531882276 Bytes (or
~1.5GB) ! The same test in Python 3.6 gives me 718038090 Bytes (~718MB).
>
> I did a full run to verify these findings, observing the free memory with 'top'.
> Even though this is not a fully scientific method, the 'used' memory
> before was ~4800 MB and while the CVE parsing was ongoing I saw peaks
> up to ~7900 MB. So yes, it seems there is a large memory footprint.
> As my machine has enough RAM, the analysis does complete and results
> seem correct.
>
>
> This seems to be caused mostly by the fact that we load the entire
> json file in memory.
> As a test, I just loaded the file from an interactive python session.
I guess we should then process the CVE files in streaming. This is quite
easy to do in the CVE.read_nvd_dir() method. I'll give it a try today.
[-- SNIP --]
> Doing some quick google search, I stumbled upon the 'pandas' python
> package, which has a read_json function too. During a quick test, it
> seemed to be more memory efficient, and the total memory size on
> subsequent reads stayed in the 2.x GB range.
You probably don't want to use pandas here, which is a large library
(10MB) to process data on top of numpy (pydata ecosystem). I use it a
lot for data analysis on other projects, but it is definitely overkill
to simply read a json file :)
>
> content = pandas.read_json('/tmp/nvd/nvdcve-1.0-2019.json.gz')
> content = pandas.read_json('/tmp/nvd/nvdcve-1.0-2018.json.gz')
>
> In the full test of pkg-stats, I still saw a peak memory usage near
> the end, but it 'seemed' better :-)
>
> Thomas, could you try this on your 4GB server?
>
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index c113cf9606..8b4035dfd4 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -29,6 +29,7 @@ import certifi
> import distutils.version
> import time
> import gzip
> +import pandas
> from urllib3 import HTTPSConnectionPool
> from urllib3.exceptions import HTTPError
> from multiprocessing import Pool
> @@ -231,7 +232,7 @@ class CVE:
> for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
> filename = CVE.download_nvd_year(nvd_dir, year)
> try:
> - content = json.load(gzip.GzipFile(filename))
> + content = pandas.read_json(gzip.GzipFile(filename))
> except:
> print("ERROR: cannot read %s. Please remove the file
> then rerun this script" % filename)
> raise
>
>
> pandas can be installed with pip.
>
>
> Best regards,
> Thomas
>
Kind regards,
Titouan
^ permalink raw reply
* [PATCH 1/2] tty: serial: samsung_tty: build it for any platform
From: Greg Kroah-Hartman @ 2020-02-20 10:26 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Kukjin Kim, Donghoon Yu, Hyunki Koo,
HYUN-KI KOO, Shinbeom Choi, Krzysztof Kozlowski, Jiri Slaby,
linux-arm-kernel, linux-samsung-soc, linux-kernel
There is no need to tie this driver to only a specific SoC, or compile
test, so remove that dependancy from the Kconfig rules.
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Donghoon Yu <hoony.yu@samsung.com>
Cc: Hyunki Koo <kkoos00@naver.com>
Cc: HYUN-KI KOO <hyunki00.koo@samsung.com>
Cc: Shinbeom Choi <sbeom.choi@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/serial/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 52eaac21ff9f..a310bd22f1e2 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -237,7 +237,6 @@ config SERIAL_CLPS711X_CONSOLE
config SERIAL_SAMSUNG
tristate "Samsung SoC serial support"
- depends on PLAT_SAMSUNG || ARCH_EXYNOS || COMPILE_TEST
select SERIAL_CORE
help
Support for the on-chip UARTs on the Samsung S3C24XX series CPUs,
base-commit: 11a48a5a18c63fd7621bb050228cebf13566e4d8
--
2.25.1
^ permalink raw reply related
* [PATCH 1/2] tty: serial: samsung_tty: build it for any platform
From: Greg Kroah-Hartman @ 2020-02-20 10:26 UTC (permalink / raw)
To: linux-serial
Cc: Donghoon Yu, linux-samsung-soc, Greg Kroah-Hartman, linux-kernel,
Krzysztof Kozlowski, Shinbeom Choi, Hyunki Koo, Kukjin Kim,
linux-arm-kernel, Jiri Slaby, HYUN-KI KOO
There is no need to tie this driver to only a specific SoC, or compile
test, so remove that dependancy from the Kconfig rules.
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Donghoon Yu <hoony.yu@samsung.com>
Cc: Hyunki Koo <kkoos00@naver.com>
Cc: HYUN-KI KOO <hyunki00.koo@samsung.com>
Cc: Shinbeom Choi <sbeom.choi@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/serial/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 52eaac21ff9f..a310bd22f1e2 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -237,7 +237,6 @@ config SERIAL_CLPS711X_CONSOLE
config SERIAL_SAMSUNG
tristate "Samsung SoC serial support"
- depends on PLAT_SAMSUNG || ARCH_EXYNOS || COMPILE_TEST
select SERIAL_CORE
help
Support for the on-chip UARTs on the Samsung S3C24XX series CPUs,
base-commit: 11a48a5a18c63fd7621bb050228cebf13566e4d8
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/2] tty: serial: samsung_tty: remove SERIAL_SAMSUNG_DEBUG
From: Greg Kroah-Hartman @ 2020-02-20 10:26 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Kukjin Kim, Donghoon Yu, Hyunki Koo,
HYUN-KI KOO, Shinbeom Choi, Krzysztof Kozlowski, Jiri Slaby,
linux-arm-kernel, linux-samsung-soc, linux-kernel
In-Reply-To: <20200220102628.3371996-1-gregkh@linuxfoundation.org>
Since a05025d0ce72 ("tty: serial: samsung_tty: use standard debugging
macros") this configuration option is not used at all, so remove it from
the Kconfig file.
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Donghoon Yu <hoony.yu@samsung.com>
Cc: Hyunki Koo <kkoos00@naver.com>
Cc: HYUN-KI KOO <hyunki00.koo@samsung.com>
Cc: Shinbeom Choi <sbeom.choi@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/serial/Kconfig | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index a310bd22f1e2..fa4380b0b869 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -259,15 +259,6 @@ config SERIAL_SAMSUNG_UARTS
help
Select the number of available UART ports for the Samsung S3C
serial driver
-
-config SERIAL_SAMSUNG_DEBUG
- bool "Samsung SoC serial debug"
- depends on SERIAL_SAMSUNG && DEBUG_LL
- help
- Add support for debugging the serial driver. Since this is
- generally being used as a console, we use our own output
- routines that go via the low-level debug printascii()
- function.
config SERIAL_SAMSUNG_CONSOLE
bool "Support for console on Samsung SoC serial port"
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.