RE: [PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[Mario_Limonciello]

> -----Original Message-----
> From: MichaÅ KÄpieÅ [mailto:kernel@xxxxxxxxxx]
> Sent: Monday, May 9, 2016 7:02 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-
> driver-x86@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] dell-smbios: Add a helper function for complex
> SMI requests
> 
> > More complex SMI requests can return data that exceeds the 4 32 byte
> 
> 32-bit

Ack, thanks.

> 
> > arguments that are traditionally part of a request.
> >
> > To support more complex requests, the first input argument can be a
> > 32 bit physical address with a buffer properly built in advance.
> >
> > This helper function prepares the buffer as the BIOS will look for it
> > in these requests.
> >
> > Signed-off-by: Mario Limonciello <mario_limonciello@xxxxxxxx>
> > ---
> >  drivers/platform/x86/dell-smbios.c | 28
> ++++++++++++++++++++++++++++
> > drivers/platform/x86/dell-smbios.h |  1 +
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-smbios.c
> > b/drivers/platform/x86/dell-smbios.c
> > index d2412ab..00b29df 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -92,6 +92,34 @@ void dell_smbios_send_request(int class, int
> > select)  }  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
> >
> > +/* More complex requests are served by sending
> > + * a pointer to a pre-allocated buffer
> > + * Bytes 0:3 are the size of the return value
> > + * Bytes 4:length are the returned value
> > + *
> > + * This helper function prepares it properly
> > + * with the size length to be provided
> > + *
> > + * caller still needs to pre-allocate and clear
> > + * the input buffer
> 
> Which input buffer, the original one (struct calling_interface_buffer) or the
> one for "complex requests"?
> 
> If the former, pre-allocation is done by dell_smbios_init() while
> dell_smbios_get_buffer() clears the buffer before use, so the comment
> above may be misleading.
> 
> If the latter, then the caller in patch 2/2 doesn't clear the input buffer, it just
> allocates it using __get_free_page().

Yeah this is a comment mistake.  I was meaning for  the caller to just allocated it, but as your other comment noted it's better for code re-use to have this happen in dell-smbios, so I'll try to work towards that goal in next patch attempt.

> 
> > + */
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length) {
> > +	u32 i;
> > +	u32 *data = (u32 *) input;
> > +
> > +	data[0] = length - 4;
> > +	for (i = 4; i < length; i += 4) {
> > +		if (length - i > 4) {
> > +			input[i]   = 'D';
> > +			input[i+1] = 'S';
> > +			input[i+2] = 'C';
> > +			input[i+3] = 'I';
> 
> memcpy(&input[i], "DSCI", 4) or something along those lines?
> 

Yeah that's cleaner, I'll use that, thanks.

> Anyway, I'm confused about filling the buffer with repeating "DSCI"
> strings.  The buffer's length is provided explicitly in its first four bytes.  Some
> bytes at the end will always be left uninitialized.  So why bother initializing
> anything besides length at all?
> 

The reason for any of this in the first place was some situations where memory was getting clobbered because it wasn't properly allocated by users of this interface.  So the determination was to have a the 32 bit unsigned length the first 4 bytes and then repeat that string on 4 byte alignment for the rest of the buffer.  Anything not aligned on 4 bytes isn't checked by the SMM, so it's not important to initialize.

The SMM will check the buffer for that repeating string and reject (error -5) if it's not built properly.  

> > +		}
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smbios_prepare_v2_call);
> > +
> >  struct calling_interface_token *dell_smbios_find_token(int tokenid)
> > {
> >  	int i;
> > diff --git a/drivers/platform/x86/dell-smbios.h
> > b/drivers/platform/x86/dell-smbios.h
> > index ec7d40a..5d4184c 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -41,6 +41,7 @@ struct calling_interface_buffer
> > *dell_smbios_get_buffer(void);  void dell_smbios_clear_buffer(void);
> > void dell_smbios_release_buffer(void);  void
> > dell_smbios_send_request(int class, int select);
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length);
> 
> I will send my remarks regarding the proposed changes to dell-smbios API in
> response to patch 2/2.
> 
> --
> Best regards,
> MichaÅ KÄpieÅ