On Friday, 2019-05-24 07:49:37 +0300, Tapani Pälli wrote: > > > On 5/23/19 8:22 PM, Sumit Semwal wrote: > > Hi Eric, > > > > On Thu, 23 May 2019 at 20:25, Eric Engestrom <eric.engest...@intel.com> > > wrote: > > > > > > On Thursday, 2019-05-23 08:34:40 +0300, Tapani Pälli wrote: > > > > Hi; > > > > > > > > On 5/22/19 9:20 PM, Alistair Strachan wrote: > > > > > On Tue, May 21, 2019 at 10:10 PM Tapani Pälli > > > > > <tapani.pa...@intel.com> wrote: > > > > > > > > > > > > > > > > > > On 5/21/19 4:53 PM, Sumit Semwal wrote: > > > > > > > Hello everyone, > > > > > > > > > > > > > > First up, my apologies on not being able to respond earlier; > > > > > > > secondly, > > > > > > > thanks very much for your review. > > > > > > > > > > > > > > On Wed, 15 May 2019 at 19:27, Emil Velikov > > > > > > > <emil.l.veli...@gmail.com> wrote: > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > On Tue, 14 May 2019 at 08:18, Tapani Pälli > > > > > > > > <tapani.pa...@intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On 5/13/19 6:52 PM, Haehnle, Nicolai wrote: > > > > > > > > > > This approach seems entirely incompatible with > > > > > > > > > > si_debug_options.h, and > > > > > > > > > > will be an absolute maintenance nightmare going forward for > > > > > > > > > > adding / > > > > > > > > > > removing options, because you're introducing a second > > > > > > > > > > location where > > > > > > > > > > options are defined. > > > > > > > > > > > > > > > > > > > > Quite frankly, this seems like a terrible idea as-is. > > > > > > > > > > > > > > > > > > > > If you really can't use XML for whatever reason, then > > > > > > > > > > please find some > > > > > > > > > > way of deriving both the tables here and the XML from the > > > > > > > > > > same single > > > > > > > > > > source of truth. > > > > > > > > > > > > > > > > > > I was looking at this yesterday and came up with same > > > > > > > > > conclusion. We > > > > > > > > > should have the options in one place. Currently libexpat is > > > > > > > > > statically > > > > > > > > > linked with Android >=O, maybe for such restricted > > > > > > > > > environments we could > > > > > > > > > just inline the xml as is at compile time and parse that > > > > > > > > > later or > > > > > > > > > alternatively (maybe cleaner) parse and generate default > > > > > > > > > option cache > > > > > > > > > already during compilation? > > > > > > > > > > > > > > > > > I realise that jumping the "me too" train does not help much, > > > > > > > > so here > > > > > > > > are some alternative ideas. > > > > > > > > > > > > > > > > How about we first distil the reasons why this is a problem and > > > > > > > > what > > > > > > > > kind. Then explore independent solution for each one - as-is > > > > > > > > this > > > > > > > > seems like a one-size-fits-all approach. > > > > > > > I totally agree that this seems like a rudimentary / ugly > > > > > > > approach, > > > > > > > and we can definitely improve upon it once the reasons are > > > > > > > discussed. > > > > > > > > > > > > > > > Some examples: > > > > > > > > - XML file may be inaccessible - the in-driver defaults > > > > > > > > should work(tm) > > > > > > > > Yes there are some app specific ones, yet neither(?) of these > > > > > > > > apps is > > > > > > > > present on Android > > > > > > > > - libexpat is not available, but libFOO is - investigate > > > > > > > > into a compat wrapper > > > > > > > > - cannot use external libraries (libexpat or equivalent) - > > > > > > > > static link > > > > > > > > > > > > > > > > > > > > > > AFAIU, in the Android space, it is a combination of some of the > > > > > > > above: > > > > > > > a. current Android doesn't allow GL drivers to access config files > > > > > > > from the vendor partition: this is enforced via selinux policy. > > > > > > > > > > > > For point a, vendors can (and should) define their own policy rules > > > > > > regarding what file access and ioctl's are required. This is done by > > > > > > setting BOARD_SEPOLICY_DIRS in BoardConfig.mk file. That directory > > > > > > then > > > > > > contains all the necessary rules required for the particular driver > > > > > > to > > > > > > work. As example: > > > > > > > > > > > > BOARD_SEPOLICY_DIRS += device/samsung/tuna/sepolicy > > > > > > > > > > > > If a vendor wanted to use xml based configuration for Mesa it > > > > > > should be > > > > > > possible by setting a sepolicy rule so that particular library is > > > > > > able > > > > > > to access such file. Looking at Android Celadon selinux files, > > > > > > 'file_contexts' is probably the place to do it. > > > > > > > > > > The EGL/GLES driver stack is a special kind of HAL in Android > > > > > (same-process HAL) so we have to be very careful about expanding the > > > > > sepolicy rules to work around unnecessary file accesses. We also have > > > > > strict sepolicy "neverallows" for untrusted apps (the processes this > > > > > same-process HAL might be loaded into). I strongly disagree with your > > > > > suggestion here. > > > > > > > > > > From an Android PoV, the EGL/GLES drivers should minimize their > > > > > dependencies so as to not affect other NDK libraries loaded into the > > > > > app processes. They should also limit interactions with the rest of > > > > > the system, such as opening configuration files. It's clear that Mesa > > > > > can work just fine without reading a configuration file, and that the > > > > > use case of opening a configuration file should only be necessary for > > > > > development and bring-up. > > > > > > > > > > The discussion so far on this thread seems to be optimizing for Mesa's > > > > > configuration file, rather than for security and file size. On an > > > > > embedded platform such as Android, in cases where Mesa might ship in a > > > > > production configuration, there should be no configuration file, and > > > > > we would want vendors to optimize for security and file size. > > > > > > > > > > My opinion is that we need Sumit's changes, or something like them. > > > > > Pulling in libexpat just to build internal configuration state from a > > > > > built-in XML file seems quite over-engineered. > > > > > > > > > > That said, I agree with other feedback on this thread that it should > > > > > be possible to derive the baked configuration from the same source of > > > > > truth (possibly an XML file) as another platform which might not have > > > > > a baked configuration. > > > > > > > > > > > > b. Also, they had some concerns around how safe libexpat is > > > > > > > vis-a-vis > > > > > > > dual-loading, and that's where the concern around static linking > > > > > > > came > > > > > > > from. > > > > > > > > > > > > > > Alistair, could you please correct me if I am wrong, and if there > > > > > > > are > > > > > > > additional details on the need of this? > > > > > > > > > > > > > > > > > The concern is basically that libexpat might be "dual loaded" - by the > > > > > linker namespace for the sphal, and that of the app itself - and that > > > > > there might be global data (like pthread keys, etc.) that could > > > > > conflict. The versions of the library needn't be the same, either; the > > > > > app and the EGL/GLES stack might be using different versions > > > > > (static+static, dynamic+static, etc.) > > > > > > > > > > My concern is more basic though - libexpat is a non-trivial amount of > > > > > code, and Mesa only uses it to parse a configuration file that a > > > > > production device won't have or want. It won't be allowed to by system > > > > > sepolicy. So, we are increasing the size of the graphics stack just to > > > > > parse an internally-baked XML file, which seems like a very reasonable > > > > > dependency to work on optimizing out. > > > > > > > > With selinux suggestion I was mainly trying to balance between having 2 > > > > separate solutions against 1. *Ideally* all systems would use same code > > > > for > > > > configuration mgmt so that we wouldn't need to support and maintain 2, > > > > that > > > > comes with a cost. > > > > > > > > I don't strongly oppose changes though and maybe some kind of 'default > > > > config generation during compile time' would bring this closer to the > > > > goal. > > > > > > Just FYI, I've started working on this; I should have a branch usable by > > > tomorrow, but the idea is to use a python script to turn the drirc xml > > > into a c source containing the build-time values to be used by xmlconfig.c > > > if the files can't be read (for whatever reason), and then another > > > commit making expat optional at build time. > > > > > > I think this should satisfy everyone's desires? > > So, I am assuming you mean the altxmlconfig.c that's part of this > > patchset, rather than the existing xmlconfig.c which depends on expat. > > If yes, then I feel this could be a good solution.
Actually, my idea was to preprocess the expat stuff out of xmlconfig.c > > > > Yeah, I think combination of these 2 sounds good. Vendors can then patch the > default values in xml when building. As example Android Celadon has been > shipping with custom drirc in the past, main reason was that s3tc formats > were not enabled by default back then. Other usecases would be to implement > and use game workarounds etc. Agreed. --- It's not finished by any stretch, but I'm going on holiday for a week so here's a branch with what I've done so far: https://gitlab.freedesktop.org/eric/mesa/commits/xml-fallback The last two commits are the interesting ones: the second to last adds the python script and wires the build-time values loading, and the last one makes expat optional when possible - some stuff still has a hard dependency on expat, namely intel drivers and opencl, but if you're not compiling those you can have an expat-free mesa :D _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
