Re: [PATCH V2] powerpc/mm: Add validation for platform reserved memory ranges

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 07/05/2016 07:25 AM, Michael Ellerman wrote:
> Anshuman Khandual <khandual@linux.vnet.ibm.com> writes:
> 
>> For partition running on PHYP, there can be a adjunct partition
>> which shares the virtual address range with the operating system.
>> Virtual address ranges which can be used by the adjunct partition
>> are communicated with virtual device node of the device tree with
>> a property known as "ibm,reserved-virtual-addresses". This patch
>> introduces a new function named 'validate_reserved_va_range' which
>> is called  during initialization to validate that these reserved
>> virtual address ranges do not overlap with the address ranges used
>> by the kernel for all supported memory contexts. This helps prevent
>> the possibility of getting return codes similar to H_RESOURCE for
>> H_PROTECT hcalls for conflicting HPTE entries.
> 
> Have you tested this? The endian conversions look wrong to me.

I had tested this both on LE and BE LPARs on PVM environment.

> 
>> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
>> index ba59d59..b47f667 100644
>> --- a/arch/powerpc/mm/hash_utils_64.c
>> +++ b/arch/powerpc/mm/hash_utils_64.c
>> @@ -1564,3 +1564,80 @@ void setup_initial_memory_limit(phys_addr_t first_memblock_base,
>>  	/* Finally limit subsequent allocations */
>>  	memblock_set_current_limit(ppc64_rma_size);
>>  }
>> +
>> +/*
>> + * PAPR says that each reserved virtual address range record
>> + * contains three be32 elements which is of toal 12 bytes.
>> + * First two be32 elements contain the abbreviated virtual
>> + * address (high order 32 bits and low order 32 bits that
>> + * generate the abbreviated virtual address of 64 bits which
>> + * need to be concatenated with 24 bits of 0 at the end) and
>> + * the third be32 element contains the size of the reserved
>> + * virtual address range as number of consecutive 4K pages.
>> + */
>> +struct reserved_va_record {
>> +	__be32	high_addr;
>> +	__be32	low_addr;
>> +	__be32	nr_pages_4K;
>> +};
> 
> Here you define those fields as __be32.

Hmm, I believe we had agreed upon this. Will check back.

> 
>> +/*
>> + * Linux uses 65 bits (CONTEXT_BITS + ESID_BITS + SID_SHIFT)
>> + * of virtual address. As reserved virtual address comes in
>> + * as an abbreviated form (64 bits) from the device tree, we
>> + * will use a partial address bit mask (65 >> 24) to match it
>> + * for simplicity.
>> + */
>> +#define RVA_LESS_BITS		24
>> +#define LINUX_VA_BITS		(CONTEXT_BITS + ESID_BITS + SID_SHIFT)
>> +#define PARTIAL_LINUX_VA_MASK	((1ULL << (LINUX_VA_BITS - RVA_LESS_BITS)) - 1)
>> +
>> +static int __init validate_reserved_va_range(void)
>> +{
>> +	struct reserved_va_record rva;
>> +	struct device_node *np;
>> +	int records, ret, i;
>> +	__be64 vaddr;
>> +
>> +	np = of_find_node_by_name(NULL, "vdevice");
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	records = of_property_count_elems_of_size(np,
>> +			"ibm,reserved-virtual-addresses",
>> +				sizeof(struct reserved_va_record));
>> +	if (records < 0)
>> +		return records;
>> +
>> +	for (i = 0; i < records; i++) {
>> +		ret = of_property_read_u32_index(np,
>> +			"ibm,reserved-virtual-addresses",
>> +				3 * i, &rva.high_addr);
> 
> But then here you use of_property_read_u32_index(), which does the
> endian conversion (to CPU endian) for you.

Okay.

> 
>> +		ret = of_property_read_u32_index(np,
>> +			"ibm,reserved-virtual-addresses",
>> +				3 * i + 1, &rva.low_addr);
> 
>> +		ret = of_property_read_u32_index(np,
>> +			"ibm,reserved-virtual-addresses",
>> +				3 * i + 2, &rva.nr_pages_4K);
> 
> So now all the values in rva are CPU endian.

Okay.

> 
>> +		vaddr =  rva.high_addr;
>> +		vaddr =  (vaddr << 32) | rva.low_addr;
>> +		if (vaddr & cpu_to_be64(~PARTIAL_LINUX_VA_MASK))
>> +			continue;
> 
> But then here you do the comparison against a __be64 value.
> 
> I know I told you to use "properly endian-annotated struct", but you
> stil need to use the right conversions in the right places.
> 
> I think the best option is to use of_property_read_u32_array() and just
> read the three 32 values into a CPU endian struct.

Sure. But I have kind of lost context of this patch, will look into these
details and get back.