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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
