| Message ID | b59027b3-a48e-f168-5f12-c67758c0c2c7@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Jun 13, 2018 at 4:24 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: > > If the WSMT ACPI table is present and indicates that a fixed communication > buffer should be used, use the firmware-specified buffer instead of > allocating a buffer in memory for communications between the dcdbas driver > and firmare. Thanks for an update. My comments below. > - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE) > + if ((pos + count) > max_smi_data_buf_size) > return -EINVAL; Parens are redundant, but okay, not purpose of the change. > + /* Calling Interface SMI I suppose the style of the comments like /* * Calling ... ... > + * > + * Provide physical address of command buffer field within > + * the struct smi_cmd... can't use virt_to_phys on smi_cmd > + * because address may be from memremap. Wait, memremap() might return a virtual address. How we be sure that we got still physical address here? (Also note () when referring to functions) > + */ > + smi_cmd->ebx = smi_data_buf_phys_addr + > + offsetof(struct smi_cmd, command_buffer); Btw, can it be one line (~83 character are okay for my opinion). > + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt); > + if (!wsmt) > + return 0; > + > + /* Check if WSMT ACPI table shows that protection is enabled */ > + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) > + || !(wsmt->protection_flags > + & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION)) Better to indent like if (... || !(... & ...) > + return 0; > + > + /* Scan for EPS (entry point structure) */ > + for (addr = (u8 *)__va(0xf0000); > + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); > + addr += 1) { This wasn't commented IIRC and changed. So, why?
On 6/13/2018 3:54 AM, Andy Shevchenko wrote: >> + /* Calling Interface SMI > > I suppose the style of the comments like > /* > * Calling ... > ... Yes... goof on my part. Thanks. >> + * >> + * Provide physical address of command buffer field within >> + * the struct smi_cmd... can't use virt_to_phys on smi_cmd >> + * because address may be from memremap. > > Wait, memremap() might return a virtual address. How we be sure that > we got still physical address here? > > (Also note () when referring to functions) > >> + */ >> + smi_cmd->ebx = smi_data_buf_phys_addr + >> + offsetof(struct smi_cmd, command_buffer); > > Btw, can it be one line (~83 character are okay for my opinion). > Before this patch, the address in smi_cmd always came from an alloc, so virt_to_phys() was used to get the physical address here. With WSMT, we could be using a BIOS-provided buffer for SMI, in which case the address in smi_cmd will come from memremap(), so we can't use virt_to_phys() on it. So instead I changed this to use the physical address of smi_data_buf that is stored in smi_data_buf_phys_addr, which will be valid regardless of how the address of smi_data_buf was generated. But that would be like 97 characters long if I made it all one line... >> + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt); >> + if (!wsmt) >> + return 0; >> + >> + /* Check if WSMT ACPI table shows that protection is enabled */ >> + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) > >> + || !(wsmt->protection_flags >> + & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION)) > > Better to indent like > > if (... || > !(... & ...) > Yes thanks. >> + return 0; >> + >> + /* Scan for EPS (entry point structure) */ >> + for (addr = (u8 *)__va(0xf0000); >> + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); > >> + addr += 1) { > > This wasn't commented IIRC and changed. So, why? > I changed this is response to your earlier comment (7 june)... you had pointed out that it would be better if I put an "if (eps) break;" inside the for loop instead of having "&& !eps" in the condition of the for loop. I put the note "Changed loop searching 0xf0000 to be more readable" in the list of changes for patch version v3 to cover this change. Thanks again for your time!
On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: > On 6/13/2018 3:54 AM, Andy Shevchenko wrote: >>> + * Provide physical address of command buffer field within >>> + * the struct smi_cmd... can't use virt_to_phys on smi_cmd >>> + * because address may be from memremap. >> >> Wait, memremap() might return a virtual address. How we be sure that >> we got still physical address here? > Before this patch, the address in smi_cmd always came from an alloc, so > virt_to_phys() was used to get the physical address here. With WSMT, we > could be using a BIOS-provided buffer for SMI, in which case the address in > smi_cmd will come from memremap(), so we can't use virt_to_phys() on it. > So instead I changed this to use the physical address of smi_data_buf that > is stored in smi_data_buf_phys_addr, which will be valid regardless of how > the address of smi_data_buf was generated. Yes, but what does guarantee that memremap() will return you still physical address? >>> + return 0; >>> + >>> + /* Scan for EPS (entry point structure) */ >>> + for (addr = (u8 *)__va(0xf0000); >>> + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); >> >>> + addr += 1) { >> >> This wasn't commented IIRC and changed. So, why? > I changed this is response to your earlier comment (7 june)... you had pointed > out that it would be better if I put an "if (eps) break;" inside the for loop > instead of having "&& !eps" in the condition of the for loop. I put the note > "Changed loop searching 0xf0000 to be more readable" in the list of changes for > patch version v3 to cover this change. Thanks, but here I meant += 1 vs += 16 step.
On 6/14/2018 12:25 PM, Andy Shevchenko wrote: > On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >> On 6/13/2018 3:54 AM, Andy Shevchenko wrote: > >>>> + * Provide physical address of command buffer field within >>>> + * the struct smi_cmd... can't use virt_to_phys on smi_cmd >>>> + * because address may be from memremap. >>> >>> Wait, memremap() might return a virtual address. How we be sure that >>> we got still physical address here? > >> Before this patch, the address in smi_cmd always came from an alloc, so >> virt_to_phys() was used to get the physical address here. With WSMT, we >> could be using a BIOS-provided buffer for SMI, in which case the address in >> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it. >> So instead I changed this to use the physical address of smi_data_buf that >> is stored in smi_data_buf_phys_addr, which will be valid regardless of how >> the address of smi_data_buf was generated. > > Yes, but what does guarantee that memremap() will return you still > physical address? > Sorry, I'm not sure I understand the question. Up to now, this driver always just allocated a buffer from main memory that it used to send/receive information from BIOS when it generated a SMI. That's what smi_cmd points to where this comment is. And it was safe to use virt_to_phys() on this address. With this patch, though, the driver may now be using a buffer that isn't part of main memory--it could now be using a buffer that BIOS provided the physical address for, and this would not be part of main memory. So smi_cmd may contain a virtual address that memremap() provided. And because memremap() is just like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the physical address of the buffer. My comment is just pointing that out... I was trying to say, "the code can't use virt_to_phys(smi_cmd) to get the virtual address here". memremap() should always return a virtual address that points to the physical address we send it (unless it fails of course). >>>> + return 0; >>>> + >>>> + /* Scan for EPS (entry point structure) */ >>>> + for (addr = (u8 *)__va(0xf0000); >>>> + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); >>> >>>> + addr += 1) { >>> >>> This wasn't commented IIRC and changed. So, why? > >> I changed this is response to your earlier comment (7 june)... you had pointed >> out that it would be better if I put an "if (eps) break;" inside the for loop >> instead of having "&& !eps" in the condition of the for loop. I put the note >> "Changed loop searching 0xf0000 to be more readable" in the list of changes for >> patch version v3 to cover this change. > > Thanks, but here I meant += 1 vs += 16 step. > Sorry, I thought I had answered this earlier. The spec does not say that the EPS table will be on a 16-byte boundary. And I just added a printk in this driver to see where it is on the system I had at hand, and it isn't on a 16-byte boundary: [ 4680.192542] dcdbas - EPS table at 000000005761efb7 [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer. [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3)
On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: > On 6/14/2018 12:25 PM, Andy Shevchenko wrote: >> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote: >> >>>>> + * Provide physical address of command buffer field within >>>>> + * the struct smi_cmd... can't use virt_to_phys on smi_cmd >>>>> + * because address may be from memremap. >>>> >>>> Wait, memremap() might return a virtual address. How we be sure that >>>> we got still physical address here? >> >>> Before this patch, the address in smi_cmd always came from an alloc, so >>> virt_to_phys() was used to get the physical address here. With WSMT, we >>> could be using a BIOS-provided buffer for SMI, in which case the address in >>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it. >>> So instead I changed this to use the physical address of smi_data_buf that >>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how >>> the address of smi_data_buf was generated. >> >> Yes, but what does guarantee that memremap() will return you still >> physical address? > Sorry, I'm not sure I understand the question. > > Up to now, this driver always just allocated a buffer from main memory that > it used to send/receive information from BIOS when it generated a SMI. That's > what smi_cmd points to where this comment is. And it was safe to use > virt_to_phys() on this address. > > With this patch, though, the driver may now be using a buffer that isn't part > of main memory--it could now be using a buffer that BIOS provided the physical > address for, and this would not be part of main memory. Hmm... But is it CPU address or bus address what BIOS provides? If it's a CPU address why do you need to call memremap() on it in the first place? I could guess that you want to access it from CPU side and rather would get faults. > So smi_cmd may contain > a virtual address that memremap() provided. And because memremap() is just > like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the > physical address of the buffer. Yes, and ioremap() is dedicated for the resources that are not available directly by the memory accesses, but rather require some bus transactions (like MMIO). > > My comment is just pointing that out... I was trying to say, "the code can't > use virt_to_phys(smi_cmd) to get the virtual address here". > Please, add bits from above paragraphs to elaborate this in the comment. > memremap() should always return a virtual address that points to the physical > address we send it (unless it fails of course). >>>>> + /* Scan for EPS (entry point structure) */ >>>>> + for (addr = (u8 *)__va(0xf0000); >>>>> + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); >>>> >>>>> + addr += 1) { >>>> >>>> This wasn't commented IIRC and changed. So, why? >> >>> I changed this is response to your earlier comment (7 june)... you had pointed >>> out that it would be better if I put an "if (eps) break;" inside the for loop >>> instead of having "&& !eps" in the condition of the for loop. I put the note >>> "Changed loop searching 0xf0000 to be more readable" in the list of changes for >>> patch version v3 to cover this change. >> >> Thanks, but here I meant += 1 vs += 16 step. >> > > Sorry, I thought I had answered this earlier. The spec does not say that the EPS > table will be on a 16-byte boundary. And I just added a printk in this driver to > see where it is on the system I had at hand, and it isn't on a 16-byte boundary: Oh, that's sad. Btw, does XSDT have a link to this table? > [ 4680.192542] dcdbas - EPS table at 000000005761efb7 Can you, by the way, dump some bytes around this address, using print_hex_dump_bytes(); where the adrress is aligned to let say 32 byte boundary and size like 64 bytes? > [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer. > [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3) OK, now the most important question, did you investigate "SMM Communication ACPI Table"? Can you utilize information in it?
On 06/27/2018 06:52 PM, Andy Shevchenko wrote: > On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >> On 6/14/2018 12:25 PM, Andy Shevchenko wrote: >>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote: >>> >>>>>> + * Provide physical address of command buffer field within >>>>>> + * the struct smi_cmd... can't use virt_to_phys on smi_cmd >>>>>> + * because address may be from memremap. >>>>> >>>>> Wait, memremap() might return a virtual address. How we be sure that >>>>> we got still physical address here? >>> >>>> Before this patch, the address in smi_cmd always came from an alloc, so >>>> virt_to_phys() was used to get the physical address here. With WSMT, we >>>> could be using a BIOS-provided buffer for SMI, in which case the address in >>>> smi_cmd will come from memremap(), so we can't use virt_to_phys() on it. >>>> So instead I changed this to use the physical address of smi_data_buf that >>>> is stored in smi_data_buf_phys_addr, which will be valid regardless of how >>>> the address of smi_data_buf was generated. >>> >>> Yes, but what does guarantee that memremap() will return you still >>> physical address? > >> Sorry, I'm not sure I understand the question. >> >> Up to now, this driver always just allocated a buffer from main memory that >> it used to send/receive information from BIOS when it generated a SMI. That's >> what smi_cmd points to where this comment is. And it was safe to use >> virt_to_phys() on this address. >> >> With this patch, though, the driver may now be using a buffer that isn't part >> of main memory--it could now be using a buffer that BIOS provided the physical >> address for, and this would not be part of main memory. > > Hmm... But is it CPU address or bus address what BIOS provides? > > If it's a CPU address why do you need to call memremap() on it in the > first place? > I could guess that you want to access it from CPU side and rather > would get faults. > The BIOS-provided EPS provides the physical (bus) address of the fixed SMI buffer. This memory will be of type EfiReservedMemoryType, so it will not be usable RAM to the linux kernel and won't already be mapped. >> So smi_cmd may contain >> a virtual address that memremap() provided. And because memremap() is just >> like ioremap(), the driver can no longer use virt_to_phys(smi_cmd) to get the >> physical address of the buffer. > > Yes, and ioremap() is dedicated for the resources that are not > available directly by the memory accesses, but rather require some bus > transactions (like MMIO)>> >> My comment is just pointing that out... I was trying to say, "the code can't >> use virt_to_phys(smi_cmd) to get the virtual address here". >> > > Please, add bits from above paragraphs to elaborate this in the comment. > No problem, will do. >> memremap() should always return a virtual address that points to the physical >> address we send it (unless it fails of course). > >>>>>> + /* Scan for EPS (entry point structure) */ >>>>>> + for (addr = (u8 *)__va(0xf0000); >>>>>> + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); >>>>> >>>>>> + addr += 1) { >>>>> >>>>> This wasn't commented IIRC and changed. So, why? >>> >>>> I changed this is response to your earlier comment (7 june)... you had pointed >>>> out that it would be better if I put an "if (eps) break;" inside the for loop >>>> instead of having "&& !eps" in the condition of the for loop. I put the note >>>> "Changed loop searching 0xf0000 to be more readable" in the list of changes for >>>> patch version v3 to cover this change. >>> >>> Thanks, but here I meant += 1 vs += 16 step. >>> >> >> Sorry, I thought I had answered this earlier. The spec does not say that the EPS >> table will be on a 16-byte boundary. And I just added a printk in this driver to >> see where it is on the system I had at hand, and it isn't on a 16-byte boundary: > > Oh, that's sad. > > Btw, does XSDT have a link to this table? > The XSDT has a link to the WSMT table, but not to the EPS table that actually has the address of the buffer that BIOS provides. The EPS table isn't an ACPI table, it's just a Dell-defined table. I did realize that the debug code that printed the address of the EPS table was not working right, and the table on my system is actually 16-byte aligned. However, the documentation on the EPS does not specify that it will be 16-byte aligned, so I don't think the driver should make that assumption, especially since scanning a 64K region byte by byte should take very little extra time over scanning every 16th byte. >> [ 4680.192542] dcdbas - EPS table at 000000005761efb7 > > Can you, by the way, dump some bytes around this address, using > print_hex_dump_bytes(); > > where the adrress is aligned to let say 32 byte boundary and size like 64 bytes? > I guess this isn't needed since the table on my system actually is 16-byte aligned. >> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer. >> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3) > > OK, now the most important question, did you investigate "SMM > Communication ACPI Table"? > Can you utilize information in it? > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS. It appears that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec. Also, the dcdbas driver is for Dell systems only.
On Fri, Jun 29, 2018 at 9:56 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: > On 06/27/2018 06:52 PM, Andy Shevchenko wrote: >> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote: >>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: >>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote: >>>> Thanks, but here I meant += 1 vs += 16 step. > I did realize that the debug code that printed the address of the EPS table was not > working right, and the table on my system is actually 16-byte aligned. However, > the documentation on the EPS does not specify that it will be 16-byte aligned, so I > don't think the driver should make that assumption, especially since scanning a 64K > region byte by byte should take very little extra time over scanning every 16th byte. > I guess this isn't needed since the table on my system actually is 16-byte aligned. I think the 16 byte is a natural alignment applicable to all tables like a such. I would rather go with it until we will get a (weird) BIOS which does not support that. (I believe this alignment just comes by definition of C ABIs for the structures / types. So, each defined type is located on an aligned boundaries) >>> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI buffer. >>> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version 5.6.0-3.3) >> >> OK, now the most important question, did you investigate "SMM >> Communication ACPI Table"? >> Can you utilize information in it? > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS. It appears > that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec. Also, the > dcdbas driver is for Dell systems only. > The EPS table isn't an ACPI table, > it's just a Dell-defined table. Hmm... It's either strange or has some background why is so. OK it has a note that there is no use case, but here it is! Mario, can we consider this as a BIOS bug which doesn't provide SMM Communication ACPI table? Can it be escalated on UEFI committee?
> -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Friday, June 29, 2018 2:30 PM > To: Stuart Hayes > Cc: Darren Hart; Limonciello, Mario; Linux Kernel Mailing List; Platform Driver > Subject: Re: [PATCH v3] dcdbas: Add support for WSMT ACPI table > > On Fri, Jun 29, 2018 at 9:56 PM, Stuart Hayes <stuart.w.hayes@gmail.com> wrote: > > On 06/27/2018 06:52 PM, Andy Shevchenko wrote: > >> On Fri, Jun 15, 2018 at 1:31 AM, Stuart Hayes <stuart.w.hayes@gmail.com> > wrote: > >>> On 6/14/2018 12:25 PM, Andy Shevchenko wrote: > >>>> On Thu, Jun 14, 2018 at 5:22 PM, Stuart Hayes <stuart.w.hayes@gmail.com> > wrote: > >>>>> On 6/13/2018 3:54 AM, Andy Shevchenko wrote: > > >>>> Thanks, but here I meant += 1 vs += 16 step. > > > I did realize that the debug code that printed the address of the EPS table was not > > working right, and the table on my system is actually 16-byte aligned. However, > > the documentation on the EPS does not specify that it will be 16-byte aligned, so I > > don't think the driver should make that assumption, especially since scanning a > 64K > > region byte by byte should take very little extra time over scanning every 16th > byte. > > > I guess this isn't needed since the table on my system actually is 16-byte aligned. > > I think the 16 byte is a natural alignment applicable to all tables like a such. > I would rather go with it until we will get a (weird) BIOS which does > not support that. > > (I believe this alignment just comes by definition of C ABIs for the > structures / types. So, each defined type is located on an aligned > boundaries) > > >>> [ 4680.194012] dcdbas dcdbas: WSMT found, using firmware-provided SMI > buffer. > >>> [ 4680.195327] dcdbas dcdbas: Dell Systems Management Base Driver (version > 5.6.0-3.3) > >> > >> OK, now the most important question, did you investigate "SMM > >> Communication ACPI Table"? > >> Can you utilize information in it? > > > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS. It > appears > > that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec. > Also, the > > dcdbas driver is for Dell systems only. > > > The EPS table isn't an ACPI table, > > it's just a Dell-defined table. > > Hmm... It's either strange or has some background why is so. > OK it has a note that there is no use case, but here it is! > > Mario, can we consider this as a BIOS bug which doesn't provide SMM > Communication ACPI table? > > Can it be escalated on UEFI committee? > I don't believe SMM communication ACPI table has ever been implemented by Dell on server or client BIOS. I do agree this table describes the behavior that DCDBAS driver has used since before even UEFI BIOS pretty accurately. Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if its possible to move EPS to SMM communication ACPI table however since it's been deprecated by UEFI 2.7 they weren't willing to adopt it. Stuart, anything else you want to add here?
On Mon, Jul 2, 2018 at 5:07 PM, <Mario.Limonciello@dell.com> wrote: >> >> OK, now the most important question, did you investigate "SMM >> >> Communication ACPI Table"? >> >> Can you utilize information in it? >> >> > Dell BIOS does not provide a "SMM Communication ACPI Table", just the EPS. It >> appears >> > that the "SMM Communication ACPI Table" is deprecated in the UEFI 2.7 spec. >> Also, the >> > dcdbas driver is for Dell systems only. >> >> > The EPS table isn't an ACPI table, >> > it's just a Dell-defined table. >> >> Hmm... It's either strange or has some background why is so. >> OK it has a note that there is no use case, but here it is! >> >> Mario, can we consider this as a BIOS bug which doesn't provide SMM >> Communication ACPI table? >> >> Can it be escalated on UEFI committee? > I don't believe SMM communication ACPI table has ever been implemented by Dell > on server or client BIOS. I do agree this table describes the behavior that DCDBAS driver > has used since before even UEFI BIOS pretty accurately. So, EPS table has been for ages in Dell machines? Can we consider it as a predecessor of that SMM communication ACPI table? > Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if its possible > to move EPS to SMM communication ACPI table however since it's been deprecated by > UEFI 2.7 they weren't willing to adopt it. It's pity, but the motivation to deprecate is "lack of use" which is not true. That's why I would suggest to escalate this to UEFI committee. > Stuart, anything else you want to add here? Darren, what's your opinion about this? P.S. I'm not against this approach (just some technical comments I already shared), but on the other hand it would be nice to have undo that deprecation and follow the standard in new firmwares. Would you agree?
> > > I don't believe SMM communication ACPI table has ever been implemented by > Dell > > on server or client BIOS. I do agree this table describes the behavior that DCDBAS > driver > > has used since before even UEFI BIOS pretty accurately. > > So, EPS table has been for ages in Dell machines? > Can we consider it as a predecessor of that SMM communication ACPI table? No, EPS is new this year, specifically for server BIOS to be able to support SMM communication when WSMT is enabled. The code tests in Stuart's patch will detect if WSMT is enabled and if it's enabled test if EPS was defined. On server BIOS when EPS is defined dcdbas will be able to communicate using addresses defined in EPS. Server BIOS will support EPS for applications using dcdbas interface and may at a later time introduce same WMI interface as client too (but applications will need time to update so they need to support both). Actually Stuart's patch will cause client BIOS that has WSMT enabled make dcdbas fail initialization (as it should because dcdbas doesn't have a region that it can successfully communicate). In client machines we moved this communication to ACPI buffer allocated by WMI, which is why we have dell-smbios-wmi now in kernel. I think once some variation of Stuart's patch is merged, I'll send a follow up patch to drop this test because it's no longer necessary: https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106 > > > Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if > its possible > > to move EPS to SMM communication ACPI table however since it's been > deprecated by > > UEFI 2.7 they weren't willing to adopt it. > > It's pity, but the motivation to deprecate is "lack of use" which is > not true. That's why I would suggest to escalate this to UEFI > committee. > > > Stuart, anything else you want to add here? > > Darren, what's your opinion about this? > > P.S. I'm not against this approach (just some technical comments I > already shared), but on the other hand it would be nice to have undo > that deprecation and follow the standard in new firmwares. > Would you agree? Sure. Due to the timing of how long this will take, even if SMM communication ACPI table is undone from deprecation we may have to still support both EPS and SMM communication ACPI table though (maybe it would be order of preference).
On 7/2/2018 11:15 AM, Mario.Limonciello@dell.com wrote: >> >>> I don't believe SMM communication ACPI table has ever been implemented by >> Dell >>> on server or client BIOS. I do agree this table describes the behavior that DCDBAS >> driver >>> has used since before even UEFI BIOS pretty accurately. >> >> So, EPS table has been for ages in Dell machines? >> Can we consider it as a predecessor of that SMM communication ACPI table? > > No, EPS is new this year, specifically for server BIOS to be able to support SMM communication > when WSMT is enabled. The code tests in Stuart's patch will detect if WSMT is enabled > and if it's enabled test if EPS was defined. On server BIOS when EPS is defined dcdbas > will be able to communicate using addresses defined in EPS. > > Server BIOS will support EPS for applications using dcdbas interface and may at a later time > introduce same WMI interface as client too (but applications will need time to update so > they need to support both). > > Actually Stuart's patch will cause client BIOS that has WSMT enabled make dcdbas fail > initialization (as it should because dcdbas doesn't have a region that it can successfully > communicate). > > In client machines we moved this communication to ACPI buffer allocated by WMI, which > is why we have dell-smbios-wmi now in kernel. > > I think once some variation of Stuart's patch is merged, I'll send a follow > up patch to drop this test because it's no longer necessary: > https://github.com/torvalds/linux/blob/master/drivers/platform/x86/dell-smbios-smm.c#L106 > > >> >>> Stuart and I did discuss with server BIOS (who uses this EPS mechanism) to see if >> its possible >>> to move EPS to SMM communication ACPI table however since it's been >> deprecated by >>> UEFI 2.7 they weren't willing to adopt it. >> >> It's pity, but the motivation to deprecate is "lack of use" which is >> not true. That's why I would suggest to escalate this to UEFI >> committee. >> >>> Stuart, anything else you want to add here? >> >> Darren, what's your opinion about this? >> >> P.S. I'm not against this approach (just some technical comments I >> already shared), but on the other hand it would be nice to have undo >> that deprecation and follow the standard in new firmwares. >> Would you agree? > > Sure. Due to the timing of how long this will take, even if SMM communication > ACPI table is undone from deprecation we may have to still support both EPS > and SMM communication ACPI table though (maybe it would be order of preference). > > I have confirmation that the EPS table will be 16-byte aligned, so I can make that change. I'll send a v5 with that and the updated comment.
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c index 0bdea60c65dd..e95dc9aee2fa 100644 --- a/drivers/firmware/dcdbas.c +++ b/drivers/firmware/dcdbas.c @@ -21,6 +21,7 @@ */ #include <linux/platform_device.h> +#include <linux/acpi.h> #include <linux/dma-mapping.h> #include <linux/errno.h> #include <linux/cpu.h> @@ -41,7 +42,7 @@ #include "dcdbas.h" #define DRIVER_NAME "dcdbas" -#define DRIVER_VERSION "5.6.0-3.2" +#define DRIVER_VERSION "5.6.0-3.3" #define DRIVER_DESCRIPTION "Dell Systems Management Base Driver" static struct platform_device *dcdbas_pdev; @@ -49,19 +50,23 @@ static struct platform_device *dcdbas_pdev; static u8 *smi_data_buf; static dma_addr_t smi_data_buf_handle; static unsigned long smi_data_buf_size; +static unsigned long max_smi_data_buf_size = MAX_SMI_DATA_BUF_SIZE; static u32 smi_data_buf_phys_addr; static DEFINE_MUTEX(smi_data_lock); +static u8 *eps_buffer; static unsigned int host_control_action; static unsigned int host_control_smi_type; static unsigned int host_control_on_shutdown; +static bool wsmt_enabled; + /** * smi_data_buf_free: free SMI data buffer */ static void smi_data_buf_free(void) { - if (!smi_data_buf) + if (!smi_data_buf || wsmt_enabled) return; dev_dbg(&dcdbas_pdev->dev, "%s: phys: %x size: %lu\n", @@ -86,7 +91,7 @@ static int smi_data_buf_realloc(unsigned long size) if (smi_data_buf_size >= size) return 0; - if (size > MAX_SMI_DATA_BUF_SIZE) + if (size > max_smi_data_buf_size) return -EINVAL; /* new buffer is needed */ @@ -169,7 +174,7 @@ static ssize_t smi_data_write(struct file *filp, struct kobject *kobj, { ssize_t ret; - if ((pos + count) > MAX_SMI_DATA_BUF_SIZE) + if ((pos + count) > max_smi_data_buf_size) return -EINVAL; mutex_lock(&smi_data_lock); @@ -322,8 +327,14 @@ static ssize_t smi_request_store(struct device *dev, ret = count; break; case 1: - /* Calling Interface SMI */ - smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer); + /* Calling Interface SMI + * + * Provide physical address of command buffer field within + * the struct smi_cmd... can't use virt_to_phys on smi_cmd + * because address may be from memremap. + */ + smi_cmd->ebx = smi_data_buf_phys_addr + + offsetof(struct smi_cmd, command_buffer); ret = dcdbas_smi_request(smi_cmd); if (!ret) ret = count; @@ -482,6 +493,94 @@ static void dcdbas_host_control(void) } } +/* WSMT */ + +static u8 checksum(u8 *buffer, u8 length) +{ + u8 sum = 0; + u8 *end = buffer + length; + + while (buffer < end) + sum += *buffer++; + return sum; +} + +static inline struct smm_eps_table *check_eps_table(u8 *addr) +{ + struct smm_eps_table *eps = (struct smm_eps_table *)addr; + + if (strncmp(eps->smm_comm_buff_anchor, SMM_EPS_SIG, 4) != 0) + return NULL; + + if (checksum(addr, eps->length) != 0) + return NULL; + + return eps; +} + +static int dcdbas_check_wsmt(void) +{ + struct acpi_table_wsmt *wsmt = NULL; + struct smm_eps_table *eps = NULL; + u64 remap_size; + u8 *addr; + + acpi_get_table(ACPI_SIG_WSMT, 0, (struct acpi_table_header **)&wsmt); + if (!wsmt) + return 0; + + /* Check if WSMT ACPI table shows that protection is enabled */ + if (!(wsmt->protection_flags & ACPI_WSMT_FIXED_COMM_BUFFERS) + || !(wsmt->protection_flags + & ACPI_WSMT_COMM_BUFFER_NESTED_PTR_PROTECTION)) + return 0; + + /* Scan for EPS (entry point structure) */ + for (addr = (u8 *)__va(0xf0000); + addr < (u8 *)__va(0x100000 - sizeof(struct smm_eps_table)); + addr += 1) { + eps = check_eps_table(addr); + if (eps) + break; + } + + if (!eps) { + dev_dbg(&dcdbas_pdev->dev, "found WSMT, but no EPS found\n"); + return -ENODEV; + } + + /* + * Get physical address of buffer and map to virtual address. + * Table gives size in 4K pages, regardless of actual system page size. + */ + if (upper_32_bits(eps->smm_comm_buff_addr + 8)) { + dev_warn(&dcdbas_pdev->dev, "found WSMT, but EPS buffer address is above 4GB\n"); + return -EINVAL; + } + /* + * Limit remap size to MAX_SMI_DATA_BUF_SIZE + 8 (since the first 8 + * bytes are used for a semaphore, not the data buffer itself). + */ + remap_size = eps->num_of_4k_pages * PAGE_SIZE; + if (remap_size > MAX_SMI_DATA_BUF_SIZE + 8) + remap_size = MAX_SMI_DATA_BUF_SIZE + 8; + eps_buffer = memremap(eps->smm_comm_buff_addr, remap_size, MEMREMAP_WB); + if (!eps_buffer) { + dev_warn(&dcdbas_pdev->dev, "found WSMT, but failed to map EPS buffer\n"); + return -ENOMEM; + } + + /* First 8 bytes is for a semaphore, not part of the smi_data_buf */ + smi_data_buf_phys_addr = eps->smm_comm_buff_addr + 8; + smi_data_buf = eps_buffer + 8; + smi_data_buf_size = remap_size - 8; + max_smi_data_buf_size = smi_data_buf_size; + wsmt_enabled = true; + dev_info(&dcdbas_pdev->dev, + "WSMT found, using firmware-provided SMI buffer.\n"); + return 1; +} + /** * dcdbas_reboot_notify: handle reboot notification for host control */ @@ -548,6 +647,11 @@ static int dcdbas_probe(struct platform_device *dev) dcdbas_pdev = dev; + /* Check if ACPI WSMT table specifies protected SMI buffer address */ + error = dcdbas_check_wsmt(); + if (error < 0) + return error; + /* * BIOS SMI calls require buffer addresses be in 32-bit address space. * This is done by setting the DMA mask below. @@ -635,6 +739,8 @@ static void __exit dcdbas_exit(void) */ if (dcdbas_pdev) smi_data_buf_free(); + if (eps_buffer) + memunmap(eps_buffer); platform_device_unregister(dcdbas_pdev_reg); platform_driver_unregister(&dcdbas_driver); } diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h index ca3cb0a54ab6..52729a494b00 100644 --- a/drivers/firmware/dcdbas.h +++ b/drivers/firmware/dcdbas.h @@ -53,6 +53,7 @@ #define EXPIRED_TIMER (0) #define SMI_CMD_MAGIC (0x534D4931) +#define SMM_EPS_SIG "$SCB" #define DCDBAS_DEV_ATTR_RW(_name) \ DEVICE_ATTR(_name,0600,_name##_show,_name##_store); @@ -103,5 +104,14 @@ struct apm_cmd { int dcdbas_smi_request(struct smi_cmd *smi_cmd); +struct smm_eps_table { + char smm_comm_buff_anchor[4]; + u8 length; + u8 checksum; + u8 version; + u64 smm_comm_buff_addr; + u64 num_of_4k_pages; +} __packed; + #endif /* _DCDBAS_H_ */
If the WSMT ACPI table is present and indicates that a fixed communication buffer should be used, use the firmware-specified buffer instead of allocating a buffer in memory for communications between the dcdbas driver and firmare. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> --- v2 Bumped driver version to 5.6.0-3.3 v3 Removed dependency on ACPI in Kconfig Moved the added #include to be in alphabetical order Added comments in smi_request_store() Simplified checksum code Changed loop searching 0xf0000 to be more readable Reworked calculation of remap_size & smi_data_buf_size drivers/firmware/dcdbas.c | 118 +++++++++++++++++++++++++++++++++++++++++++--- drivers/firmware/dcdbas.h | 10 ++++ 2 files changed, 122 insertions(+), 6 deletions(-)