[Mesa-dev] [PATCH 2/2] anv: Add the pci_id into the shader cache UUID

[Chad Versace]

On Mon 27 Feb 2017, Jason Ekstrand wrote:
> On Mon, Feb 27, 2017 at 9:38 AM, Chad Versace <chadversary at chromium.org>
> wrote:
> 
> > On Fri 24 Feb 2017, Jason Ekstrand wrote:
> > > This prevents a user from using a cache created on one hardware
> > > generation on a different one.  Of course, with Intel hardware, this
> > > requires moving their drive from one machine to another but it's still
> > > possible and we should prevent it.
> >
> > Or if you rsync stuff between test machines.
> >
> > Or if your test machines share stuff over NFS. (Mine do).
> >
> > Or...
> >
> > It's easy to hit this bug without a screwdriver ;)
> >
> > > ---
> > >  src/intel/vulkan/anv_device.c | 20 +++++++++++++++-----
> > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > > index 38def35..5168331 100644
> > > --- a/src/intel/vulkan/anv_device.c
> > > +++ b/src/intel/vulkan/anv_device.c
> > > @@ -32,6 +32,7 @@
> > >  #include "util/strtod.h"
> > >  #include "util/debug.h"
> > >  #include "util/build_id.h"
> > > +#include "util/mesa-sha1.h"
> > >  #include "util/vk_util.h"
> > >
> > >  #include "genxml/gen7_pack.h"
> > > @@ -53,17 +54,26 @@ compiler_perf_log(void *data, const char *fmt, ...)
> > >  }
> > >
> > >  static bool
> > > -anv_device_get_cache_uuid(void *uuid)
> > > +anv_device_get_cache_uuid(void *uuid, uint16_t pci_id)
> > >  {
> > >     const struct build_id_note *note = build_id_find_nhdr("libvulkan_
> > intel.so");
> > >     if (!note)
> > >        return false;
> > >
> > > -   unsigned len = build_id_length(note);
> > > -   if (len < VK_UUID_SIZE)
> > > +   unsigned build_id_len = build_id_length(note);
> > > +   if (build_id_len < VK_UUID_SIZE)
> > >        return false;
> >
> > Below, you're no longer copying the build_id into the pipelineCacheUUID.
> > So checking `build_id_len >= VK_UUID_SIZE` is unneeded.
> >
> 
> Yes and no.  I want to be sure that it's a SHA1 hash and not a timestamp.
> Maybe we should check for >= 20 instead?

That sounds good.

> > > -   memcpy(uuid, build_id_data(note), VK_UUID_SIZE);
> > > +   uint8_t sha1[20];
> > > +   struct mesa_sha1 *sha1_ctx = _mesa_sha1_init();
> > > +   if (sha1_ctx == NULL)
> > > +      return false;
> > > +
> > > +   _mesa_sha1_update(sha1_ctx, build_id_data(note), build_id_len);
> > > +   _mesa_sha1_update(sha1_ctx, &pci_id, sizeof(pci_id));
> > > +   _mesa_sha1_final(sha1_ctx, sha1);
> > > +
> > > +   memcpy(uuid, sha1, VK_UUID_SIZE);
> >
> > What really needs checking is `ARRAY_LEN(sha1) >= VK_UUID_SIZE`. That
> > could be done as a static assert.
> >
> 
> sure.

I was almost right. The check should be `VK_UUID_SIZE <= sizeof(sha1)`.
memcpy cares about the size in bytes, not size in elems.
> 
> 
> > >     return true;
> > >  }
> > >
> > > @@ -148,7 +158,7 @@ anv_physical_device_init(struct anv_physical_device
> > *device,
> > >        goto fail;
> > >     }
> > >
> > > -   if (!anv_device_get_cache_uuid(device->uuid)) {
> > > +   if (!anv_device_get_cache_uuid(device->uuid, device->chipset_id)) {
> > >        result = vk_errorf(VK_ERROR_INITIALIZATION_FAILED,
> > >                           "cannot generate UUID");
> > >        goto fail;
> > > --
> > > 2.5.0.400.gff86faf
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev