Re: [PATCH v7 4/4] boot/param: add pointer to next argument to unknown parameter callback

[msuchanek]

On Thu, 24 Aug 2017 21:04:51 +1000
Michael Ellerman  wrote:

> Michal Suchanek  writes:
> 
> > The fadump parameter processing re-does the logic of next_arg quote
> > stripping to determine where the argument ends. Pass pointer to the
> > next argument instead to make this more robust.
> >
> > Signed-off-by: Michal Suchanek 
> > ---
> >  arch/powerpc/kernel/fadump.c  | 13 +
> >  arch/powerpc/mm/hugetlbpage.c |  4 ++--
> >  include/linux/moduleparam.h   |  2 +-
> >  init/main.c   | 12 ++--
> >  kernel/module.c   |  4 ++--
> >  kernel/params.c   | 19 +++
> >  lib/dynamic_debug.c   |  2 +-
> >  7 files changed, 28 insertions(+), 28 deletions(-)  
> 
> Can you split out a patch that adds the next argument and updates the
> callers. And then a patch for the fadump to use the new arg.
> 
> cheers

Yes, that makes sense.

Thanks

Michal

Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically.

[msuchanek]

On Thu, 24 Aug 2017 21:02:47 +1000
Michael Ellerman  wrote:

> Michal Suchanek  writes:
> 
> > Remove quotes from argument value only if there is qoute on both
> > sides.
> >
> > Signed-off-by: Michal Suchanek 
> > ---
> >  arch/powerpc/kernel/fadump.c | 6 ++
> >  lib/cmdline.c| 7 ++-  
> 
> Can you split that into two patches?

Not really. There is logic in lib/cmdline.c which is duplicated in
arch/powerpc/kernel/fadump.c and so the two places should be updated in
sync. 

Thanks

Michal

> 
> cheers
> 
> >  2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/fadump.c
> > b/arch/powerpc/kernel/fadump.c index a1614d9b8a21..d7da4ce9f7ae
> > 100644 --- a/arch/powerpc/kernel/fadump.c
> > +++ b/arch/powerpc/kernel/fadump.c
> > @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct
> > param_info *param_info, *tgt++ = ' ';
> >  
> > /* next_arg removes one leading and one trailing '"' */
> > -   if (*tgt == '"')
> > -   shortening += 1;
> > -   if (*(tgt + vallen + shortening) == '"')
> > -   shortening += 1;
> > +   if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"'))
> > +   shortening += 2;
> >  
> > /* remove one leading and one trailing quote if both are
> > present */ if ((val[0] == '"') && (val[vallen - 1] == '"')) {
> > diff --git a/lib/cmdline.c b/lib/cmdline.c
> > index 4c0888c4a68d..01e701b2afe8 100644
> > --- a/lib/cmdline.c
> > +++ b/lib/cmdline.c
> > @@ -227,14 +227,11 @@ char *next_arg(char *args, char **param, char
> > **val) *val = args + equals + 1;
> >  
> > /* Don't include quotes in value. */
> > -   if (**val == '"') {
> > +   if ((**val == '"') && (args[i-1] == '"')) {
> > (*val)++;
> > -   if (args[i-1] == '"')
> > -   args[i-1] = '\0';
> > +   args[i-1] = '\0';
> > }
> > }
> > -   if (quoted && args[i-1] == '"')
> > -   args[i-1] = '\0';
> >  
> > if (args[i]) {
> > args[i] = '\0';
> > -- 
> > 2.10.2  

Re: [RFT PATCH] tpm: ibmvtpm: simplify crq initialization and document crq format

[msuchanek]

ping?

On Fri, 24 Feb 2017 20:35:16 +0100
Michal Suchanek  wrote:

> The crq is passed in registers and is the same on BE and LE hosts.
> However, current implementation allocates a structure on-stack to
> represent the crq, initializes the members swapping them to BE, and
> loads the structure swapping it from BE. This is pointless and causes
> GCC warnings about ununitialized members. Get rid of the structure and
> the warnings.
> 
> Signed-off-by: Michal Suchanek 
> Reviewed-by: Jarkko Sakkinen 
> ---
> v2
> 
> fix typos and spelling in comments
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 96
> ++ 1 file changed, 60
> insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c index 1b9d61ffe991..89027339d55f
> 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -39,19 +39,63 @@ static struct vio_device_id
> tpm_ibmvtpm_device_table[] = { MODULE_DEVICE_TABLE(vio,
> tpm_ibmvtpm_device_table); 
>  /**
> + *
> + * ibmvtpm_send_crq_word - Send a CRQ request
> + * @vdev:vio device struct
> + * @w1:  pre-constructed first word of tpm crq (second
> word is reserved)
> + *
> + * Return:
> + *   0 - Success
> + *   Non-zero - Failure
> + */
> +static int ibmvtpm_send_crq_word(struct vio_dev *vdev, u64 w1)
> +{
> + return plpar_hcall_norets(H_SEND_CRQ, vdev->unit_address,
> w1, 0); +}
> +
> +/**
> + *
>   * ibmvtpm_send_crq - Send a CRQ request
>   *
>   * @vdev:vio device struct
> - * @w1:  first word
> - * @w2:  second word
> + * @valid:   Valid field
> + * @msg: Type field
> + * @len: Length field
> + * @data:Data field
> + *
> + * The ibmvtpm crq is defined as follows:
> + *
> + * Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |
> 7
> + *
> ---
> + * Word0 | Valid | Type  | Length|  Data
> + *
> ---
> + * Word1 |Reserved
> + *
> ---
> + *
> + * Which matches the following structure (on bigendian host):
> + *
> + * struct ibmvtpm_crq {
> + * u8 valid;
> + * u8 msg;
> + * __be16 len;
> + * __be32 data;
> + * __be64 reserved;
> + * } __attribute__((packed, aligned(8)));
> + *
> + * However, the value is passed in a register so just compute the
> numeric value
> + * to load into the register avoiding byteswap altogether. Endian
> only affects
> + * memory loads and stores - registers are internally represented
> the same. *
>   * Return:
> - *   0 -Sucess
> + *   0 (H_SUCCESS) - Success
>   *   Non-zero - Failure
>   */
> -static int ibmvtpm_send_crq(struct vio_dev *vdev, u64 w1, u64 w2)
> +static int ibmvtpm_send_crq(struct vio_dev *vdev,
> + u8 valid, u8 msg, u16 len, u32 data)
>  {
> - return plpar_hcall_norets(H_SEND_CRQ, vdev->unit_address,
> w1, w2);
> + u64 w1 = ((u64)valid << 56) | ((u64)msg << 48) | ((u64)len
> << 32) |
> + (u64)data;
> + return ibmvtpm_send_crq_word(vdev, w1);
>  }
>  
>  /**
> @@ -109,8 +153,6 @@ static int tpm_ibmvtpm_recv(struct tpm_chip
> *chip, u8 *buf, size_t count) static int tpm_ibmvtpm_send(struct
> tpm_chip *chip, u8 *buf, size_t count) {
>   struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(>dev);
> - struct ibmvtpm_crq crq;
> - __be64 *word = (__be64 *)
>   int rc, sig;
>  
>   if (!ibmvtpm->rtce_buf) {
> @@ -137,10 +179,6 @@ static int tpm_ibmvtpm_send(struct tpm_chip
> *chip, u8 *buf, size_t count) spin_lock(>rtce_lock);
>   ibmvtpm->res_len = 0;
>   memcpy((void *)ibmvtpm->rtce_buf, (void *)buf, count);
> - crq.valid = (u8)IBMVTPM_VALID_CMD;
> - crq.msg = (u8)VTPM_TPM_COMMAND;
> - crq.len = cpu_to_be16(count);
> - crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>  
>   /*
>* set the processing flag before the Hcall, since we may
> get the @@ -148,8 +186,9 @@ static int tpm_ibmvtpm_send(struct
> tpm_chip *chip, u8 *buf, size_t count) */
>   ibmvtpm->tpm_processing_cmd = true;
>  
> - rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
> -   be64_to_cpu(word[1]));
> + rc = ibmvtpm_send_crq(ibmvtpm->vdev,
> + IBMVTPM_VALID_CMD, VTPM_TPM_COMMAND,
> + count, ibmvtpm->rtce_dma_handle);
>   if (rc != H_SUCCESS) {
>   dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed
> rc=%d\n", rc); rc = 0;
> @@ -182,15 +221,10 @@ static u8 tpm_ibmvtpm_status(struct tpm_chip
> *chip) */
>  static int ibmvtpm_crq_get_rtce_size(struct ibmvtpm_dev *ibmvtpm)
>  {
> - struct ibmvtpm_crq crq;
> - u64 *buf = (u64 *) 
>   int rc;
>  
> - crq.valid = 

Re: [PATCH v7 3/4] lib/cmdline.c Remove quotes symmetrically.

[msuchanek]

On Thu, 17 Aug 2017 22:14:30 +0200
Michal Suchanek  wrote:

> Remove quotes from argument value only if there is qoute on both
> sides.
> 
> Signed-off-by: Michal Suchanek 
> ---
>  arch/powerpc/kernel/fadump.c | 6 ++
>  lib/cmdline.c| 7 ++-
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/fadump.c
> b/arch/powerpc/kernel/fadump.c index a1614d9b8a21..d7da4ce9f7ae 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -489,10 +489,8 @@ static void __init fadump_update_params(struct
> param_info *param_info, *tgt++ = ' ';
>  
>   /* next_arg removes one leading and one trailing '"' */
> - if (*tgt == '"')
> - shortening += 1;
> - if (*(tgt + vallen + shortening) == '"')
> - shortening += 1;
> + if ((*tgt == '"') && (*(tgt + vallen + shortening) == '"'))
s/shortening/1/^

in case somebody would want this patch and not the following one that
removes the code.

Re: [PATCH v5 2/2] powerpc/fadump: update documentation about 'fadump_append=' parameter

[msuchanek]

Hello,

On Wed, 12 Jul 2017 00:00:57 +0530
Hari Bathini  wrote:

> Hi Michal,
> 
> 
> Thanks for the review..
> 
> 
> On Monday 26 June 2017 05:45 PM, Michal Suchánek wrote:
> > Hello,
> >
> > On Tue, 20 Jun 2017 21:14:08 +0530
> > Hari Bathini  wrote:
> >  

> I would prefer documenting over a complex implementation. Actually, I
> am considering a simple approach of replacing every occurrence of
> "fadump_extra_args=" with "fadump_extra_args " in fadump capture
> kernel. The cmdline
> 
>"root=/dev/sda2 ro fadump_extra_args="a b c" crashkernel=512M 
> fadump_extra_args=d"
> 
> becomes
> 
>"root=/dev/sda2 ro fadump_extra_args "a b c" crashkernel=512M 
> fadump_extra_args d"

which is totally broken

> 
> in fadump capture kernel. This must take care of the pitfalls with
> the current approach and also,
> doesn't rely on parse_args() which was not designed for this scenario
> to start with..?

It was designed for parsing arguments. To handle replacing arguments
you have to extend it. You need to get more information from it for
this case.

Best regards

Michal

Re: ibmvtpm byteswapping inconsistency

[msuchanek]

Hello,

On 2017-01-27 21:32, Tyrel Datwyler wrote:

On 01/27/2017 11:58 AM, Benjamin Herrenschmidt wrote:

On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:

The problem is that we are packing an in-memory structure into 2
registers and it's expected that this structure is laid out in the
registers as if it had been loaded by a BE CPU.


This is only the case if the cpu is BE. If the cpu is LE, regardless 
of
the fact that our in memory structure is laid out BE, when we break 
it

into 2 words each of those words needs to be loaded LE.


That doesn't make sense and doesn't match the code... The structure
needs to always have the same in-register layout regardless of the
endianness of the CPU, especially since the underlying hypervisor
will most likely be BE :-)

Thta's why the code does a be64_to_cpu() when loading it, this in
effect performs a "BE" load, which on a BE CPU is just a normal load
and on LE is a swap to compensate for the CPU loading it the "wrong 
way

around".


Its possible being the end of the week I'm just a little dense, but
wouldn't be64_to_cpu() imply that we are byte-swapping something that 
is
already, or supposedly already, in BE format to cpu endianness? Which 
on

a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
been swapped from BE --> LE?

In my eyes the code does seem to support what I've argued. The same
thing is done in the scsi VIO drivers. The CRQ structure is laid out 
and

annotated BE. We use cpu_to_be() calls to load any non 8bit field.
Finally, each word is swapped to cpu endian when we hand it off for the
hcall.

from ibmvfc_send_event():

__be64 *crq_as_u64 = (__be64 *) >crq;

<..snip..>

if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
  be64_to_cpu(crq_as_u64[1] {

Again, maybe I'm missing something.



Ok, so you perform really difficult operation for no good reason. You 
say

that the ppc dual-endian works like this: there is an internal in-cpu
representation of numbers which is always the same. What is affected by
switching endian is how memory loads and stores work.

If you pass these two words in registers you never need to swap 
anything.



Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
---
Word0 | Valid | Type  | Length|  Data
---
Word1 |Reserved
---

The following definition looks to match:

struct ibmvtpm_crq {
u8 valid;
u8 msg;
__be16 len;
__be32 data;
__be64 reserved;
} __attribute__((packed, aligned(8)));


If under BE valid is first byte then it is MSB and you would get value
to pass in word 0 as (valid << 56) | (type << 48) | (length << 32 ) | 
data.


No swaps involved.

To achieve same with structure and swaps you would indeed first swap the
members and then the whole word. Much harder to read code that way, 
though.


Thanks

Michal