Re: [ImageBuilder][PATCH v2 2/2] uboot-script-gen: add support for static shared memory

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Mar 2023, jiamei.xie wrote:
> Introduce support for creating shared-mem node for dom0less domUs in
> the device tree. Add the following option:
> - DOMU_SHARED_MEM[number]="SHM-ID HPA GPA size"
>   if specified, indicate the unique identifier of the shared memory
>   region is SHM-ID, the host physical address HPA will get mapped at
>   guest address GPA in domU and the memory of size will be reserved to
>   be shared memory.
> 
> The static shared memory is used between two dom0less domUs.
> 
> Below is an example:
> NUM_DOMUS=2
> DOMU_SHARED_MEM[0]="my-shared-mem-0 0x50000000 0x6000000 0x10000000"
> DOMU_SHARED_MEM[1]="my-shared-mem-0 0x50000000 0x6000000 0x10000000"
> 
> This static shared memory region is identified as "my-shared-mem-0",
> host physical address starting at 0x50000000 of 256MB will be reserved
> to be shared between two domUs. It will get mapped at 0x6000000 in both
> guest physical address space. Both DomUs are the borrower domain, the
> owner domain is the default owner domain DOMID_IO.
> 
> Signed-off-by: jiamei.xie <jiamei.xie@xxxxxxx>

The patch is good just a couple of minor comments


> ---
> Changes from v1:
>  - Rather than two separate properties and just use one like follows:
>    Change
>      DOMU_SHARED_MEM[0]="0x50000000 0x6000000 0x10000000"
>      DOMU_SHARED_MEM_ID[0]="my-shared-mem-0"
>    to
>      DOMU_SHARED_MEM[0]="my-shared-mem-0 0x50000000 0x6000000 0x10000000"
> - Use split_value function instead of opencoding it.
> ---
>  README.md                | 17 +++++++++++++++++
>  scripts/uboot-script-gen | 27 +++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/README.md b/README.md
> index 78b83f1..fe5d205 100644
> --- a/README.md
> +++ b/README.md
> @@ -196,6 +196,23 @@ Where:
>    if specified, indicates the host physical address regions
>    [baseaddr, baseaddr + size) to be reserved to the VM for static allocation.
>  
> +- DOMU_SHARED_MEM[number]="SHM-ID HPA GPA size"
> +  if specified, indicate SHM-ID represents the unique identifier of the 
> shared
> +  memory region, the host physical address HPA will get mapped at guest
> +  address GPA in domU and the memory of size will be reserved to be shared
> +  memory. The shared memory is used between two dom0less domUs.
> +
> +  Below is an example:
> +  NUM_DOMUS=2
> +  DOMU_SHARED_MEM[0]="my-shared-mem-0 0x50000000 0x6000000 0x10000000"
> +  DOMU_SHARED_MEM[1]="my-shared-mem-0 0x50000000 0x6000000 0x10000000"
> +
> +  This static shared memory region is identified as "my-shared-mem-0", host
> +  physical address starting at 0x50000000 of 256MB will be reserved to be
> +  shared between two domUs. It will get mapped at 0x6000000 in both guest
> +  physical address space. Both DomUs are the borrower domain, the owner
> +  domain is the default owner domain DOMID_IO.
> +
>  - DOMU_DIRECT_MAP[number] can be set to 1 or 0.
>    If set to 1, the VM is direct mapped. The default is 1.
>    This is only applicable when DOMU_STATIC_MEM is specified.
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index cca3e59..46ea7e5 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -204,6 +204,28 @@ function add_device_tree_xen_static_heap()
>      dt_set "$path" "xen,static-heap" "hex" "${cells[*]}"
>  }
>  
> +function add_device_tree_static_shared_mem()
> +{
> +    local path=$1
> +    local domid=$2

I don't think we need a "domid" parameter, do we? Given that the node
name has the address in it, it cannot conflict anyway with other similar
nodes. So, the following should work?

dt_set "${path}/shared-mem@${shared_mem_host}" "compatible" "str" 
"xen,domain-shared-memory-v1"


> +    local shared_mem=$3
> +    local SHARED_MEM_ID=${shared_mem%% *}
> +    local regions="${shared_mem#* }"
> +    local cells=()
> +    local SHARED_MEM_HOST=${regions%% *}

please use lower capital letters for local variables, so shared_mem_host
instead of SHARED_MEM_HOST


> +    dt_mknode "${path}" "domU${domid}-shared-mem@${SHARED_MEM_HOST}"
> +
> +    for val in ${regions[@]}
> +    do
> +        cells+=("$(split_value $val)")
> +    done
> +
> +    dt_set "${path}/domU${domid}-shared-mem@${SHARED_MEM_HOST}" "compatible" 
> "str" "xen,domain-shared-memory-v1"
> +    dt_set "${path}/domU${domid}-shared-mem@${SHARED_MEM_HOST}" "xen,shm-id" 
> "str" "${SHARED_MEM_ID}"
> +    dt_set "${path}/domU${domid}-shared-mem@${SHARED_MEM_HOST}" 
> "xen,shared-mem" "hex" "${cells[*]}"
> +}
> +
>  function add_device_tree_cpupools()
>  {
>      local cpu
> @@ -329,6 +351,11 @@ function xen_device_tree_editing()
>              dt_set "/chosen/domU$i" "xen,enhanced" "str" "enabled"
>          fi
>  
> +        if test -n "${DOMU_SHARED_MEM[i]}"
> +        then
> +            add_device_tree_static_shared_mem "/chosen/domU${i}" "${i}" 
> "${DOMU_SHARED_MEM[i]}"
> +        fi

The reason why I suggested to remove the "domid" parameter above is
that ${i} here is not actually the domid, it is just the numeric order
of the domain in the ImageBuilder config file. It happens often in my
tests that Xen assigns different domids to the domains compared to the
order they appear in the ImageBuilder config file and in device tree.


>          if test "${DOMU_COLORS[$i]}"
>          then
>              local startcolor=$(echo "${DOMU_COLORS[$i]}"  | cut -d "-" -f 1)
> -- 
> 2.25.1
>