Subject: Re: [PATCH 3/3] vTPM: support little endian guests
From: "Hon Ching (Vicky) Lo" <honclo@linux.vnet.ibm.com>
To: Ashley Lai <ashley@ashleylai.com>
Cc: tpmdd-devel@lists.sourceforge.net, Peter Huewe <PeterHuewe@gmx.de>,
        Mimi Zohar <zohar@linux.vnet.ibm.com>, Vicky Lo <honclo2014@gmail.com>,
        linux-kernel@vger.kernel.org, Joy Latten <jmlatten@linux.vnet.ibm.com>
In-Reply-To: <alpine.DEB.2.11.1505191533410.15762@ant>
References: <1430873486-25868-1-git-send-email-honclo@linux.vnet.ibm.com>
	 <1430873486-25868-4-git-send-email-honclo@linux.vnet.ibm.com>
	 <alpine.DEB.2.11.1505191533410.15762@ant>
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
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4378
Lines: 133

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/