Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi,
On Thu, Jul 28, 2022 at 9:41 PM Julien Grall wrote:
>
> Hi,
>
> On 27/07/2022 07:33, Jens Wiklander wrote:
> > On Fri, Jul 8, 2022 at 9:54 PM Julien Grall wrote:
> >>> +unsigned int n;
> >>> +unsigned int m;
> >>> +p2m_type_t t;
> >>> +uint64_t addr;
> >>> +
> >>> +for ( n = 0; n < range_count; n++ )
> >>> +{
> >>> +for ( m = 0; m < range[n].page_count; m++ )
> >>> +{
> >>> +if ( pg_idx >= shm->page_count )
> >>> +return FFA_RET_INVALID_PARAMETERS;
> >>
> >> Shouldn't we call put_page() to drop the references taken by
> >> get_page_from_gfn()?
> >
> > Yes, and that's done by put_shm_pages(). One would normally expect
> > get_shm_pages() to do this on error, but that's not needed here since
> > we're always calling put_shm_pages() just before freeing the shm. I
> > can change to let get_shm_pages() do the cleanup on error instead if
> > you prefer that.
>
> I am fine with the current approach. I would suggest to document it on
> top of get_shm_pages().
>
> Also, if you expect put_shm_pages() to always be called before freeing
> shm, then I think it would be worth adding an helper that is doing the
> two. So the requirement is clearer.
OK, I'll fix.
>
> [...]
>
> >>
> >> How do you guarantee that both Xen and the domain agree on the page size?
> >
> > For now, I'll add a BUILD_BUG_ON() to check that the hypervisor page
> > size is 4K to simplify the initial implementation. We can update to
> > support a larger minimal memory granule later on.
>
> I am fine with that. FWIW, this is also what we did in the OP-TEE case.
>
> >>> +for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
> >>> +{
> >>> +pa = page_to_maddr(shm->pages[n]);
> >>> +if ( last_pa + PAGE_SIZE == pa )
> >>> +{
> >>
> >> Coding style: We usually avoid {} for single line.
> >
> > OK
> >
> >>
> >>> +continue;
> >>> +}
> >>> +region_descr->address_range_count++;
> >>> +}
> >>> +
> >>> +tot_len = sizeof(*descr) + sizeof(*mem_access_array) +
> >>> + sizeof(*region_descr) +
> >>> + region_descr->address_range_count * sizeof(*addr_range);
> >>
> >> How do you make sure that you will not write past the end of ffa_tx?
> >>
> >> I think it would be worth to consider adding an helper that would allow
> >> you to allocate space in ffa_tx and zero it. This would return an error
> >> if there is not enough space.
> >
> > That's what I'm doing with frag_len. If the descriptor cannot fit it's
> > divided into fragments that will fit.
>
> Oh, so this is what the loop below is for, am I correct? If so, I would
> suggest to document a bit the code because this function is fairly
> confusing to understand.
Yeah, I'm sorry about that. I'll add a comment describing what's going on.
>
> [...]
>
> >>> +if ( read_atomic(_access->access_perm.perm) != FFA_MEM_ACC_RW )
> >>> +{
> >>> +ret = FFA_RET_NOT_SUPPORTED;
> >>> +goto out_unlock;
> >>> +}
> >>> +
> >>> +region_offs = read_atomic(_access->region_offs);
> >>> +if ( sizeof(*region_descr) + region_offs > frag_len )
> >>> +{
> >>> +ret = FFA_RET_NOT_SUPPORTED;
> >>> +goto out_unlock;
> >>> +}
> >>> +
> >>> +region_descr = (void *)((vaddr_t)ctx->tx + region_offs);
> >>> +range_count = read_atomic(_descr->address_range_count);
> >>> +page_count = read_atomic(_descr->total_page_count);
> >>> +
> >>> +shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count)
> >> This will allow a guest to allocate an arbitrarily large array in Xen.
> >> So please sanitize page_count before using it.
> >
> > This is tricky, what is a reasonable limit?
>
> Indeed. We need a limit that will prevent an untrusted domain to DoS Xen
> and at the same doesn't prevent the majority of well-behave domain to
> function.
>
> How is this call going to be used?
>
> > If we do set a limit the
> > guest can still share many separate memory ranges.
>
> This would also need to be limited if there is a desire to support
> untrusted domain.
I see that someone obviously has asked the same questions about the
OP-TEE mediator in the TEE mediator framework. I'll try the same
approach with limit etc since I guess the use-case there and here at
least initially will be similar.
>
> [...]
>
> >>> +ret = get_shm_pages(d, shm, region_descr->address_range_array,
> >>> range_count,
> >>> +0, _page_idx);
> >>> +if ( ret )
> >>> +goto out;
> >>> +if ( last_page_idx != shm->page_count )
> >>> +{
> >>> +ret = FFA_RET_INVALID_PARAMETERS;
> >>> +goto out;
> >>> +}
> >>> +
> >>> +/* Note that share_shm() uses our tx buffer */
> >>> +spin_lock(_buffer_lock);
> >>> +ret = share_shm(shm);
> >>> +spin_unlock(_buffer_lock);
> >>> +if ( ret )
> >>> +goto out;
> >>> +
> >>> +spin_lock(_mem_list_lock);
> >>> +
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi,
On Thu, Jul 28, 2022 at 9:15 PM Julien Grall wrote:
>
> Hi,
>
> On 26/07/2022 07:17, Jens Wiklander wrote:
> > On Fri, Jul 8, 2022 at 3:41 PM Julien Grall wrote:
> >>
> >> Hi Jens,
> >>
> >> I haven't checked whether the FFA driver is complaint with the spec. I
> >> mainly checked whether the code makes sense from Xen PoV.
> >>
> >> This is a fairly long patch to review. So I will split my review in
> >> multiple session/e-mail.
> >>
> >> On 22/06/2022 14:42, Jens Wiklander wrote:
> >>> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> >>> Partition in secure world.
> >>>
> >>> The implementation is the bare minimum to be able to communicate with
> >>> OP-TEE running as an SPMC at S-EL1.
> >>>
> >>> This is loosely based on the TEE mediator framework and the OP-TEE
> >>> mediator.
> >>>
> >>> [1] https://developer.arm.com/documentation/den0077/latest
> >>> Signed-off-by: Jens Wiklander
> >>> ---
> >>>SUPPORT.md|7 +
> >>>tools/libs/light/libxl_arm.c |3 +
> >>>tools/libs/light/libxl_types.idl |1 +
> >>>tools/xl/xl_parse.c |3 +
> >>
> >> These changes would need an ack from the toolstack maintainers.
> >
> > OK, I'll keep them in CC.
> >
> >>
> >>>xen/arch/arm/Kconfig | 11 +
> >>>xen/arch/arm/Makefile |1 +
> >>>xen/arch/arm/domain.c | 10 +
> >>>xen/arch/arm/domain_build.c |1 +
> >>>xen/arch/arm/ffa.c| 1683 +
> >>>xen/arch/arm/include/asm/domain.h |4 +
> >>>xen/arch/arm/include/asm/ffa.h| 71 ++
> >>>xen/arch/arm/vsmc.c | 17 +-
> >>>xen/include/public/arch-arm.h |2 +
> >>>13 files changed, 1811 insertions(+), 3 deletions(-)
> >>>create mode 100644 xen/arch/arm/ffa.c
> >>>create mode 100644 xen/arch/arm/include/asm/ffa.h
> >>>
> >>> diff --git a/SUPPORT.md b/SUPPORT.md
> >>> index 70e98964cbc0..215bb3c9043b 100644
> >>> --- a/SUPPORT.md
> >>> +++ b/SUPPORT.md
> >>> @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed
> >>> through.
> >>>
> >>>No support for QEMU backends in a 16K or 64K domain.
> >>>
> >>> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> >>> +
> >>> +Status, Arm64: Tech Preview
> >>> +
> >>> +There are still some code paths where a vCPU may hog a pCPU longer than
> >>> +necessary. The FF-A mediator is not yet implemented for Arm32.
> >>> +
> >>>### ARM: Guest Device Tree support
> >>>
> >>>Status: Supported
> >>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >>> index eef1de093914..a985609861c7 100644
> >>> --- a/tools/libs/light/libxl_arm.c
> >>> +++ b/tools/libs/light/libxl_arm.c
> >>> @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >>>return ERROR_FAIL;
> >>>}
> >>>
> >>> +config->arch.ffa_enabled =
> >>> +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> >>> +
> >>>return 0;
> >>>}
> >>>
> >>> diff --git a/tools/libs/light/libxl_types.idl
> >>> b/tools/libs/light/libxl_types.idl
> >>> index 2a42da2f7d78..bf4544bef399 100644
> >>> --- a/tools/libs/light/libxl_types.idl
> >>> +++ b/tools/libs/light/libxl_types.idl
> >>> @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>>
> >>>("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >>> ("vuart", libxl_vuart_type),
> >>> + ("ffa_enabled", libxl_defbool),
> >>
> >> This needs to be accompagnied with a define LIBXL_HAVE_* in
> >> tools/include/libxl.h. Please see the examples in the header.
> >
> > OK, I'll add something. I'm not entirely sure how this is used so I'm
> > afraid it will be a bit of Cargo Cult programming from my side.
>
> The LIBXL_HAVE* by toolstack built on top of libxl (like virtio) to know
> whether a future is supported by the current library.
>
> [...]
>
> >>
> >>> +
> >>> +static inline uint64_t reg_pair_to_64(uint32_t reg0, uint32_t reg1)
> >>> +{
> >>> +return (uint64_t)reg0 << 32 | reg1;
> >>> +}
> >>> +
> >>> +static void inline reg_pair_from_64(uint32_t *reg0, uint32_t *reg1,
> >>> +uint64_t val)
> >>> +{
> >>> +*reg0 = val >> 32;
> >>> +*reg1 = val;
> >>> +}
> >>
> >> Those two functions are the same as optee.c but with a different. I
> >> would rather prefer if we avoid the duplication and provide generic
> >> helpers in a pre-requisite.
> >
> > These functions are trivial but at the same time for a special purpose
> > which happens to coincide with the usage in optee.c. I can't find a
> > suitable common .h file and creating a new one seems a bit much.
>
> I would implement it in regs.h.
OK, thanks.
>
> [...]
>
> >>> +.a4 = pg_count,
> >>> +};
> >>> +struct arm_smccc_1_2_regs resp;
> >>> +
> >>> +/*
>
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi,
On 27/07/2022 07:33, Jens Wiklander wrote:
On Fri, Jul 8, 2022 at 9:54 PM Julien Grall wrote:
+unsigned int n;
+unsigned int m;
+p2m_type_t t;
+uint64_t addr;
+
+for ( n = 0; n < range_count; n++ )
+{
+for ( m = 0; m < range[n].page_count; m++ )
+{
+if ( pg_idx >= shm->page_count )
+return FFA_RET_INVALID_PARAMETERS;
Shouldn't we call put_page() to drop the references taken by
get_page_from_gfn()?
Yes, and that's done by put_shm_pages(). One would normally expect
get_shm_pages() to do this on error, but that's not needed here since
we're always calling put_shm_pages() just before freeing the shm. I
can change to let get_shm_pages() do the cleanup on error instead if
you prefer that.
I am fine with the current approach. I would suggest to document it on
top of get_shm_pages().
Also, if you expect put_shm_pages() to always be called before freeing
shm, then I think it would be worth adding an helper that is doing the
two. So the requirement is clearer.
[...]
How do you guarantee that both Xen and the domain agree on the page size?
For now, I'll add a BUILD_BUG_ON() to check that the hypervisor page
size is 4K to simplify the initial implementation. We can update to
support a larger minimal memory granule later on.
I am fine with that. FWIW, this is also what we did in the OP-TEE case.
+for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+{
+pa = page_to_maddr(shm->pages[n]);
+if ( last_pa + PAGE_SIZE == pa )
+{
Coding style: We usually avoid {} for single line.
OK
+continue;
+}
+region_descr->address_range_count++;
+}
+
+tot_len = sizeof(*descr) + sizeof(*mem_access_array) +
+ sizeof(*region_descr) +
+ region_descr->address_range_count * sizeof(*addr_range);
How do you make sure that you will not write past the end of ffa_tx?
I think it would be worth to consider adding an helper that would allow
you to allocate space in ffa_tx and zero it. This would return an error
if there is not enough space.
That's what I'm doing with frag_len. If the descriptor cannot fit it's
divided into fragments that will fit.
Oh, so this is what the loop below is for, am I correct? If so, I would
suggest to document a bit the code because this function is fairly
confusing to understand.
[...]
+if ( read_atomic(_access->access_perm.perm) != FFA_MEM_ACC_RW )
+{
+ret = FFA_RET_NOT_SUPPORTED;
+goto out_unlock;
+}
+
+region_offs = read_atomic(_access->region_offs);
+if ( sizeof(*region_descr) + region_offs > frag_len )
+{
+ret = FFA_RET_NOT_SUPPORTED;
+goto out_unlock;
+}
+
+region_descr = (void *)((vaddr_t)ctx->tx + region_offs);
+range_count = read_atomic(_descr->address_range_count);
+page_count = read_atomic(_descr->total_page_count);
+
+shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count)
This will allow a guest to allocate an arbitrarily large array in Xen.
So please sanitize page_count before using it.
This is tricky, what is a reasonable limit?
Indeed. We need a limit that will prevent an untrusted domain to DoS Xen
and at the same doesn't prevent the majority of well-behave domain to
function.
How is this call going to be used?
If we do set a limit the
guest can still share many separate memory ranges.
This would also need to be limited if there is a desire to support
untrusted domain.
[...]
+ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
+0, _page_idx);
+if ( ret )
+goto out;
+if ( last_page_idx != shm->page_count )
+{
+ret = FFA_RET_INVALID_PARAMETERS;
+goto out;
+}
+
+/* Note that share_shm() uses our tx buffer */
+spin_lock(_buffer_lock);
+ret = share_shm(shm);
+spin_unlock(_buffer_lock);
+if ( ret )
+goto out;
+
+spin_lock(_mem_list_lock);
+list_add_tail(>list, _mem_list);
A couple of questions:
- What is the maximum size of the list?
Currently, there is no limit. I'm not sure what is a reasonable limit
more than five for sure, but depending on the use case more than 100
might be excessive.
This is fine to be excessive so long it doesn't allow a guest to drive
Xen out of memory or allow long running operations.
As I wrote above, the idea is we need limits that protect Xen but at the
same time doesn't prevent the majority well-behave guest to function.
As this is a tech preview, the limits can be low. We can raise the
limits as we get a better understanding how this will be used.
Cheers,
--
Julien Grall
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi,
On 26/07/2022 07:17, Jens Wiklander wrote:
On Fri, Jul 8, 2022 at 3:41 PM Julien Grall wrote:
Hi Jens,
I haven't checked whether the FFA driver is complaint with the spec. I
mainly checked whether the code makes sense from Xen PoV.
This is a fairly long patch to review. So I will split my review in
multiple session/e-mail.
On 22/06/2022 14:42, Jens Wiklander wrote:
Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.
The implementation is the bare minimum to be able to communicate with
OP-TEE running as an SPMC at S-EL1.
This is loosely based on the TEE mediator framework and the OP-TEE
mediator.
[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander
---
SUPPORT.md|7 +
tools/libs/light/libxl_arm.c |3 +
tools/libs/light/libxl_types.idl |1 +
tools/xl/xl_parse.c |3 +
These changes would need an ack from the toolstack maintainers.
OK, I'll keep them in CC.
xen/arch/arm/Kconfig | 11 +
xen/arch/arm/Makefile |1 +
xen/arch/arm/domain.c | 10 +
xen/arch/arm/domain_build.c |1 +
xen/arch/arm/ffa.c| 1683 +
xen/arch/arm/include/asm/domain.h |4 +
xen/arch/arm/include/asm/ffa.h| 71 ++
xen/arch/arm/vsmc.c | 17 +-
xen/include/public/arch-arm.h |2 +
13 files changed, 1811 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/ffa.c
create mode 100644 xen/arch/arm/include/asm/ffa.h
diff --git a/SUPPORT.md b/SUPPORT.md
index 70e98964cbc0..215bb3c9043b 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
No support for QEMU backends in a 16K or 64K domain.
+### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
+
+Status, Arm64: Tech Preview
+
+There are still some code paths where a vCPU may hog a pCPU longer than
+necessary. The FF-A mediator is not yet implemented for Arm32.
+
### ARM: Guest Device Tree support
Status: Supported
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de093914..a985609861c7 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
return ERROR_FAIL;
}
+config->arch.ffa_enabled =
+libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
+
return 0;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d78..bf4544bef399 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
+ ("ffa_enabled", libxl_defbool),
This needs to be accompagnied with a define LIBXL_HAVE_* in
tools/include/libxl.h. Please see the examples in the header.
OK, I'll add something. I'm not entirely sure how this is used so I'm
afraid it will be a bit of Cargo Cult programming from my side.
The LIBXL_HAVE* by toolstack built on top of libxl (like virtio) to know
whether a future is supported by the current library.
[...]
+
+static inline uint64_t reg_pair_to_64(uint32_t reg0, uint32_t reg1)
+{
+return (uint64_t)reg0 << 32 | reg1;
+}
+
+static void inline reg_pair_from_64(uint32_t *reg0, uint32_t *reg1,
+uint64_t val)
+{
+*reg0 = val >> 32;
+*reg1 = val;
+}
Those two functions are the same as optee.c but with a different. I
would rather prefer if we avoid the duplication and provide generic
helpers in a pre-requisite.
These functions are trivial but at the same time for a special purpose
which happens to coincide with the usage in optee.c. I can't find a
suitable common .h file and creating a new one seems a bit much.
I would implement it in regs.h.
[...]
+.a4 = pg_count,
+};
+struct arm_smccc_1_2_regs resp;
+
+/*
+ * For arm64 we must use 64-bit calling convention if the buffer isn't
+ * passed in our tx buffer.
+ */
Can you explain why we would want to use the 32-bit calling convention
if addr is 0?
I was trying to avoid the 64-bit calling convention where possible,
OOI, why are you trying to avoid the 64-bit calling convention?
[...]
--
Julien Grall
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Bertrand,
On Thu, Jul 14, 2022 at 11:51 AM Bertrand Marquis
wrote:
>
> Hi Jens,
>
> > On 22 Jun 2022, at 14:42, Jens Wiklander wrote:
> >
> > Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> > Partition in secure world.
> >
> > The implementation is the bare minimum to be able to communicate with
> > OP-TEE running as an SPMC at S-EL1.
> >
> > This is loosely based on the TEE mediator framework and the OP-TEE
> > mediator.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander
>
> I spent quite some time on this patch and on the spec and there are far to
> much code and concepts introduced here to be able to do a review in one go.
>
> Could you try to split the patch to introduce each concept in a specific
> patch ?
> I would suggest something like introducing each call in its own patch, having
> a specific patch for the tool support, etc.
>
> At this stage I am not convinced that there is no issue here where a guest
> could
> access information from an other guest and reviewing smaller patches will help
> me following the spec for each subject and ask questions along the way.
OK, I'll try to split it up a bit in the next version.
Cheers,
Jens
>
> Cheers
> Bertrand
>
> > ---
> > SUPPORT.md|7 +
> > tools/libs/light/libxl_arm.c |3 +
> > tools/libs/light/libxl_types.idl |1 +
> > tools/xl/xl_parse.c |3 +
> > xen/arch/arm/Kconfig | 11 +
> > xen/arch/arm/Makefile |1 +
> > xen/arch/arm/domain.c | 10 +
> > xen/arch/arm/domain_build.c |1 +
> > xen/arch/arm/ffa.c| 1683 +
> > xen/arch/arm/include/asm/domain.h |4 +
> > xen/arch/arm/include/asm/ffa.h| 71 ++
> > xen/arch/arm/vsmc.c | 17 +-
> > xen/include/public/arch-arm.h |2 +
> > 13 files changed, 1811 insertions(+), 3 deletions(-)
> > create mode 100644 xen/arch/arm/ffa.c
> > create mode 100644 xen/arch/arm/include/asm/ffa.h
> >
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index 70e98964cbc0..215bb3c9043b 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
> >
> > No support for QEMU backends in a 16K or 64K domain.
> >
> > +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> > +
> > +Status, Arm64: Tech Preview
> > +
> > +There are still some code paths where a vCPU may hog a pCPU longer than
> > +necessary. The FF-A mediator is not yet implemented for Arm32.
> > +
> > ### ARM: Guest Device Tree support
> >
> > Status: Supported
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index eef1de093914..a985609861c7 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > return ERROR_FAIL;
> > }
> >
> > +config->arch.ffa_enabled =
> > +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> > +
> > return 0;
> > }
> >
> > diff --git a/tools/libs/light/libxl_types.idl
> > b/tools/libs/light/libxl_types.idl
> > index 2a42da2f7d78..bf4544bef399 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >
> > ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >("vuart", libxl_vuart_type),
> > + ("ffa_enabled", libxl_defbool),
> > ])),
> > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> > ])),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index b98c0de378b6..e0e99ed8d2b1 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -2746,6 +2746,9 @@ skip_usbdev:
> > exit(-ERROR_FAIL);
> > }
> > }
> > +libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
> > +xlu_cfg_get_defbool(config, "ffa_enabled",
> > +_info->arch_arm.ffa_enabled, 0);
> >
> > parse_vkb_list(config, d_config);
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index be9eff014120..e57e1d3757e2 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -139,6 +139,17 @@ config TEE
> >
> > source "arch/arm/tee/Kconfig"
> >
> > +config FFA
> > + bool "Enable FF-A mediator support" if EXPERT
> > + default n
> > + depends on ARM_64
> > + help
> > + This option enables a minimal FF-A mediator. The mediator is
> > + generic as it follows the FF-A specification [1], but it only
> > + implements a small subset of the specification.
> > +
> > + [1] https://developer.arm.com/documentation/den0077/latest
> > +
> > endmenu
> >
> > menu "ARM errata
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Julien,
On Fri, Jul 8, 2022 at 9:54 PM Julien Grall wrote:
>
> Hi Jens,
>
> This is the second part of the review.
>
> On 22/06/2022 14:42, Jens Wiklander wrote:
> > +static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> > + struct ffa_address_range *range, uint32_t
> > range_count,
>
> AFAICT, 'range' is not meant to be modified. So I would add 'const'.
OK
>
> > + unsigned int start_page_idx,
> > + unsigned int *last_page_idx)
> > +{
> > +unsigned int pg_idx = start_page_idx;
> > +unsigned long gfn;
>
> Below you are using gaddr_to_gfn() which will return gfn_t. This is
> using the typesafe infrastructure: gfn_t will be a structure with
> CONFIG_DEBUG=y to allow type checking and a plain 'unsigned long' when
> CONFIG_DEBUG=n.
>
> Please make sure to test build with CONFIG_DEBUG=y.
>
> As a side note, I would suggest to try booting as CONFIG_DEBUG as it
> enables extra check for the common pitfalls.
Thanks, I'll do that.
>
> > +unsigned int n;
> > +unsigned int m;
> > +p2m_type_t t;
> > +uint64_t addr;
> > +
> > +for ( n = 0; n < range_count; n++ )
> > +{
> > +for ( m = 0; m < range[n].page_count; m++ )
> > +{
> > +if ( pg_idx >= shm->page_count )
> > +return FFA_RET_INVALID_PARAMETERS;
>
> Shouldn't we call put_page() to drop the references taken by
> get_page_from_gfn()?
Yes, and that's done by put_shm_pages(). One would normally expect
get_shm_pages() to do this on error, but that's not needed here since
we're always calling put_shm_pages() just before freeing the shm. I
can change to let get_shm_pages() do the cleanup on error instead if
you prefer that.
>
> > +
> > +addr = read_atomic([n].address);
>
> IIUC, range is part of the shared page with the guest. Where do you
> check that all the access will be within the shared page?
It's checked by the callers.
>
> > +gfn = gaddr_to_gfn(addr + m * PAGE_SIZE);
>
> Is addr meant to be page-aligned? Also, you are using the hypervisor
> page size here when AFAICT page_count is provided by the domain.
You're right, this is confusing. I'll define a FFA_PAGE_SIZE to SZ_4K
and use that instead. Note that with FF-A a page is always 4K even if
the smallest unit/granule may be larger (16kB or 64kB).
>
> How do you guarantee that both Xen and the domain agree on the page size?
For now, I'll add a BUILD_BUG_ON() to check that the hypervisor page
size is 4K to simplify the initial implementation. We can update to
support a larger minimal memory granule later on.
>
> > +shm->pages[pg_idx] = get_page_from_gfn(d, gfn, , P2M_ALLOC);
> > +if ( !shm->pages[pg_idx] )
> > +return FFA_RET_DENIED;
> > +pg_idx++;
> > +/* Only normal RAM for now */
>
> Similar to my earlier remark, the comment doesn't match the check below.
I'll fix.
>
> > +if ( t != p2m_ram_rw )
> > +return FFA_RET_DENIED;
> > +}
> > +}
> > +
> > +*last_page_idx = pg_idx;
> > +
> > +return FFA_RET_OK;
> > +}
> > +
> > +static void put_shm_pages(struct ffa_shm_mem *shm)
> > +{
> > +unsigned int n;
> > +
> > +for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
> > +{
> > +put_page(shm->pages[n]);
> > +shm->pages[n] = NULL;
> > +}
> > +}
> > +
> > +static void init_range(struct ffa_address_range *addr_range,
> > + paddr_t pa) > +{
> > +memset(addr_range, 0, sizeof(*addr_range));
> > +addr_range->address = pa;
> > +addr_range->page_count = 1;
> > +}
> > +
> > +static int share_shm(struct ffa_shm_mem *shm)
>
> AFAIU, share_shm() cannot be concurrently called. You seem to use
> ffa_buffer_lock to guarantee that. So I would suggest to add:
>1) an ASSERT(spin_is_Locked(_buffer_lock))
>2) a comment on top of share_shm() explaining that the function
> should be called with ffa_buffer_lock taken.
Yes, it's the ffa_tx buffer that must be protected against concurrent
use. I'll update.
>
> > +{
> > +uint32_t max_frag_len = ffa_page_count * PAGE_SIZE;
> > +struct ffa_mem_transaction_1_1 *descr = ffa_tx;
> > +struct ffa_mem_access *mem_access_array;
> > +struct ffa_mem_region *region_descr;
> > +struct ffa_address_range *addr_range;
> > +paddr_t pa;
> > +paddr_t last_pa;
> > +unsigned int n;
> > +uint32_t frag_len;
> > +uint32_t tot_len;
> > +int ret;
> > +unsigned int range_count;
> > +unsigned int range_base;
> > +bool first;
> > +
> > +memset(descr, 0, sizeof(*descr));
> > +descr->sender_id = shm->sender_id;
> > +descr->global_handle = shm->handle;
> > +descr->mem_reg_attr = FFA_NORMAL_MEM_REG_ATTR;
> > +descr->mem_access_count = 1;
> > +descr->mem_access_size = sizeof(*mem_access_array);
> > +descr->mem_access_offs = sizeof(*descr);
> > +
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Julien,
On Fri, Jul 8, 2022 at 3:41 PM Julien Grall wrote:
>
> Hi Jens,
>
> I haven't checked whether the FFA driver is complaint with the spec. I
> mainly checked whether the code makes sense from Xen PoV.
>
> This is a fairly long patch to review. So I will split my review in
> multiple session/e-mail.
>
> On 22/06/2022 14:42, Jens Wiklander wrote:
> > Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> > Partition in secure world.
> >
> > The implementation is the bare minimum to be able to communicate with
> > OP-TEE running as an SPMC at S-EL1.
> >
> > This is loosely based on the TEE mediator framework and the OP-TEE
> > mediator.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander
> > ---
> > SUPPORT.md|7 +
> > tools/libs/light/libxl_arm.c |3 +
> > tools/libs/light/libxl_types.idl |1 +
> > tools/xl/xl_parse.c |3 +
>
> These changes would need an ack from the toolstack maintainers.
OK, I'll keep them in CC.
>
> > xen/arch/arm/Kconfig | 11 +
> > xen/arch/arm/Makefile |1 +
> > xen/arch/arm/domain.c | 10 +
> > xen/arch/arm/domain_build.c |1 +
> > xen/arch/arm/ffa.c| 1683 +
> > xen/arch/arm/include/asm/domain.h |4 +
> > xen/arch/arm/include/asm/ffa.h| 71 ++
> > xen/arch/arm/vsmc.c | 17 +-
> > xen/include/public/arch-arm.h |2 +
> > 13 files changed, 1811 insertions(+), 3 deletions(-)
> > create mode 100644 xen/arch/arm/ffa.c
> > create mode 100644 xen/arch/arm/include/asm/ffa.h
> >
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index 70e98964cbc0..215bb3c9043b 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
> >
> > No support for QEMU backends in a 16K or 64K domain.
> >
> > +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> > +
> > +Status, Arm64: Tech Preview
> > +
> > +There are still some code paths where a vCPU may hog a pCPU longer than
> > +necessary. The FF-A mediator is not yet implemented for Arm32.
> > +
> > ### ARM: Guest Device Tree support
> >
> > Status: Supported
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index eef1de093914..a985609861c7 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > return ERROR_FAIL;
> > }
> >
> > +config->arch.ffa_enabled =
> > +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> > +
> > return 0;
> > }
> >
> > diff --git a/tools/libs/light/libxl_types.idl
> > b/tools/libs/light/libxl_types.idl
> > index 2a42da2f7d78..bf4544bef399 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >
> > ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > ("vuart", libxl_vuart_type),
> > + ("ffa_enabled", libxl_defbool),
>
> This needs to be accompagnied with a define LIBXL_HAVE_* in
> tools/include/libxl.h. Please see the examples in the header.
OK, I'll add something. I'm not entirely sure how this is used so I'm
afraid it will be a bit of Cargo Cult programming from my side.
>
> > ])),
> > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> > ])),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index b98c0de378b6..e0e99ed8d2b1 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -2746,6 +2746,9 @@ skip_usbdev:
> > exit(-ERROR_FAIL);
> > }
> > }
> > +libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
> > +xlu_cfg_get_defbool(config, "ffa_enabled",
> > +_info->arch_arm.ffa_enabled, 0);
>
> This option needs to be documented in docs/man/xl.cfg.5.pod.in.
OK
>
> >
> > parse_vkb_list(config, d_config);
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index be9eff014120..e57e1d3757e2 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -139,6 +139,17 @@ config TEE
> >
> > source "arch/arm/tee/Kconfig"
> >
> > +config FFA
> > + bool "Enable FF-A mediator support" if EXPERT
> > + default n
> > + depends on ARM_64
> > + help
> > + This option enables a minimal FF-A mediator. The mediator is
> > + generic as it follows the FF-A specification [1], but it only
> > + implements a small subset of the specification.
>
> Where would a user be able to find which subset of the specification
> that Xen implements?
I'll add a bit
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Julien,
On Fri, Jul 08, 2022 at 02:40:56PM +0100, Julien Grall wrote:
> Hi Jens,
>
> I haven't checked whether the FFA driver is complaint with the spec. I
> mainly checked whether the code makes sense from Xen PoV.
>
> This is a fairly long patch to review. So I will split my review in multiple
> session/e-mail.
Thanks for the comments. It's going to take a bit longer than I first thought
to go through and respond. I'll get back once I'm though it.
Thanks,
Jens
>
> On 22/06/2022 14:42, Jens Wiklander wrote:
> > Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> > Partition in secure world.
> >
> > The implementation is the bare minimum to be able to communicate with
> > OP-TEE running as an SPMC at S-EL1.
> >
> > This is loosely based on the TEE mediator framework and the OP-TEE
> > mediator.
> >
> > [1] https://developer.arm.com/documentation/den0077/latest
> > Signed-off-by: Jens Wiklander
> > ---
> > SUPPORT.md|7 +
> > tools/libs/light/libxl_arm.c |3 +
> > tools/libs/light/libxl_types.idl |1 +
> > tools/xl/xl_parse.c |3 +
>
> These changes would need an ack from the toolstack maintainers.
>
> > xen/arch/arm/Kconfig | 11 +
> > xen/arch/arm/Makefile |1 +
> > xen/arch/arm/domain.c | 10 +
> > xen/arch/arm/domain_build.c |1 +
> > xen/arch/arm/ffa.c| 1683 +
> > xen/arch/arm/include/asm/domain.h |4 +
> > xen/arch/arm/include/asm/ffa.h| 71 ++
> > xen/arch/arm/vsmc.c | 17 +-
> > xen/include/public/arch-arm.h |2 +
> > 13 files changed, 1811 insertions(+), 3 deletions(-)
> > create mode 100644 xen/arch/arm/ffa.c
> > create mode 100644 xen/arch/arm/include/asm/ffa.h
> >
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index 70e98964cbc0..215bb3c9043b 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
> > No support for QEMU backends in a 16K or 64K domain.
> > +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> > +
> > +Status, Arm64: Tech Preview
> > +
> > +There are still some code paths where a vCPU may hog a pCPU longer than
> > +necessary. The FF-A mediator is not yet implemented for Arm32.
> > +
> > ### ARM: Guest Device Tree support
> > Status: Supported
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index eef1de093914..a985609861c7 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > return ERROR_FAIL;
> > }
> > +config->arch.ffa_enabled =
> > +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> > +
> > return 0;
> > }
> > diff --git a/tools/libs/light/libxl_types.idl
> > b/tools/libs/light/libxl_types.idl
> > index 2a42da2f7d78..bf4544bef399 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > ("vuart", libxl_vuart_type),
> > + ("ffa_enabled", libxl_defbool),
>
> This needs to be accompagnied with a define LIBXL_HAVE_* in
> tools/include/libxl.h. Please see the examples in the header.
>
> > ])),
> > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> > ])),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index b98c0de378b6..e0e99ed8d2b1 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -2746,6 +2746,9 @@ skip_usbdev:
> > exit(-ERROR_FAIL);
> > }
> > }
> > +libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
> > +xlu_cfg_get_defbool(config, "ffa_enabled",
> > +_info->arch_arm.ffa_enabled, 0);
>
> This option needs to be documented in docs/man/xl.cfg.5.pod.in.
>
> > parse_vkb_list(config, d_config);
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index be9eff014120..e57e1d3757e2 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -139,6 +139,17 @@ config TEE
> > source "arch/arm/tee/Kconfig"
> > +config FFA
> > + bool "Enable FF-A mediator support" if EXPERT
> > + default n
> > + depends on ARM_64
> > + help
> > + This option enables a minimal FF-A mediator. The mediator is
> > + generic as it follows the FF-A specification [1], but it only
> > + implements a small subset of the specification.
>
> Where would a user be able to find which subset of the specification that
> Xen implements?
>
> > +
> > + [1]
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Jens,
> On 22 Jun 2022, at 14:42, Jens Wiklander wrote:
>
> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> Partition in secure world.
>
> The implementation is the bare minimum to be able to communicate with
> OP-TEE running as an SPMC at S-EL1.
>
> This is loosely based on the TEE mediator framework and the OP-TEE
> mediator.
>
> [1] https://developer.arm.com/documentation/den0077/latest
> Signed-off-by: Jens Wiklander
I spent quite some time on this patch and on the spec and there are far to
much code and concepts introduced here to be able to do a review in one go.
Could you try to split the patch to introduce each concept in a specific patch ?
I would suggest something like introducing each call in its own patch, having
a specific patch for the tool support, etc.
At this stage I am not convinced that there is no issue here where a guest could
access information from an other guest and reviewing smaller patches will help
me following the spec for each subject and ask questions along the way.
Cheers
Bertrand
> ---
> SUPPORT.md|7 +
> tools/libs/light/libxl_arm.c |3 +
> tools/libs/light/libxl_types.idl |1 +
> tools/xl/xl_parse.c |3 +
> xen/arch/arm/Kconfig | 11 +
> xen/arch/arm/Makefile |1 +
> xen/arch/arm/domain.c | 10 +
> xen/arch/arm/domain_build.c |1 +
> xen/arch/arm/ffa.c| 1683 +
> xen/arch/arm/include/asm/domain.h |4 +
> xen/arch/arm/include/asm/ffa.h| 71 ++
> xen/arch/arm/vsmc.c | 17 +-
> xen/include/public/arch-arm.h |2 +
> 13 files changed, 1811 insertions(+), 3 deletions(-)
> create mode 100644 xen/arch/arm/ffa.c
> create mode 100644 xen/arch/arm/include/asm/ffa.h
>
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 70e98964cbc0..215bb3c9043b 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
>
> No support for QEMU backends in a 16K or 64K domain.
>
> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> +
> +Status, Arm64: Tech Preview
> +
> +There are still some code paths where a vCPU may hog a pCPU longer than
> +necessary. The FF-A mediator is not yet implemented for Arm32.
> +
> ### ARM: Guest Device Tree support
>
> Status: Supported
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de093914..a985609861c7 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> return ERROR_FAIL;
> }
>
> +config->arch.ffa_enabled =
> +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> +
> return 0;
> }
>
> diff --git a/tools/libs/light/libxl_types.idl
> b/tools/libs/light/libxl_types.idl
> index 2a42da2f7d78..bf4544bef399 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>("vuart", libxl_vuart_type),
> + ("ffa_enabled", libxl_defbool),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index b98c0de378b6..e0e99ed8d2b1 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2746,6 +2746,9 @@ skip_usbdev:
> exit(-ERROR_FAIL);
> }
> }
> +libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
> +xlu_cfg_get_defbool(config, "ffa_enabled",
> +_info->arch_arm.ffa_enabled, 0);
>
> parse_vkb_list(config, d_config);
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index be9eff014120..e57e1d3757e2 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -139,6 +139,17 @@ config TEE
>
> source "arch/arm/tee/Kconfig"
>
> +config FFA
> + bool "Enable FF-A mediator support" if EXPERT
> + default n
> + depends on ARM_64
> + help
> + This option enables a minimal FF-A mediator. The mediator is
> + generic as it follows the FF-A specification [1], but it only
> + implements a small subset of the specification.
> +
> + [1] https://developer.arm.com/documentation/den0077/latest
> +
> endmenu
>
> menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index bb7a6151c13c..af0c69f793d4 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -20,6 +20,7 @@ obj-y += domain_build.init.o
> obj-y += domctl.o
> obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> obj-y += efi/
> +obj-$(CONFIG_FFA) += ffa.o
> obj-y += gic.o
> obj-y
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Jens,
Just to let you know, I am currently going through the spec in order to
properly review your implementation but this might take a bit of time as it is
quite long :-)
Cheers
Bertrand
> On 22 Jun 2022, at 14:42, Jens Wiklander wrote:
>
> Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
> Partition in secure world.
>
> The implementation is the bare minimum to be able to communicate with
> OP-TEE running as an SPMC at S-EL1.
>
> This is loosely based on the TEE mediator framework and the OP-TEE
> mediator.
>
> [1] https://developer.arm.com/documentation/den0077/latest
> Signed-off-by: Jens Wiklander
> ---
> SUPPORT.md|7 +
> tools/libs/light/libxl_arm.c |3 +
> tools/libs/light/libxl_types.idl |1 +
> tools/xl/xl_parse.c |3 +
> xen/arch/arm/Kconfig | 11 +
> xen/arch/arm/Makefile |1 +
> xen/arch/arm/domain.c | 10 +
> xen/arch/arm/domain_build.c |1 +
> xen/arch/arm/ffa.c| 1683 +
> xen/arch/arm/include/asm/domain.h |4 +
> xen/arch/arm/include/asm/ffa.h| 71 ++
> xen/arch/arm/vsmc.c | 17 +-
> xen/include/public/arch-arm.h |2 +
> 13 files changed, 1811 insertions(+), 3 deletions(-)
> create mode 100644 xen/arch/arm/ffa.c
> create mode 100644 xen/arch/arm/include/asm/ffa.h
>
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 70e98964cbc0..215bb3c9043b 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
>
> No support for QEMU backends in a 16K or 64K domain.
>
> +### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
> +
> +Status, Arm64: Tech Preview
> +
> +There are still some code paths where a vCPU may hog a pCPU longer than
> +necessary. The FF-A mediator is not yet implemented for Arm32.
> +
> ### ARM: Guest Device Tree support
>
> Status: Supported
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de093914..a985609861c7 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> return ERROR_FAIL;
> }
>
> +config->arch.ffa_enabled =
> +libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
> +
> return 0;
> }
>
> diff --git a/tools/libs/light/libxl_types.idl
> b/tools/libs/light/libxl_types.idl
> index 2a42da2f7d78..bf4544bef399 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>("vuart", libxl_vuart_type),
> + ("ffa_enabled", libxl_defbool),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index b98c0de378b6..e0e99ed8d2b1 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2746,6 +2746,9 @@ skip_usbdev:
> exit(-ERROR_FAIL);
> }
> }
> +libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
> +xlu_cfg_get_defbool(config, "ffa_enabled",
> +_info->arch_arm.ffa_enabled, 0);
>
> parse_vkb_list(config, d_config);
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index be9eff014120..e57e1d3757e2 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -139,6 +139,17 @@ config TEE
>
> source "arch/arm/tee/Kconfig"
>
> +config FFA
> + bool "Enable FF-A mediator support" if EXPERT
> + default n
> + depends on ARM_64
> + help
> + This option enables a minimal FF-A mediator. The mediator is
> + generic as it follows the FF-A specification [1], but it only
> + implements a small subset of the specification.
> +
> + [1] https://developer.arm.com/documentation/den0077/latest
> +
> endmenu
>
> menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index bb7a6151c13c..af0c69f793d4 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -20,6 +20,7 @@ obj-y += domain_build.init.o
> obj-y += domctl.o
> obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> obj-y += efi/
> +obj-$(CONFIG_FFA) += ffa.o
> obj-y += gic.o
> obj-y += gic-v2.o
> obj-$(CONFIG_GICV3) += gic-v3.o
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 8110c1df8638..a3f00e7e234d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -756,6 +757,9 @@ int arch_domain_create(struct domain *d,
> if ( (rc = tee_domain_init(d,
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Jens,
This is the second part of the review.
On 22/06/2022 14:42, Jens Wiklander wrote:
+static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
+ struct ffa_address_range *range, uint32_t range_count,
AFAICT, 'range' is not meant to be modified. So I would add 'const'.
+ unsigned int start_page_idx,
+ unsigned int *last_page_idx)
+{
+unsigned int pg_idx = start_page_idx;
+unsigned long gfn;
Below you are using gaddr_to_gfn() which will return gfn_t. This is
using the typesafe infrastructure: gfn_t will be a structure with
CONFIG_DEBUG=y to allow type checking and a plain 'unsigned long' when
CONFIG_DEBUG=n.
Please make sure to test build with CONFIG_DEBUG=y.
As a side note, I would suggest to try booting as CONFIG_DEBUG as it
enables extra check for the common pitfalls.
+unsigned int n;
+unsigned int m;
+p2m_type_t t;
+uint64_t addr;
+
+for ( n = 0; n < range_count; n++ )
+{
+for ( m = 0; m < range[n].page_count; m++ )
+{
+if ( pg_idx >= shm->page_count )
+return FFA_RET_INVALID_PARAMETERS;
Shouldn't we call put_page() to drop the references taken by
get_page_from_gfn()?
+
+addr = read_atomic([n].address);
IIUC, range is part of the shared page with the guest. Where do you
check that all the access will be within the shared page?
+gfn = gaddr_to_gfn(addr + m * PAGE_SIZE);
Is addr meant to be page-aligned? Also, you are using the hypervisor
page size here when AFAICT page_count is provided by the domain.
How do you guarantee that both Xen and the domain agree on the page size?
+shm->pages[pg_idx] = get_page_from_gfn(d, gfn, , P2M_ALLOC);
+if ( !shm->pages[pg_idx] )
+return FFA_RET_DENIED;
+pg_idx++;
+/* Only normal RAM for now */
Similar to my earlier remark, the comment doesn't match the check below.
+if ( t != p2m_ram_rw )
+return FFA_RET_DENIED;
+}
+}
+
+*last_page_idx = pg_idx;
+
+return FFA_RET_OK;
+}
+
+static void put_shm_pages(struct ffa_shm_mem *shm)
+{
+unsigned int n;
+
+for ( n = 0; n < shm->page_count && shm->pages[n]; n++ )
+{
+put_page(shm->pages[n]);
+shm->pages[n] = NULL;
+}
+}
+
+static void init_range(struct ffa_address_range *addr_range,
+ paddr_t pa) > +{
+memset(addr_range, 0, sizeof(*addr_range));
+addr_range->address = pa;
+addr_range->page_count = 1;
+}
+
+static int share_shm(struct ffa_shm_mem *shm)
AFAIU, share_shm() cannot be concurrently called. You seem to use
ffa_buffer_lock to guarantee that. So I would suggest to add:
1) an ASSERT(spin_is_Locked(_buffer_lock))
2) a comment on top of share_shm() explaining that the function
should be called with ffa_buffer_lock taken.
+{
+uint32_t max_frag_len = ffa_page_count * PAGE_SIZE;
+struct ffa_mem_transaction_1_1 *descr = ffa_tx;
+struct ffa_mem_access *mem_access_array;
+struct ffa_mem_region *region_descr;
+struct ffa_address_range *addr_range;
+paddr_t pa;
+paddr_t last_pa;
+unsigned int n;
+uint32_t frag_len;
+uint32_t tot_len;
+int ret;
+unsigned int range_count;
+unsigned int range_base;
+bool first;
+
+memset(descr, 0, sizeof(*descr));
+descr->sender_id = shm->sender_id;
+descr->global_handle = shm->handle;
+descr->mem_reg_attr = FFA_NORMAL_MEM_REG_ATTR;
+descr->mem_access_count = 1;
+descr->mem_access_size = sizeof(*mem_access_array);
+descr->mem_access_offs = sizeof(*descr);
+mem_access_array = (void *)(descr + 1);
+region_descr = (void *)(mem_access_array + 1);
The (void *)(descr + 1) seems to be very common in your comment. Can you
consider to add a wrapper? This will make easier to read the code?
+
+memset(mem_access_array, 0, sizeof(*mem_access_array));
+mem_access_array[0].access_perm.endpoint_id = shm->ep_id;
+mem_access_array[0].access_perm.perm = FFA_MEM_ACC_RW;
+mem_access_array[0].region_offs = (vaddr_t)region_descr - (vaddr_t)ffa_tx;
Same for calculating the offset.
+
+memset(region_descr, 0, sizeof(*region_descr));
+region_descr->total_page_count = shm->page_count;
+
+region_descr->address_range_count = 1;
+last_pa = page_to_maddr(shm->pages[0]);
For hardening purpose, I would suggest to check if shm->page_count is at
least 1. If you think this could be a programming error then you could
write:
if ( )
{
ASSERT_UNREACHABLE()
return ;
}
+for ( n = 1; n < shm->page_count; last_pa = pa, n++ )
+{
+pa = page_to_maddr(shm->pages[n]);
+if ( last_pa + PAGE_SIZE == pa )
+{
Coding style: We usually avoid {} for single line.
+continue;
+}
+region_descr->address_range_count++;
+}
+
+tot_len =
Re: [PATCH v4 2/2] xen/arm: add FF-A mediator
Hi Jens,
I haven't checked whether the FFA driver is complaint with the spec. I
mainly checked whether the code makes sense from Xen PoV.
This is a fairly long patch to review. So I will split my review in
multiple session/e-mail.
On 22/06/2022 14:42, Jens Wiklander wrote:
Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.
The implementation is the bare minimum to be able to communicate with
OP-TEE running as an SPMC at S-EL1.
This is loosely based on the TEE mediator framework and the OP-TEE
mediator.
[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander
---
SUPPORT.md|7 +
tools/libs/light/libxl_arm.c |3 +
tools/libs/light/libxl_types.idl |1 +
tools/xl/xl_parse.c |3 +
These changes would need an ack from the toolstack maintainers.
xen/arch/arm/Kconfig | 11 +
xen/arch/arm/Makefile |1 +
xen/arch/arm/domain.c | 10 +
xen/arch/arm/domain_build.c |1 +
xen/arch/arm/ffa.c| 1683 +
xen/arch/arm/include/asm/domain.h |4 +
xen/arch/arm/include/asm/ffa.h| 71 ++
xen/arch/arm/vsmc.c | 17 +-
xen/include/public/arch-arm.h |2 +
13 files changed, 1811 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/ffa.c
create mode 100644 xen/arch/arm/include/asm/ffa.h
diff --git a/SUPPORT.md b/SUPPORT.md
index 70e98964cbc0..215bb3c9043b 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
No support for QEMU backends in a 16K or 64K domain.
+### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
+
+Status, Arm64: Tech Preview
+
+There are still some code paths where a vCPU may hog a pCPU longer than
+necessary. The FF-A mediator is not yet implemented for Arm32.
+
### ARM: Guest Device Tree support
Status: Supported
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de093914..a985609861c7 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
return ERROR_FAIL;
}
+config->arch.ffa_enabled =
+libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
+
return 0;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d78..bf4544bef399 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
+ ("ffa_enabled", libxl_defbool),
This needs to be accompagnied with a define LIBXL_HAVE_* in
tools/include/libxl.h. Please see the examples in the header.
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b98c0de378b6..e0e99ed8d2b1 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2746,6 +2746,9 @@ skip_usbdev:
exit(-ERROR_FAIL);
}
}
+libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
+xlu_cfg_get_defbool(config, "ffa_enabled",
+_info->arch_arm.ffa_enabled, 0);
This option needs to be documented in docs/man/xl.cfg.5.pod.in.
parse_vkb_list(config, d_config);
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index be9eff014120..e57e1d3757e2 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -139,6 +139,17 @@ config TEE
source "arch/arm/tee/Kconfig"
+config FFA
+ bool "Enable FF-A mediator support" if EXPERT
+ default n
+ depends on ARM_64
+ help
+ This option enables a minimal FF-A mediator. The mediator is
+ generic as it follows the FF-A specification [1], but it only
+ implements a small subset of the specification.
Where would a user be able to find which subset of the specification
that Xen implements?
+
+ [1] https://developer.arm.com/documentation/den0077/latest
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index bb7a6151c13c..af0c69f793d4 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -20,6 +20,7 @@ obj-y += domain_build.init.o
obj-y += domctl.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += efi/
+obj-$(CONFIG_FFA) += ffa.o
obj-y += gic.o
obj-y += gic-v2.o
obj-$(CONFIG_GICV3) += gic-v3.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df8638..a3f00e7e234d 100644
[PATCH v4 2/2] xen/arm: add FF-A mediator
Adds a FF-A version 1.1 [1] mediator to communicate with a Secure
Partition in secure world.
The implementation is the bare minimum to be able to communicate with
OP-TEE running as an SPMC at S-EL1.
This is loosely based on the TEE mediator framework and the OP-TEE
mediator.
[1] https://developer.arm.com/documentation/den0077/latest
Signed-off-by: Jens Wiklander
---
SUPPORT.md|7 +
tools/libs/light/libxl_arm.c |3 +
tools/libs/light/libxl_types.idl |1 +
tools/xl/xl_parse.c |3 +
xen/arch/arm/Kconfig | 11 +
xen/arch/arm/Makefile |1 +
xen/arch/arm/domain.c | 10 +
xen/arch/arm/domain_build.c |1 +
xen/arch/arm/ffa.c| 1683 +
xen/arch/arm/include/asm/domain.h |4 +
xen/arch/arm/include/asm/ffa.h| 71 ++
xen/arch/arm/vsmc.c | 17 +-
xen/include/public/arch-arm.h |2 +
13 files changed, 1811 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/arm/ffa.c
create mode 100644 xen/arch/arm/include/asm/ffa.h
diff --git a/SUPPORT.md b/SUPPORT.md
index 70e98964cbc0..215bb3c9043b 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -785,6 +785,13 @@ that covers the DMA of the device to be passed through.
No support for QEMU backends in a 16K or 64K domain.
+### ARM: Firmware Framework for Arm A-profile (FF-A) Mediator
+
+Status, Arm64: Tech Preview
+
+There are still some code paths where a vCPU may hog a pCPU longer than
+necessary. The FF-A mediator is not yet implemented for Arm32.
+
### ARM: Guest Device Tree support
Status: Supported
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de093914..a985609861c7 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -101,6 +101,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
return ERROR_FAIL;
}
+config->arch.ffa_enabled =
+libxl_defbool_val(d_config->b_info.arch_arm.ffa_enabled);
+
return 0;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d78..bf4544bef399 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -646,6 +646,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
+ ("ffa_enabled", libxl_defbool),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b98c0de378b6..e0e99ed8d2b1 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2746,6 +2746,9 @@ skip_usbdev:
exit(-ERROR_FAIL);
}
}
+libxl_defbool_setdefault(_info->arch_arm.ffa_enabled, false);
+xlu_cfg_get_defbool(config, "ffa_enabled",
+_info->arch_arm.ffa_enabled, 0);
parse_vkb_list(config, d_config);
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index be9eff014120..e57e1d3757e2 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -139,6 +139,17 @@ config TEE
source "arch/arm/tee/Kconfig"
+config FFA
+ bool "Enable FF-A mediator support" if EXPERT
+ default n
+ depends on ARM_64
+ help
+ This option enables a minimal FF-A mediator. The mediator is
+ generic as it follows the FF-A specification [1], but it only
+ implements a small subset of the specification.
+
+ [1] https://developer.arm.com/documentation/den0077/latest
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index bb7a6151c13c..af0c69f793d4 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -20,6 +20,7 @@ obj-y += domain_build.init.o
obj-y += domctl.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += efi/
+obj-$(CONFIG_FFA) += ffa.o
obj-y += gic.o
obj-y += gic-v2.o
obj-$(CONFIG_GICV3) += gic-v3.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df8638..a3f00e7e234d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -756,6 +757,9 @@ int arch_domain_create(struct domain *d,
if ( (rc = tee_domain_init(d, config->arch.tee_type)) != 0 )
goto fail;
+if ( (rc = ffa_domain_init(d, config->arch.ffa_enabled)) != 0 )
+goto fail;
+
update_domain_wallclock_time(d);
/*
@@ -998,6 +1002,7 @@ static int relinquish_memory(struct domain *d, struct
page_list_head *list)
enum {
PROG_pci = 1,
PROG_tee,
+PROG_ffa,
PROG_xen,
PROG_page,
PROG_mapping,
@@ -1043,6 +1048,11 @@ int
