Re: [PATCH 2/2] x86/boot: Add setup_indirect support in early_memremap_is_setup_data

[Ross Philipson <ross.philipson@oracle.com>]

On 2/11/22 13:41, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 12:04:16PM -0500, Ross Philipson wrote:
>> The x86 boot documentation describes the setup_indirect structures and
>> how they are used. Only one of the two functions in ioremap.c that needed
>> to be modified to be aware of the introduction of setup_indirect
>> functionality was updated. This adds comparable support to the other
> 
> s/This adds/Add/

Ack.

> 
>> function where it was missing.
>>
>> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
>>
> 
> No need for a newline here.
> 
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>  arch/x86/mm/ioremap.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index b45e86e..9129b29 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -670,17 +670,34 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>>  
>>  	paddr = boot_params.hdr.setup_data;
>>  	while (paddr) {
>> -		unsigned int len;
>> +		unsigned int len, size;
>>  
>>  		if (phys_addr == paddr)
>>  			return true;
>>  
>>  		data = early_memremap_decrypted(paddr, sizeof(*data));
>> +		size = sizeof(*data);
>>  
>>  		paddr_next = data->next;
>>  		len = data->len;
>>  
>> -		early_memunmap(data, sizeof(*data));
>> +		if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
>> +			early_memunmap(data, sizeof(*data));
>> +			return true;
>> +		}
>> +
>> +		if (data->type == SETUP_INDIRECT) {
>> +			size += len;
>> +			early_memunmap(data, sizeof(*data));
>> +			data = early_memremap_decrypted(paddr, size);
> 
> That can return NULL.

Although there is a bit more indirection here, it is the same reply with
questions in the first patch.

> 
>> +
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +				paddr = ((struct setup_indirect *)data->data)->addr;
>> +				len = ((struct setup_indirect *)data->data)->len;
> 
> As before, use a helper variable here pls.
> 
> Thx.
> 

Thanks,

Ross Philipson