Re: [Xen-devel] [PATCH v5 3/3] x86/boot: Introduce the setup_indirect

[Borislav Petkov <bp@xxxxxxxxx>]

On Mon, Nov 04, 2019 at 04:13:54PM +0100, Daniel Kiper wrote:
> diff --git a/arch/x86/kernel/kdebugfs.c b/arch/x86/kernel/kdebugfs.c
> index edaa30b20841..701a98300f86 100644
> --- a/arch/x86/kernel/kdebugfs.c
> +++ b/arch/x86/kernel/kdebugfs.c
> @@ -44,7 +44,11 @@ static ssize_t setup_data_read(struct file *file, char 
> __user *user_buf,
>       if (count > node->len - pos)
>               count = node->len - pos;
>  
> -     pa = node->paddr + sizeof(struct setup_data) + pos;
> +     pa = node->paddr + pos;
> +
> +     if (!(node->type & SETUP_INDIRECT) || node->type == SETUP_INDIRECT)

This check looks strange at a first glance and could use a comment.

> +             pa += sizeof(struct setup_data);
> +
>       p = memremap(pa, count, MEMREMAP_WB);
>       if (!p)
>               return -ENOMEM;
> @@ -108,9 +112,17 @@ static int __init create_setup_data_nodes(struct dentry 
> *parent)
>                       goto err_dir;
>               }
>  
> -             node->paddr = pa_data;
> -             node->type = data->type;
> -             node->len = data->len;
> +             if (data->type == SETUP_INDIRECT &&
> +                 ((struct setup_indirect *)data->data)->type != 
> SETUP_INDIRECT) {
> +                     node->paddr = ((struct setup_indirect 
> *)data->data)->addr;
> +                     node->type = ((struct setup_indirect 
> *)data->data)->type;
> +                     node->len = ((struct setup_indirect *)data->data)->len;

Align them vertically on the "=" sign even if they stick out over the
80-cols rule.

> +             } else {
> +                     node->paddr = pa_data;
> +                     node->type = data->type;
> +                     node->len = data->len;
> +             }
> +
>               create_setup_data_node(d, no, node);
>               pa_data = data->next;
>  
> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> index 7969da939213..14ef8121aa53 100644
> --- a/arch/x86/kernel/ksysfs.c
> +++ b/arch/x86/kernel/ksysfs.c
> @@ -100,7 +100,11 @@ static int __init get_setup_data_size(int nr, size_t 
> *size)
>               if (!data)
>                       return -ENOMEM;
>               if (nr == i) {
> -                     *size = data->len;
> +                     if (data->type == SETUP_INDIRECT &&
> +                         ((struct setup_indirect *)data->data)->type != 
> SETUP_INDIRECT)
> +                             *size = ((struct setup_indirect 
> *)data->data)->len;
> +                     else
> +                             *size = data->len;

<---- newline here.

>                       memunmap(data);
>                       return 0;
>               }
> @@ -130,7 +134,10 @@ static ssize_t type_show(struct kobject *kobj,
>       if (!data)
>               return -ENOMEM;
>  
> -     ret = sprintf(buf, "0x%x\n", data->type);
> +     if (data->type == SETUP_INDIRECT)
> +             ret = sprintf(buf, "0x%x\n", ((struct setup_indirect 
> *)data->data)->type);
> +     else
> +             ret = sprintf(buf, "0x%x\n", data->type);
>       memunmap(data);
>       return ret;
>  }
> @@ -142,7 +149,7 @@ static ssize_t setup_data_data_read(struct file *fp,
>                                   loff_t off, size_t count)
>  {
>       int nr, ret = 0;
> -     u64 paddr;
> +     u64 paddr, len;
>       struct setup_data *data;
>       void *p;
>  
> @@ -157,19 +164,28 @@ static ssize_t setup_data_data_read(struct file *fp,
>       if (!data)
>               return -ENOMEM;
>  
> -     if (off > data->len) {
> +     if (data->type == SETUP_INDIRECT &&
> +         ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> +             paddr = ((struct setup_indirect *)data->data)->addr;
> +             len = ((struct setup_indirect *)data->data)->len;
> +     } else {
> +             paddr += sizeof(*data);
> +             len = data->len;
> +     }
> +
> +     if (off > len) {
>               ret = -EINVAL;
>               goto out;
>       }
>  
> -     if (count > data->len - off)
> -             count = data->len - off;
> +     if (count > len - off)
> +             count = len - off;
>  
>       if (!count)
>               goto out;
>  
>       ret = count;
> -     p = memremap(paddr + sizeof(*data), data->len, MEMREMAP_WB);
> +     p = memremap(paddr, len, MEMREMAP_WB);
>       if (!p) {
>               ret = -ENOMEM;
>               goto out;
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 77ea96b794bd..4603702dbfc1 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -438,6 +438,10 @@ static void __init 
> memblock_x86_reserve_range_setup_data(void)
>       while (pa_data) {
>               data = early_memremap(pa_data, sizeof(*data));
>               memblock_reserve(pa_data, sizeof(*data) + data->len);

<---- newline here.

> +             if (data->type == SETUP_INDIRECT &&
> +                 ((struct setup_indirect *)data->data)->type != 
> SETUP_INDIRECT)
> +                     memblock_reserve(((struct setup_indirect 
> *)data->data)->addr,
> +                                      ((struct setup_indirect 
> *)data->data)->len);

<---- newline here.

Let's space that statement out for better readability.

>               pa_data = data->next;
>               early_memunmap(data, sizeof(*data));
>       }
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index a39dcdb5ae34..1ff9c2030b4f 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -626,6 +626,17 @@ static bool memremap_is_setup_data(resource_size_t 
> phys_addr,
>               paddr_next = data->next;
>               len = data->len;
>  
> +             if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
> +                     memunmap(data);
> +                     return true;
> +             }
> +
> +             if (data->type == SETUP_INDIRECT &&
> +                 ((struct setup_indirect *)data->data)->type != 
> SETUP_INDIRECT) {
> +                     paddr = ((struct setup_indirect *)data->data)->addr;
> +                     len = ((struct setup_indirect *)data->data)->len;
> +             }
> +
>               memunmap(data);
>  
>               if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
> -- 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel