Re: [Freeipmi-devel] Submitting patches

[Albert Chu]

I have never run on an arm machine (let alone FreeIPMI on an arm
machine), so I'm sort of going off of your guy's testing +
judgement :-)  

Just let me know what you think would be best.

Al

On Thu, 2015-09-24 at 17:00 -0600, Dann Frazier wrote:
> 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
> >
> >
-- 
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory