From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932217AbbETVYA (ORCPT ); Wed, 20 May 2015 17:24:00 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:33747 "EHLO e28smtp05.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932173AbbETVXu (ORCPT ); Wed, 20 May 2015 17:23:50 -0400 Subject: Re: [PATCH 3/3] vTPM: support little endian guests From: "Hon Ching (Vicky) Lo" To: Ashley Lai Cc: tpmdd-devel@lists.sourceforge.net, Peter Huewe , Mimi Zohar , Vicky Lo , linux-kernel@vger.kernel.org, Joy Latten In-Reply-To: References: <1430873486-25868-1-git-send-email-honclo@linux.vnet.ibm.com> <1430873486-25868-4-git-send-email-honclo@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" Date: Wed, 20 May 2015 17:23:39 -0400 Message-ID: <1432157019.1303.9.camel@vtpm2014.ibm.com> Mime-Version: 1.0 X-Mailer: Evolution 2.32.3 (2.32.3-34.el6) Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15052021-0017-0000-0000-000005415E05 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2015-05-19 at 16:08 -0500, Ashley Lai wrote: > Thank you Vicky and Joy for the clarification. This patch mainly > converts the fields in the tcpa_event structure. I see the code converts > everytime it accesses the event fields. Would it be more efficient if you > do the conversion once and reuse them when needed? Could > convert_to_host_format(x) takes x as a tcpa_event structure? > If not you still can convert individual fields and reuse them. I'm aware > that the pcr_value field is type u8 and it does not need the conversion > but if convert_to_host_format() can convert the structure it shouldn't convert > u8 type I think. > > > static const char* tcpa_event_type_strings[] = { > > "PREBOOT", > > @@ -82,9 +87,11 @@ static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos) > > event = addr; > > > > if ((addr + sizeof(struct tcpa_event)) < limit) { > > - if (event->event_type == 0 && event->event_size == 0) > > + if ((convert_to_host_format(event->event_type) == 0) && > > + (convert_to_host_format(event->event_size) == 0)) > > return NULL; > > - addr += sizeof(struct tcpa_event) + event->event_size; > > + addr += (sizeof(struct tcpa_event) + > > + convert_to_host_format(event->event_size)); > > } > > } > > > > @@ -94,8 +101,11 @@ static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos) > > > > event = addr; > > > > - if ((event->event_type == 0 && event->event_size == 0) || > > - ((addr + sizeof(struct tcpa_event) + event->event_size) >= limit)) > > + if (((convert_to_host_format(event->event_type) == 0) && > > + (convert_to_host_format(event->event_size) == 0)) > > + || > > + ((addr + sizeof(struct tcpa_event) + > > + convert_to_host_format(event->event_size)) >= limit)) > > return NULL; > > > > return addr; > > In this function, event_type and event_size can be converted > once and reuse. > > > case SEPARATOR: > > case ACTION: > > - if (MAX_TEXT_EVENT > event->event_size) { > > + if (MAX_TEXT_EVENT > > > + convert_to_host_format(event->event_size)) { > > name = event_entry; > > - n_len = event->event_size; > > + n_len = convert_to_host_format(event->event_size); > > } > > break; > > case EVENT_TAG: > > Same here. > Agree. It's more efficient to do the conversion once and reuse. convert_to_host_format(x) cannot take tcpa_event structure, as be64_to_cpu() only converts raw integers. > > @@ -208,11 +229,43 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) > > struct tcpa_event *event = v; > > char *data = v; > > int i; > > - > > - for (i = 0; i < sizeof(struct tcpa_event) + event->event_size; i++) > > + u32 x; > > + char tmp[4]; > > + > > + /* PCR */ > > + x = convert_to_host_format(event->pcr_index); > > + memcpy(tmp, &x, 4); > > + for (i = 0; i < 4; i++) > > + seq_putc(m, tmp[i]); > > + data += 4; > > + > > + /* Event Type */ > > + x = convert_to_host_format(event->event_type); > > + memcpy(tmp, &x, 4); > > + for (i = 0; i < 4; i++) > > + seq_putc(m, tmp[i]); > > + data += 4; > > + > > + /* HASH */ > > + for (i = 0; i < 20; i++) > > seq_putc(m, data[i]); > > + data += 20; > > + > > + /* Size */ > > + x = convert_to_host_format(event->event_size); > > + memcpy(tmp, &x, 4); > > + for (i = 0; i < 4; i++) > > + seq_putc(m, tmp[i]); > > + data += 4; > > + > > + /* Data */ > > + if (convert_to_host_format(event->event_size)) { > > + for (i = 0; i < convert_to_host_format(event->event_size); i++) > > + seq_putc(m, data[i]); > > + } > > > > return 0; > > + > > } > If the tcpa_event structure is converted, you may be able to get away > with memcpy and the for loop. > > Thanks, > --Ashley Lai > To simplify the code, we can try creating a struct that contains the first 4 fields in the tcpa_event so to only copy over the header part. Then we do conversion on it. That way, we can use a loop to print byte-by-byte on the new struct. Then, we can use a small loop to print byte-by-byte of the data part of the original event structure. I'll try this and re-submit patch. Thanks! Vicky