Re: [Freeipmi-devel] Submitting patches

[Dann Frazier]

Thanks Al!

However, looking closer at the code, I wonder if the proposed fix
isn't too heavy handed. It looks like there is support for locating
the smbios table address from the efi systable in
ipmi-locate-dmidecode.c. I don't know of any reason accessing the
memory region found here would be harmful (assuming firmware isn't
lying!), and I'd hate to break support for systems that support this
interface.

The real problem is falling back to the legacy address (0xf0000) -
indeed, that's what Newell's strace output shows we are doing when the
fault is triggered.

Newell - can you try just disabling that?

On Thu, Sep 24, 2015 at 2:40 PM, Albert Chu <address@hidden> wrote:
> Thanks, applied.
>
> Al
>
> On Thu, 2015-09-24 at 12:28 -0700, Newell Jensen wrote:
>> Al,
>>
>>
>> Here is my patch (which I have also attached):
>>
>>
>> Index: ipmi-locate/ipmi-locate.c
>> ===================================================================
>> --- ipmi-locate/ipmi-locate.c (revision 10210)
>> +++ ipmi-locate/ipmi-locate.c (working copy)
>> @@ -537,9 +537,13 @@
>>        exit (EXIT_FAILURE);
>>      }
>>
>> +#ifndef __arm__
>> +#ifndef __aarch64__
>>    dmidecode_probe_display (ctx);
>>    smbios_probe_display (ctx);
>>    acpi_probe_display (ctx);
>> +#endif
>> +#endif
>>    pci_probe_display (ctx);
>>    if (cmd_args.defaults)
>>      defaults_display (ctx);
>>
>>
>>
>> On Thu, Sep 24, 2015 at 10:08 AM, Dann Frazier
>> <address@hidden> wrote:
>>         Hi Al,
>>          There's no urgency for a formal release. We're past feature
>>         freeze
>>         for 15.10, so we couldn't pull in a new upstream release
>>         anyway.
>>         However, we can cherry pick it as a bug fix. Though, as Newell
>>         just
>>         mentioned to me, we probably want to rework the patch so apply
>>         to
>>         ARM32 as well.
>>
>>         On Thu, Sep 24, 2015 at 10:59 AM, Albert Chu <address@hidden>
>>         wrote:
>>         > Hi Dann,
>>         >
>>         > Thanks for the additional info, I wasn't aware of it.
>>         >
>>         > In that case, I think we can use the patch.
>>         >
>>         > What's the urgency on a new release w/ this patch?
>>         >
>>         > Al
>>         >
>>         > On Thu, 2015-09-24 at 10:28 -0600, Dann Frazier wrote:
>>         >> On Thu, Sep 24, 2015 at 9:29 AM, Newell Jensen
>>         <address@hidden> wrote:
>>         >> > Adding in Dann Frazier to thread
>>         >>
>>         >> Thanks Newell!
>>         >>
>>         >> > On Wed, Sep 23, 2015 at 1:50 PM, Albert Chu
>>         <address@hidden> wrote:
>>         >> >>
>>         >> >> Hi Newell,
>>         >> >>
>>         >> >> Hmmm, I'm not sure if your patch is the right approach.
>>         While there may
>>         >> >> be a problem w/ /dev/mem on your system, it may not be
>>         something that
>>         >> >> exists on all systems.  Or if it's a bug, it may be
>>         fixed in the future.
>>         >> >> So just removing the probes on arm64 may not be the best
>>         choice.
>>         >>
>>         >> While SMBIOS tables will certainly exist on ARM systems,
>>         they should
>>         >> be described properly by firmware (e.g. EFI) and exposed by
>>         the kernel
>>         >> at /sys/firmware/dmi/tables/. My understanding is that
>>         poking in
>>         >> /dev/mem for the table at legacy addresses on ARM will
>>         always be bad
>>         >> because IO memory is memory mapped. This has been a problem
>>         for every
>>         >> ARM server platform I've seen (that is, every one supported
>>         by
>>         >> Ubuntu).
>>         >>
>>         >> lshw had this same issue, and we resolved it by dropping
>>         the /dev/mem
>>         >> probing for ARM (and other platforms), while still
>>         retaining
>>         >> non-legacy SMBIOS access:
>>         >>   http://www.ezix.org/project/ticket/628
>>         >>
>>         >> >> Perhaps a better option would be to create a set of
>>         options in
>>         >> >> ipmi-locate to limit what probes to do?  That way (if
>>         you're scripting
>>         >> >> this), you can avoid the probing of known problem areas?
>>         >> >>
>>         >> >> It probably wouldn't be hard to add a bunch of the
>>         options.  Do you
>>         >> >> think that'd suffice?
>>         >>
>>         >> That would be sufficient for the specific use case Newell
>>         and I are
>>         >> working in the MAAS project - that is, we could pass
>>         different
>>         >> parameters depending on the architecture. However, it would
>>         still
>>         >> remain an issue for other users of ipmi-locate, who would
>>         need to know
>>         >> that a special argument needs to be passed if they are on
>>         ARM.
>>         >>
>>         >>  -dann
>>         > --
>>         > Albert Chu
>>         > address@hidden
>>         > Computer Scientist
>>         > High Performance Systems Division
>>         > Lawrence Livermore National Laboratory
>>         >
>>         >
>>
>>
>>
> --
> Albert Chu
> address@hidden
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
>
>