Re: [Freeipmi-devel] Submitting patches

On Thu, Sep 24, 2015 at 5:25 PM, Albert Chu <address@hidden> wrote:

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

From:	Newell Jensen
Subject:	Re: [Freeipmi-devel] Submitting patches
Date:	Tue, 29 Sep 2015 14:08:45 -0700