[Skiboot] [RFC PATCH v3 2/6] Add ultravisor support in OPAL

[Ryan Grimm]

Oliver,

On Wed, 2020-01-29 at 01:24 +1100, Oliver O'Halloran wrote:
> 
> Is ultra.tcl supposed to be included in this patch?

It resides in the uv tree but, good point, I moved it to
external/mambo/skiboot.tcl.

We can now boot the skiboot way by setting environment variables.

In my next post, v4, I included the exit_uv_mode function and this can
be tested using skiboot.lid as the ultra.lid.  For example:

export SKIBOOT=skiboot.lid
export ULTRA_LID=skiboot.lid
export SKIBOOT_ZIMAGE=~/obj/linux-powernv.obj/vmlinux
export ULTRA_ENTRY=0x"$(grep exit_uv_mode skiboot.map|cut -f1 -d " ")"
export MAMBO_ENABLE_ULTRA=1
export MAMBO_ENABLE_SMF=1

~/code/mambo/run/p9/run_cmdline -f external/mambo/skiboot.tcl -rl

Will get to user space.  It's a little funky with that grep foo, but it
works.

I added a uv-entry dt property so we can offset into any image for
testing.

We could add a little piece of assembly for testing as well if want to.

> 
> > For BML, the BML script parses secure memory from the Cronus config
> > file
> > and creates the secure-memory-ranges proprty.
> 
> Is this still true? Looks like secure-memory-ranges went away with
> the
> switch to secure-memory@<addr> nodes.
> 

Yes, tsecure-memory-ranges is gone.  Fixed.

> > 
> > A new type of system call called the ultra call is used to get the
> > services of the ultravisor. This ultracall support is needed in
> > skiboot
> > to access the xscoms which are in the secure memory area.
> 
> Move the ultra-call stuff into 4/6 where it's actually used. 
> 

k

> 
> 
> Too big. At the very least the HDAT parsing and the ucall support can
> go into seperate patches. The memory region changes probably be in a
> seperate patch too.
> 


K, with all the squashing and reworking I made it too big.  I split it
up into discrete pieces for v4 and if I went too far we can squash
without too much trouble.

> > 
> > +	ld	%r0,STACK_LR(%r1)
> > +	mtlr	%r0
> > +	addi	%r1,%r1,STACK_FRAMESIZE
> > +	blr				/* return r3 = status */
> 
> put this stuff in asm/misc.S unless there's a compelling reason it
> needs to be in head.S
> 

k, done.  I have a procedure I posted to turning off SMF that I put in
there as well.

> > diff --git a/core/flash.c b/core/flash.c
> > index de748641..247aec5c 100644
> > --- a/core/flash.c
> > +++ b/core/flash.c
> > @@ -45,6 +45,7 @@ static struct {
> >  	{ RESOURCE_ID_INITRAMFS,RESOURCE_SUBID_NONE,		"RO
> > OTFS" },
> >  	{ RESOURCE_ID_CAPP,	RESOURCE_SUBID_SUPPORTED,	"CAPP" },
> >  	{ RESOURCE_ID_IMA_CATALOG,  RESOURCE_SUBID_SUPPORTED,	"IM
> > A_CATALOG" },
> > +	{ RESOURCE_ID_UV_IMAGE, RESOURCE_SUBID_NONE,		"UV
> > ISOR" },
> >  	{ RESOURCE_ID_VERSION,	RESOURCE_SUBID_NONE,		"VE
> > RSION" },
> >  	{ RESOURCE_ID_KERNEL_FW,	RESOURCE_SUBID_NONE,		"BO
> > OTKERNFW" },
> >  };
> > diff --git a/core/init.c b/core/init.c
> > index 339462e5..4160e68d 100644
> > --- a/core/init.c
> > +++ b/core/init.c
> > @@ -46,6 +46,7 @@
> >  #include <debug_descriptor.h>
> >  #include <occ.h>
> >  #include <opal-dump.h>
> > +#include <ultravisor.h>
> >  
> >  enum proc_gen proc_gen;
> >  unsigned int pcie_max_link_speed;
> > @@ -558,6 +559,8 @@ void __noreturn load_and_boot_kernel(bool
> > is_reboot)
> >  
> >  	trustedboot_exit_boot_services();
> >  
> > +	start_ultravisor();
> > +
> >  	ipmi_set_fw_progress_sensor(IPMI_FW_OS_BOOT);
> >  
> >  
> > @@ -1296,6 +1299,11 @@ void __noreturn __nomcount
> > main_cpu_entry(const void *fdt)
> >  	pci_nvram_init();
> >  
> >  	preload_capp_ucode();
> > +
> > +	/* preload and decompress ultravisor image */
> > +	uv_preload_image();
> > +	uv_decompress_image();
> 
> What's the point of pre-loading it if we're just going to wait around
> until it's loaded so that we can start decompressing it?
> 

Moved uv_decompress_image to init_uv. 

> > +
> >  void *__local_alloc(unsigned int chip_id, size_t size, size_t
> > align,
> >  		    const char *location)
> >  {
> 
> Crack this into a seperate patch and add a unit test in
> core/test/run-
> mem_region.c
> 

k, that makes a lot more sense.  I'm posting v4 without the unit test
but I'll post as a follow up.  I need to learn more about how to do
unit tests properly.

> > +#define MS_ATTR_PEF			(PPC_BIT32(23))
> > +#define UV_SECURE_MEM_BIT		(PPC_BIT(15))
> 
> doesn't need parens

k

> >  		/*
> >  		 * Workaround broken HDAT reserve regions which are
> > -		 * bigger than 512MB
> > +		 * bigger than 512MB and not secure memory
> >  		 */
> > -		if ((end_addr - start_addr) > 0x20000000) {
> > +		if (((end_addr - start_addr) > 0x20000000) &&
> > +			!(start_addr & UV_SECURE_MEM_BIT)) {
> >  			prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve:
> > too big\n");
> >  			continue;
> >  		}
> 
> I think you can drop this hunk since secure memory isn't in the
> reserved memory ranges any more. The definition of UV_SECURE_MEM_BIT
> in
> hdata/memory.c can go too since it's only used here.
> 

I talk about this in the doc:

Regions of secure memory will be reserved by hostboot such as OCC,
HOMER, and SBE. Skiboot will use the existing reserve infrastructure to
reserve them. For example:

ibm,HCODE at 100fffcaf0000
ibm,OCC at 100fffcdd0000
ibm,RINGOVD at 100fffcae0000

So we need to not ignore those reserves.  If the code is awkard, I can
fix but we need it.


> > +
> > +static char *uv_image = NULL;
> > +static size_t uv_image_size;
> > +struct xz_decompress *uv_xz = NULL;
> > +static struct uv_opal *uv_opal;
> > +
> > +const char * wrap_key_prop_str[] = {
> 
> no space between the * and the variable name.
> 

k

> > +	"wrapping-key-passwd",
> > +	"wrapping-key-publicname",
> > +	"wrapping-key-policy-a",
> > +	"wrapping-key-policy-b",
> > +	NULL
> > +};
> > +
> > +static void create_uv_fw_node(struct dt_node *uv_node, uint64_t
> > addr, uint32_t size) {
> 
> brace goes on a new line.
> 

k

> > +	struct dt_node *uv_fw_node;
> > +	char fw_name[64];
> > +
> > +	snprintf(fw_name, 64, "firmware@%llx", addr);
> > +	uv_fw_node = dt_new_check(uv_node, fw_name);
> > +	dt_add_property_string(uv_fw_node, "compatible", "ibm,uv-
> > firmware");
> > +	dt_add_property_cells(uv_fw_node, "reg", addr >> 32, addr &
> > 0xfffffff, size);
> 
> Hmm, in the event the uv_fw_node already exists this is probably
> going
> to blow up on since the properties added below will already exist.
> 
> Also, the having #address-cells = 2 and #size-cells = 1 makes it
> awkward to work with since u64 helpers. There's no point in saving
> the
> extra four bytes so bump #size-cells to 2 and use
> dt_add_property_u64s()
> 

This is fixed to use size = 2.  I also see the light and use
dt_get_address where appropriate.

I put add_uv in hdata/spira.c, I think that's a good place.  So the
device tree entry with reg is created and hdat parsing time and entries
are added later but not modified.

> > +}
> > +
> > +static uint64_t find_secure_mem(void)
> > +{
> > +	struct dt_node *node;
> > +	const struct dt_property *reg;
> > +	uint64_t secure_mem;
> 
> declarations should be in reverse christmas tree order

k, this looks nice and now I can't unsee reverse christmas tree

> 
> > +	dt_for_each_compatible(dt_root, node, "ibm,secure-memory")
> > +		if (dt_get_chip_id(node) == 0)
> > +			break;
> > +
> > +	reg = dt_find_property(node, "reg");
> > +	secure_mem = dt_get_number(reg->prop, 2);
> 
> Use dt_get_address()
> 

I admit I didn't know how to use dt_get_address, I found it confusing
with the name "reg" but, I get it now, I have gone through the code and
fixed up places that don't use it.

> > +	return secure_mem;
> > +}
> > +
> > +static struct dt_node *add_uv_dt_node(void)
> > +{
> > +	struct dt_node *uv_node;
> > +	uint64_t uv_fw_start;
> > +
> > +	uv_node = dt_new_check(dt_root, "ibm,ultravisor");
> > +	dt_add_property_string(uv_node, "compatible",
> > "ibm,ultravisor");
> > +	dt_add_property_cells(uv_node, "#address-cells", 2);
> > +	dt_add_property_cells(uv_node, "#size-cells", 1);
> 
> as mentioned above, make it 2
> 
> > +	uv_fw_start = find_secure_mem();
> > +	create_uv_fw_node(uv_node, uv_fw_start, UV_LOAD_MAX_SIZE);
> > +	return uv_node;
> > +}
> > +
> > +static struct dt_node *find_tpm_sim_node(void)
> > +{
> > +	struct dt_node *tpm_sim_node;
> > +
> > +	tpm_sim_node = dt_find_compatible_node(dt_root, NULL,
> > "uv,tpm_sim");
> > +	if (!tpm_sim_node) {
> > +		prlog(PR_INFO, "uv,tpm_sim compatible node not
> > found\n");
> > +		return NULL;
> > +	}
> > +
> > +	return tpm_sim_node;
> > +}
> 
> Fold this into the one place it's called from.
> 

k.  

> > +
> > +/* create ibm,ultravisor node if it does not exist. Mambo and BML
> > pass it in.
> > + * For HB, hdata/memory.c code parses HDAT and creates secure-
> > memory nodes and
> > + * find_secure_mem will return the reg property */
> 
> /*
>  * Use this style for block comments.
>  */
> 

k.

> > +static bool get_uv_fw_region(uint64_t *addr, uint32_t *size)
> > +{
> > +	struct dt_node *node;
> > +	const struct dt_property *reg;
> > +
> > +	node = get_uv_fw_node();
> > +
> > +	reg = dt_find_property(node, "reg");
> > +	*addr = dt_get_number(reg->prop, 2);
> > +	*size = dt_get_number(reg->prop + sizeof(u64), 1);
> 
> dt_get_address()

Doubly done.

> 
> > +
> > +	return true;
> > +}
> > +
> > +static void update_uv_fw_node(uint64_t addr, uint32_t size)
> > +{
> > +	struct dt_node *uv_node, *uv_fw_node;
> > +
> > +	if (!(uv_fw_node = dt_find_compatible_node(dt_root, NULL,
> > "ibm,uv-firmware"))) {
> 
> Don't do assignments inside a conditional. It usually winds up
> looking
> awkward, especially if it's split across multiple lines, and I think
> this one should be. Seperating the assignment and test usually ends
> up
> looking cleaner anyway:
> 
> uv_fw_node = dt_find_compatible_node(dt_root, NULL, "ibm,uv-
> firmware");
> if (!uv_fw_node) {
> 	/* blah */
> }
> 
> It's even 80 cols compliant.
> 

k, I went through and fixed all of these.

> > +		prerror("UV: No ibm,uv-firmware node found\n");
> > +		return;
> > +	}
> > +
> > +	uv_node = uv_fw_node->parent;
> > +	dt_free(uv_fw_node);
> > +	create_uv_fw_node(uv_node, addr, size);
> 
> If you find yourself hacking the DT at runtime it's usually a sign
> that
> you need to rethink what you're doing. The parts of the DT that are
> provided to skiboot in the input DT (or HDAT) should be treated as
> read-only by the rest of skiboot since a) We don't do any refcounting
> of DT nodes, so we have no idea what else is using it. And b) working
> out where a DT node / property is generated is relatively
> straightforward if there is a single producer. It pretty rapidly
> becomes a PATA when you start having to think about what random hacks
> are applied along the way so I strongly prefer that we don't.
> 
> We have done some runtime modifications to the DT in the past, but
> that's usually because we need to work around bugs in an input DT
> that
> we don't have control over (i.e P8 hostboot). That reasoning doesn't
> really apply since we've got full control over how the input DT is
> generated (BML or skiboot.tcl).
> 

K, as mentioned above, I do it now at hdat parsing time and don't
modify entries, only add.

> Further comments below where this is called.
> 
> > +static int add_wrapping_key_mambo(void *fdt)
> 
> I'd rather mambo specific code went into platforms/mambo/*.c
> 

k, moved to platforms/mambo/uv.c.

> > +{
> > +	struct dt_node *tpm_sim_node;
> > +	const struct dt_property *property = NULL;
> > +
> > +	if ((tpm_sim_node = find_tpm_sim_node())) {
> > +		int i;
> > +
> > +		fdt_begin_node(fdt, "ibm,uv-tpm");
> > +		fdt_property_string(fdt, "compatible", "ibm,uv-tpm");
> > +
> > +		for (i = 0; wrap_key_prop_str[i] != NULL; i++) {
> > +			property = dt_find_property(tpm_sim_node,
> > +					wrap_key_prop_str[i]);
> > +			if (property) {
> > +				fdt_property(fdt, wrap_key_prop_str[i],
> > +					property->prop,
> > +					property->len);
> > +			}
> > +		}
> 
> Move the definition of wrap_key_prop_str[] to above this function or 
> make it a local variable.

k

> 
> > +
> > +		fdt_end_node(fdt);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_add_wrapping_key(void *fdt)
> > +{
> > +	return add_wrapping_key_mambo(fdt);
> 
> what's this supposed to be doing?
> 

The wrapping key stuff is incomplete, so, for v4 I just have the mambo
version.

> > +}
> > +
> > +static int create_dtb_uv(void *uv_fdt)
> > +{
> > +	if (fdt_create(uv_fdt, UV_FDT_MAX_SIZE)) {
> > +		prerror("UV: Failed to create uv_fdt\n");
> > +		return 1;
> > +	}
> > +
> > +	fdt_finish_reservemap(uv_fdt);
> > +	fdt_begin_node(uv_fdt, "");
> > +	fdt_property_string(uv_fdt, "description", "Ultravisor fdt");
> > +	fdt_begin_node(uv_fdt, "ibm,uv-fdt");
> > +	fdt_property_string(uv_fdt, "compatible", "ibm,uv-fdt");
> > +	if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS) {
> > +		if (add_wrapping_key_mambo(uv_fdt))
> > +			prlog(PR_ERR, "Failed to add the mambo wrapping
> > key"
> > +					" to dt\n");
> > +	} else
> > +		if (fdt_add_wrapping_key(uv_fdt))
> > +			prlog(PR_ERR, "Failed to add the wrapping key"
> > +					" to dt\n");
> 
> Don't split prlog()s, use prerror(), and if any arm of an if-else
> block
> has braces then every arm should.
> 

k

> > +	fdt_end_node(uv_fdt);
> > +	fdt_end_node(uv_fdt);
> > +	fdt_finish(uv_fdt);
> > +
> > +	return OPAL_SUCCESS;
> > +}
> > +
> > +static void cpu_start_ultravisor(void *data)
> > +{
> > +	struct uv_opal *ptr = (struct uv_opal *)data;
> 
> don't cast void pointers.

k, uv_opal gone anyway
> 
> > +	start_uv(ptr->uv_base_addr, ptr);
> > +}
> > +
> > +int start_ultravisor(void)
> > +{
> > +	struct cpu_thread *cpu;
> > +	struct cpu_job **jobs;
> > +	uint64_t uv_fdt_addr;
> > +	uint64_t addr;
> > +	uint32_t size;
> > +	struct dt_node *uv_fw_node;
> > +	int i = 0;
> > +
> > +	/* init_uv should have made the ibm,ultravisor node by now so
> > don't
> > +	 * start if something went wrong */
> 
> fix the comment style

K, I went through and fixed all these

> 
> > +	jobs = zalloc(sizeof(struct cpu_job*) * cpu_max_pir);
> 
> space between cpu_job and *

k

> 
> > +
> > +	for_each_available_cpu(cpu) {
> > +		if (cpu == this_cpu())
> > +			continue;
> > +		jobs[i++] = cpu_queue_job(cpu, "start_ultravisor",
> > +					cpu_start_ultravisor, (void
> > *)uv_opal);
> > +	}
> > +
> > +	cpu_start_ultravisor((void *)uv_opal);
> > +
> > +	while (i > 0)
> > +		cpu_wait_job(jobs[--i], true);
> > +
> > +	free(jobs);
> > +
> > +	return OPAL_SUCCESS;
> > +}
> > +
> > +static bool alloc_uv(void)
> > +{
> > +	struct proc_chip *chip = next_chip(NULL);
> > +
> > +	uv_image_size = MAX_COMPRESSED_UV_IMAGE_SIZE;
> 
> Shouldn't this be probed based on the size of the UV partition?


I don't know how to do this but it sounds like a good idea!

> 
> > +	if (!(uv_image = local_alloc(chip->id, uv_image_size,
> > uv_image_size)))
> > +		return false;
> > +	memset(uv_image, 0, uv_image_size);
> > +	return true;
> > +}
> 
> Is uv_image being freed anywhere?
> 

It should be, in v4 it is.

> > +
> > +/* For HB, check for uv_xz != NULL and wait for decompress.  UV
> > should have
> > + * been loaded from PNOR or FSP and decompressed earlier via
> > uv_preload_image
> > + * and uv_decompress_image.  The secure location of the UV
> > provided by those
> > + * functions is in the xz struct.
> > + *
> > + * If uv_xz == NULL, Mambo and BML are expected to provide the
> > source address
> > + * in the device tree.  Cronus can't use putmemdma to secure
> > memory (or secure
> > + * memory might be inaccessible to Cronus) so Skiboot copies the
> > UV image from
> > + * insecure memory to secure and updates the device tree.
> + */
> 
> Put this documentation below around where the if (uv_xz) check is 
> done. It's not helping anyone up here.
> 

k, I think the code is a little clearer now so I don't have a comment,
as this is described in doc/opal-uv-abi.rst.

> > +void init_uv()
> 
> void init_uv(void)
> 
> > +{
> > +	struct dt_node *uv_fw_node;
> > +	uint64_t uv_dt_src, uv_dt_reg, sys_fdt_addr;
> > +	uint32_t uv_fw_sz;
> > +
> > +	prlog(PR_DEBUG, "UV: Init starting\n");
> > +
> > +	if (!is_msr_bit_set(MSR_S)) {
> > +		prlog(PR_DEBUG, "UV: S bit not set\n");
> > +		return;
> > +	}
> > +
> > +	if (!(uv_fw_node = get_uv_fw_node())) {
> > +		prerror("UV: ibm,uv-firmware node creation failed\n");
> > +		return;
> > +	}
> > +
> > +	uv_opal = local_alloc(this_cpu()->chip_id, sizeof(struct
> > uv_opal), 8);
> 
> use malloc() or memalign(), uv_opal can live in the skiboot heap.
> 
> > +
> > +	tpm_nv_init();
> 
> The implementation above is a no-op, so what's this supposed to be
> doing and is there going to be a conflict with the TPM NV stuff being
> added for secure boot?

yeah, I ditched that little wrapper 

> 
> > +	get_uv_fw_region(&uv_dt_reg, &uv_fw_sz);
> > +
> > +	if (uv_xz) {
> > +		/* Hostboot path */
> > +		wait_xz_decompress(uv_xz);
> > +		if (uv_xz->status) {
> > +			prerror("UV: Compressed Ultravisor image failed
> > to decompress\n");
> > +			local_free(uv_xz);
> > +			local_free(uv_opal);
> > +			return;
> > +		}
> > +		local_free(uv_xz);
> > +	} else {
> > +		prlog(PR_INFO, "UV: Platform load failed.  You're using
> > Mambo or Cronus.\n");
> > +		/* use addr provided by reg as source and update with
> > secure memory */
> > +		uv_dt_src = uv_dt_reg;
> > +		uv_dt_reg = find_secure_mem();
> > +		prlog(PR_INFO, "UV: Copying Ultravisor to protected
> > memory 0x%llx from 0x%llx\n", uv_dt_reg, uv_dt_src);
> > +		memcpy((void *)uv_dt_reg, (void *)uv_dt_src, uv_fw_sz);
> > +		update_uv_fw_node(uv_dt_reg, uv_fw_sz);
> 
> As I said above runtime hacking the DT isn't a good idea. If you want
> to support a pre-loaded image then put it into a /reserved-memory/
> node and scan the DT for that instead of doing this dual-purpose
> thing.
> 

k, as mentioned removed runtime hacking.  I made a uv-src-address, but
didn't reserve it, will add a reserve.

> > +	
> > +
> > +	uv_opal->uv_base_addr = uv_dt_reg;
> > +
> > +	if (!(sys_fdt_addr = (__be64)create_dtb(dt_root, false)))
> > +		prerror("UV: Failed to create system fdt\n");
> 
> Why are we doing this here and not just prior to entering the UV? Can
> we use the same FDT blob that we would normally give to the OS?
> 

Yes, we can.  I shoehorned it in.  We're still starting in
load_and_boot kernel and using the fdt from there.

> > +
> > +	uv_opal->sys_fdt = sys_fdt_addr;
> > +	dt_add_property_u64s(uv_fw_node, "sys-fdt", sys_fdt_addr);
> > +
> > +}
> > +
> > +/*
> > + * Preload the UV image from PNOR partition
> > + */
> > +void uv_preload_image(void)
> > +{
> > +	int ret;
> > +
> > +	prlog(PR_DEBUG, "UV: Preload starting\n");
> > +
> > +	if (!alloc_uv()) {
> > +		prerror("UV: Memory allocation failed\n");
> > +		return;
> > +	}
> > +
> > +	ret = start_preload_resource(RESOURCE_ID_UV_IMAGE,
> > RESOURCE_SUBID_NONE,
> > +				     uv_image, &uv_image_size);
> > +
> > +	if (ret != OPAL_SUCCESS)
> > +		prerror("UV: platform load failed: %d\n", ret);
> > +}
> > +
> > +/*
> > + * Decompress the UV image
> > + *
> > + * This function modifies the uv_image variable to point to the
> > decompressed
> > + * image location.
> > + */
> > +void uv_decompress_image(void)
> > +{
> > +	uint64_t uv_fw_addr;
> > +	uint32_t uv_fw_size;
> > +
> > +	if (uv_image == NULL) {
> 
> !var is preferred to var == NULL

k, yes

> 
> > +		prerror("UV: Preload hasn't started yet! Aborting.\n");
> > +		return;
> > +	}
> > +
> > +	if (wait_for_resource_loaded(RESOURCE_ID_UV_IMAGE,
> > +				     RESOURCE_SUBID_NONE) !=
> > OPAL_SUCCESS) {
> > +		prerror("UV: Ultravisor image load failed\n");
> > +		return;
> > +	}
> > +	uv_xz = local_alloc(this_cpu()->chip_id, sizeof(struct
> > xz_decompress), 8);
> > +	if (!uv_xz) {
> > +		prerror("UV: Cannot allocate memory for decompression
> > of UV\n");
> > +		return;
> > +	}
> 
> Use malloc() for uv_xz. It's a small structure and it'll only ever be
> used inside of skiboot so it can go on the skiboot heap.
> 

K, good call.

> > +struct uv_opal {
> > +	__be32 magic;		/**< 'OPUV' 0x4F505556 OPUV_MAGIC
> > */
> > +	__be32 version;		/**< uv_opal struct version */
> > +	__be32 uv_api_ver;	/**< Current uv api version. */
> > +	__be64 uv_base_addr;	/**< Base address of UV in secure
> > memory. */
> > +	__be64 sys_fdt;		/**< System FDT. */
> > +	__be64 uv_fdt;		/**< UV FDT in secure memory. */
> > +	__be64 uv_mem;		/**< struct memcons */
> > +};
> 
> I still don't understand why this exists.

It's gone!  Everything passed to the UV is in the fdt now.

> 
> > +#define UV_SECURE_MEM_BIT              (PPC_BIT(15))
> 
> doesn't need parens
> 
> > +#define MAX_COMPRESSED_UV_IMAGE_SIZE 0x40000 /* 256 Kilobytes */
> > +#define UV_ACCESS_BIT		0x1ULL << 48
> 
> does need parens

Yep, thanks for the attention to detail.

Thanks again,
Ryan