| Message ID | c553f850d76f4c31f5ce464f39bcb010722b9f99.1677079672.git.jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen FF-A mediator | expand |
Hi Jens, > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > When initializing the FF-A mediator map the RX and TX buffers shared with > the SPMC. > > These buffer are later used to to transmit data that cannot be passed in > registers only. > > Adds a check that the SP supports the needed FF-A features > FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we > must use FFA_RXTX_MAP_64 since registers are used to transmit the > physical addresses of the RX/TX buffers. Right now, FFA on 32bit would only work correctly if LPAE is not used and only addresses under 4G are used by Xen and by guests as addresses are transferred through a single register. I think that we need for now to only enable FFA support on 64bit as the limitations we would need to enforce on 32bit are complex and the use case for FFA on 32bit platforms is not that obvious now. > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > index a5d8a12635b6..07dd5c36d54b 100644 > --- a/xen/arch/arm/tee/ffa.c > +++ b/xen/arch/arm/tee/ffa.c > @@ -148,6 +148,15 @@ struct ffa_ctx { > /* Negotiated FF-A version to use with the SPMC */ > static uint32_t ffa_version __ro_after_init; > > +/* > + * Our rx/tx buffers shared with the SPMC. > + * > + * ffa_page_count is the number of pages used in each of these buffers. > + */ > +static void *ffa_rx __read_mostly; > +static void *ffa_tx __read_mostly; > +static unsigned int ffa_page_count __read_mostly; > + > static bool ffa_get_version(uint32_t *vers) > { > const struct arm_smccc_1_2_regs arg = { > @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id) > return !ret; > } > > +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > + uint32_t page_count) Using register_t type here is doing an implicit cast when called and on 32bit this might later remove part of the address. This function must take paddr_t as parameters. > +{ > + uint32_t fid = FFA_RXTX_MAP_32; > + > + if ( IS_ENABLED(CONFIG_ARM_64) ) > + fid = FFA_RXTX_MAP_64; > + > + return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0); simple call might not be suitable on 32bits due to the conversion. As said earlier, it would make more sense to disable FFA on 32bit and put some comments/build_bug_on in the code in places where there would be something to fix. > +} > + > static uint16_t get_vm_id(const struct domain *d) > { > /* +1 since 0 is reserved for the hypervisor in FF-A */ > @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d) > static bool ffa_probe(void) > { > uint32_t vers; > + int e; > unsigned int major_vers; > unsigned int minor_vers; > > @@ -426,12 +447,46 @@ static bool ffa_probe(void) > printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > major_vers, minor_vers); > > - if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > + if ( > +#ifdef CONFIG_ARM_64 > + !check_mandatory_feature(FFA_RXTX_MAP_64) || > +#endif > +#ifdef CONFIG_ARM_32 > + !check_mandatory_feature(FFA_RXTX_MAP_32) || > +#endif > + !check_mandatory_feature(FFA_RXTX_UNMAP) || > + !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > return false; > > + ffa_rx = alloc_xenheap_pages(0, 0); > + if ( !ffa_rx ) > + return false; > + > + ffa_tx = alloc_xenheap_pages(0, 0); > + if ( !ffa_tx ) > + goto err_free_ffa_rx; > + > + e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1); > + if ( e ) > + { > + printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e); > + goto err_free_ffa_tx; > + } > + ffa_page_count = 1; ffa_page_count is a constant here and is not used to do the allocation or passed as parameter to ffa_rxtx_map. Do you expect this value to be modified ? how ? Please set it first and use it for allocation and as parameter to rxtx_map so that a modification of the value would only have to be done in one place. Please use a define if this is a constant. As it is a global variable, does the parameter to rxtx_map make sense ? Cheers Bertrand > ffa_version = vers; > > return true; > + > +err_free_ffa_tx: > + free_xenheap_pages(ffa_tx, 0); > + ffa_tx = NULL; > +err_free_ffa_rx: > + free_xenheap_pages(ffa_rx, 0); > + ffa_rx = NULL; > + ffa_page_count = 0; > + ffa_version = 0; > + > + return false; > } > > static const struct tee_mediator_ops ffa_ops = > -- > 2.34.1 >
Hi Bertrand, On Tue, Feb 28, 2023 at 1:57 PM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Jens, > > > On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > When initializing the FF-A mediator map the RX and TX buffers shared with > > the SPMC. > > > > These buffer are later used to to transmit data that cannot be passed in > > registers only. > > > > Adds a check that the SP supports the needed FF-A features > > FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we > > must use FFA_RXTX_MAP_64 since registers are used to transmit the > > physical addresses of the RX/TX buffers. > > Right now, FFA on 32bit would only work correctly if LPAE is not used and only addresses > under 4G are used by Xen and by guests as addresses are transferred through a single register. > > I think that we need for now to only enable FFA support on 64bit as the limitations we > would need to enforce on 32bit are complex and the use case for FFA on 32bit platforms > is not that obvious now. OK, I'll drop the #ifdef CONFIG_ARM_64 and #ifdef CONFIG_ARM_32 and instead depend on ARM_64 in Kconfig. If we ever want to use this on ARM_32 we'll have to go through everything anyway. > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > > --- > > xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > > index a5d8a12635b6..07dd5c36d54b 100644 > > --- a/xen/arch/arm/tee/ffa.c > > +++ b/xen/arch/arm/tee/ffa.c > > @@ -148,6 +148,15 @@ struct ffa_ctx { > > /* Negotiated FF-A version to use with the SPMC */ > > static uint32_t ffa_version __ro_after_init; > > > > +/* > > + * Our rx/tx buffers shared with the SPMC. > > + * > > + * ffa_page_count is the number of pages used in each of these buffers. > > + */ > > +static void *ffa_rx __read_mostly; > > +static void *ffa_tx __read_mostly; > > +static unsigned int ffa_page_count __read_mostly; > > + > > static bool ffa_get_version(uint32_t *vers) > > { > > const struct arm_smccc_1_2_regs arg = { > > @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id) > > return !ret; > > } > > > > +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > > + uint32_t page_count) > > Using register_t type here is doing an implicit cast when called and on > 32bit this might later remove part of the address. > This function must take paddr_t as parameters. I'll change to paddr_t for rx/tx. > > > +{ > > + uint32_t fid = FFA_RXTX_MAP_32; > > + > > + if ( IS_ENABLED(CONFIG_ARM_64) ) > > + fid = FFA_RXTX_MAP_64; > > + > > + return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0); > > simple call might not be suitable on 32bits due to the conversion. > As said earlier, it would make more sense to disable FFA on 32bit and > put some comments/build_bug_on in the code in places where there > would be something to fix. I'm dropping the 32-bit support. > > > +} > > + > > static uint16_t get_vm_id(const struct domain *d) > > { > > /* +1 since 0 is reserved for the hypervisor in FF-A */ > > @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d) > > static bool ffa_probe(void) > > { > > uint32_t vers; > > + int e; > > unsigned int major_vers; > > unsigned int minor_vers; > > > > @@ -426,12 +447,46 @@ static bool ffa_probe(void) > > printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > > major_vers, minor_vers); > > > > - if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > > + if ( > > +#ifdef CONFIG_ARM_64 > > + !check_mandatory_feature(FFA_RXTX_MAP_64) || > > +#endif > > +#ifdef CONFIG_ARM_32 > > + !check_mandatory_feature(FFA_RXTX_MAP_32) || > > +#endif > > + !check_mandatory_feature(FFA_RXTX_UNMAP) || > > + !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > > return false; > > > > + ffa_rx = alloc_xenheap_pages(0, 0); > > + if ( !ffa_rx ) > > + return false; > > + > > + ffa_tx = alloc_xenheap_pages(0, 0); > > + if ( !ffa_tx ) > > + goto err_free_ffa_rx; > > + > > + e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1); > > + if ( e ) > > + { > > + printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e); > > + goto err_free_ffa_tx; > > + } > > + ffa_page_count = 1; > > ffa_page_count is a constant here and is not used to do the allocation or > passed as parameter to ffa_rxtx_map. > > Do you expect this value to be modified ? how ? I expect this value to match how many FFA_PAGE_SIZE pages have been mapped for the RX/TX buffers. Currently, this is only 1 but will have to be changed later if PAGE_SIZE in Xen or in the secure world is larger than FFA_PAGE_SIZE. We may also later add support configurations where RX/TX buffers aren't mapped. > > Please set it first and use it for allocation and as parameter to rxtx_map so > that a modification of the value would only have to be done in one place. > > Please use a define if this is a constant. How about adding a define FFA_MIN_RXTX_PAGE_COUNT and giving that to ffa_rxtx_map() and later assigning it to ffa_page_count if the call succeeds? > > As it is a global variable, does the parameter to rxtx_map make sense ? Yes, ffa_rxtx_map() is a dumb wrapper so it should have all the needed parameters for the SMC provided. Cheers, Jens > > Cheers > Bertrand > > > ffa_version = vers; > > > > return true; > > + > > +err_free_ffa_tx: > > + free_xenheap_pages(ffa_tx, 0); > > + ffa_tx = NULL; > > +err_free_ffa_rx: > > + free_xenheap_pages(ffa_rx, 0); > > + ffa_rx = NULL; > > + ffa_page_count = 0; > > + ffa_version = 0; > > + > > + return false; > > } > > > > static const struct tee_mediator_ops ffa_ops = > > -- > > 2.34.1 > > >
Hi Jens, > On 1 Mar 2023, at 10:30, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Bertrand, > > On Tue, Feb 28, 2023 at 1:57 PM Bertrand Marquis > <Bertrand.Marquis@arm.com> wrote: >> >> Hi Jens, >> >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: >>> >>> When initializing the FF-A mediator map the RX and TX buffers shared with >>> the SPMC. >>> >>> These buffer are later used to to transmit data that cannot be passed in >>> registers only. >>> >>> Adds a check that the SP supports the needed FF-A features >>> FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we >>> must use FFA_RXTX_MAP_64 since registers are used to transmit the >>> physical addresses of the RX/TX buffers. >> >> Right now, FFA on 32bit would only work correctly if LPAE is not used and only addresses >> under 4G are used by Xen and by guests as addresses are transferred through a single register. >> >> I think that we need for now to only enable FFA support on 64bit as the limitations we >> would need to enforce on 32bit are complex and the use case for FFA on 32bit platforms >> is not that obvious now. > > OK, I'll drop the #ifdef CONFIG_ARM_64 and #ifdef CONFIG_ARM_32 and > instead depend on ARM_64 in Kconfig. > If we ever want to use this on ARM_32 we'll have to go through > everything anyway. Yes this is the best solution for now. And support.md patch is already saying already arm64. > >> >>> >>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >>> --- >>> xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 56 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>> index a5d8a12635b6..07dd5c36d54b 100644 >>> --- a/xen/arch/arm/tee/ffa.c >>> +++ b/xen/arch/arm/tee/ffa.c >>> @@ -148,6 +148,15 @@ struct ffa_ctx { >>> /* Negotiated FF-A version to use with the SPMC */ >>> static uint32_t ffa_version __ro_after_init; >>> >>> +/* >>> + * Our rx/tx buffers shared with the SPMC. >>> + * >>> + * ffa_page_count is the number of pages used in each of these buffers. >>> + */ >>> +static void *ffa_rx __read_mostly; >>> +static void *ffa_tx __read_mostly; >>> +static unsigned int ffa_page_count __read_mostly; >>> + >>> static bool ffa_get_version(uint32_t *vers) >>> { >>> const struct arm_smccc_1_2_regs arg = { >>> @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id) >>> return !ret; >>> } >>> >>> +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, >>> + uint32_t page_count) >> >> Using register_t type here is doing an implicit cast when called and on >> 32bit this might later remove part of the address. >> This function must take paddr_t as parameters. > > I'll change to paddr_t for rx/tx. > >> >>> +{ >>> + uint32_t fid = FFA_RXTX_MAP_32; >>> + >>> + if ( IS_ENABLED(CONFIG_ARM_64) ) >>> + fid = FFA_RXTX_MAP_64; >>> + >>> + return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0); >> >> simple call might not be suitable on 32bits due to the conversion. >> As said earlier, it would make more sense to disable FFA on 32bit and >> put some comments/build_bug_on in the code in places where there >> would be something to fix. > > I'm dropping the 32-bit support. > >> >>> +} >>> + >>> static uint16_t get_vm_id(const struct domain *d) >>> { >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ >>> @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d) >>> static bool ffa_probe(void) >>> { >>> uint32_t vers; >>> + int e; >>> unsigned int major_vers; >>> unsigned int minor_vers; >>> >>> @@ -426,12 +447,46 @@ static bool ffa_probe(void) >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", >>> major_vers, minor_vers); >>> >>> - if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) >>> + if ( >>> +#ifdef CONFIG_ARM_64 >>> + !check_mandatory_feature(FFA_RXTX_MAP_64) || >>> +#endif >>> +#ifdef CONFIG_ARM_32 >>> + !check_mandatory_feature(FFA_RXTX_MAP_32) || >>> +#endif >>> + !check_mandatory_feature(FFA_RXTX_UNMAP) || >>> + !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) >>> return false; >>> >>> + ffa_rx = alloc_xenheap_pages(0, 0); >>> + if ( !ffa_rx ) >>> + return false; >>> + >>> + ffa_tx = alloc_xenheap_pages(0, 0); >>> + if ( !ffa_tx ) >>> + goto err_free_ffa_rx; >>> + >>> + e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1); >>> + if ( e ) >>> + { >>> + printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e); >>> + goto err_free_ffa_tx; >>> + } >>> + ffa_page_count = 1; >> >> ffa_page_count is a constant here and is not used to do the allocation or >> passed as parameter to ffa_rxtx_map. >> >> Do you expect this value to be modified ? how ? > > I expect this value to match how many FFA_PAGE_SIZE pages have been > mapped for the RX/TX buffers. Currently, this is only 1 but will have > to be changed later if PAGE_SIZE in Xen or in the secure world is > larger than FFA_PAGE_SIZE. We may also later add support > configurations where RX/TX buffers aren't mapped. So it is a constant and the buffers are just mapped or not mapped. > >> >> Please set it first and use it for allocation and as parameter to rxtx_map so >> that a modification of the value would only have to be done in one place. >> >> Please use a define if this is a constant. > > How about adding a define FFA_MIN_RXTX_PAGE_COUNT and giving that to > ffa_rxtx_map() and later assigning it to ffa_page_count if the call > succeeds? Why MIN ? How about just using ffa_rx or ffa_tx being NULL or not to check if the buffers are mapped and remove the count. > >> >> As it is a global variable, does the parameter to rxtx_map make sense ? > > Yes, ffa_rxtx_map() is a dumb wrapper so it should have all the needed > parameters for the SMC provided. Then passing FFA_MIN_RXTX_PAGE_COUNT should be enough. Cheers Bertrand > > Cheers, > Jens > >> >> Cheers >> Bertrand >> >>> ffa_version = vers; >>> >>> return true; >>> + >>> +err_free_ffa_tx: >>> + free_xenheap_pages(ffa_tx, 0); >>> + ffa_tx = NULL; >>> +err_free_ffa_rx: >>> + free_xenheap_pages(ffa_rx, 0); >>> + ffa_rx = NULL; >>> + ffa_page_count = 0; >>> + ffa_version = 0; >>> + >>> + return false; >>> } >>> >>> static const struct tee_mediator_ops ffa_ops = >>> -- >>> 2.34.1
Hi, On Wed, Mar 1, 2023 at 10:56 AM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Jens, > > > On 1 Mar 2023, at 10:30, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > Hi Bertrand, > > > > On Tue, Feb 28, 2023 at 1:57 PM Bertrand Marquis > > <Bertrand.Marquis@arm.com> wrote: > >> > >> Hi Jens, > >> > >>> On 22 Feb 2023, at 16:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > >>> > >>> When initializing the FF-A mediator map the RX and TX buffers shared with > >>> the SPMC. > >>> > >>> These buffer are later used to to transmit data that cannot be passed in > >>> registers only. > >>> > >>> Adds a check that the SP supports the needed FF-A features > >>> FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we > >>> must use FFA_RXTX_MAP_64 since registers are used to transmit the > >>> physical addresses of the RX/TX buffers. > >> > >> Right now, FFA on 32bit would only work correctly if LPAE is not used and only addresses > >> under 4G are used by Xen and by guests as addresses are transferred through a single register. > >> > >> I think that we need for now to only enable FFA support on 64bit as the limitations we > >> would need to enforce on 32bit are complex and the use case for FFA on 32bit platforms > >> is not that obvious now. > > > > OK, I'll drop the #ifdef CONFIG_ARM_64 and #ifdef CONFIG_ARM_32 and > > instead depend on ARM_64 in Kconfig. > > If we ever want to use this on ARM_32 we'll have to go through > > everything anyway. > > Yes this is the best solution for now. > And support.md patch is already saying already arm64. > > > > >> > >>> > >>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > >>> --- > >>> xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 56 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >>> index a5d8a12635b6..07dd5c36d54b 100644 > >>> --- a/xen/arch/arm/tee/ffa.c > >>> +++ b/xen/arch/arm/tee/ffa.c > >>> @@ -148,6 +148,15 @@ struct ffa_ctx { > >>> /* Negotiated FF-A version to use with the SPMC */ > >>> static uint32_t ffa_version __ro_after_init; > >>> > >>> +/* > >>> + * Our rx/tx buffers shared with the SPMC. > >>> + * > >>> + * ffa_page_count is the number of pages used in each of these buffers. > >>> + */ > >>> +static void *ffa_rx __read_mostly; > >>> +static void *ffa_tx __read_mostly; > >>> +static unsigned int ffa_page_count __read_mostly; > >>> + > >>> static bool ffa_get_version(uint32_t *vers) > >>> { > >>> const struct arm_smccc_1_2_regs arg = { > >>> @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id) > >>> return !ret; > >>> } > >>> > >>> +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, > >>> + uint32_t page_count) > >> > >> Using register_t type here is doing an implicit cast when called and on > >> 32bit this might later remove part of the address. > >> This function must take paddr_t as parameters. > > > > I'll change to paddr_t for rx/tx. > > > >> > >>> +{ > >>> + uint32_t fid = FFA_RXTX_MAP_32; > >>> + > >>> + if ( IS_ENABLED(CONFIG_ARM_64) ) > >>> + fid = FFA_RXTX_MAP_64; > >>> + > >>> + return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0); > >> > >> simple call might not be suitable on 32bits due to the conversion. > >> As said earlier, it would make more sense to disable FFA on 32bit and > >> put some comments/build_bug_on in the code in places where there > >> would be something to fix. > > > > I'm dropping the 32-bit support. > > > >> > >>> +} > >>> + > >>> static uint16_t get_vm_id(const struct domain *d) > >>> { > >>> /* +1 since 0 is reserved for the hypervisor in FF-A */ > >>> @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d) > >>> static bool ffa_probe(void) > >>> { > >>> uint32_t vers; > >>> + int e; > >>> unsigned int major_vers; > >>> unsigned int minor_vers; > >>> > >>> @@ -426,12 +447,46 @@ static bool ffa_probe(void) > >>> printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > >>> major_vers, minor_vers); > >>> > >>> - if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > >>> + if ( > >>> +#ifdef CONFIG_ARM_64 > >>> + !check_mandatory_feature(FFA_RXTX_MAP_64) || > >>> +#endif > >>> +#ifdef CONFIG_ARM_32 > >>> + !check_mandatory_feature(FFA_RXTX_MAP_32) || > >>> +#endif > >>> + !check_mandatory_feature(FFA_RXTX_UNMAP) || > >>> + !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) > >>> return false; > >>> > >>> + ffa_rx = alloc_xenheap_pages(0, 0); > >>> + if ( !ffa_rx ) > >>> + return false; > >>> + > >>> + ffa_tx = alloc_xenheap_pages(0, 0); > >>> + if ( !ffa_tx ) > >>> + goto err_free_ffa_rx; > >>> + > >>> + e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1); > >>> + if ( e ) > >>> + { > >>> + printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e); > >>> + goto err_free_ffa_tx; > >>> + } > >>> + ffa_page_count = 1; > >> > >> ffa_page_count is a constant here and is not used to do the allocation or > >> passed as parameter to ffa_rxtx_map. > >> > >> Do you expect this value to be modified ? how ? > > > > I expect this value to match how many FFA_PAGE_SIZE pages have been > > mapped for the RX/TX buffers. Currently, this is only 1 but will have > > to be changed later if PAGE_SIZE in Xen or in the secure world is > > larger than FFA_PAGE_SIZE. We may also later add support > > configurations where RX/TX buffers aren't mapped. > > So it is a constant and the buffers are just mapped or not mapped. Correct > > > > >> > >> Please set it first and use it for allocation and as parameter to rxtx_map so > >> that a modification of the value would only have to be done in one place. > >> > >> Please use a define if this is a constant. > > > > How about adding a define FFA_MIN_RXTX_PAGE_COUNT and giving that to > > ffa_rxtx_map() and later assigning it to ffa_page_count if the call > > succeeds? > > Why MIN ? I was trying to prepare a bit for the future with a minimum value that would be rounded up to a larger granule as needed depending on PAGE_SIZE in Xen and what might be discovered about the secure world. > > How about just using ffa_rx or ffa_tx being NULL or not to check if the buffers are > mapped and remove the count. Sure, I'll drop the _MIN_ part of the define then. > > > > >> > >> As it is a global variable, does the parameter to rxtx_map make sense ? > > > > Yes, ffa_rxtx_map() is a dumb wrapper so it should have all the needed > > parameters for the SMC provided. > > Then passing FFA_MIN_RXTX_PAGE_COUNT should be enough. OK. Thanks, Jens > > Cheers > Bertrand > > > > > Cheers, > > Jens > > > >> > >> Cheers > >> Bertrand > >> > >>> ffa_version = vers; > >>> > >>> return true; > >>> + > >>> +err_free_ffa_tx: > >>> + free_xenheap_pages(ffa_tx, 0); > >>> + ffa_tx = NULL; > >>> +err_free_ffa_rx: > >>> + free_xenheap_pages(ffa_rx, 0); > >>> + ffa_rx = NULL; > >>> + ffa_page_count = 0; > >>> + ffa_version = 0; > >>> + > >>> + return false; > >>> } > >>> > >>> static const struct tee_mediator_ops ffa_ops = > >>> -- > >>> 2.34.1 > >
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c index a5d8a12635b6..07dd5c36d54b 100644 --- a/xen/arch/arm/tee/ffa.c +++ b/xen/arch/arm/tee/ffa.c @@ -148,6 +148,15 @@ struct ffa_ctx { /* Negotiated FF-A version to use with the SPMC */ static uint32_t ffa_version __ro_after_init; +/* + * Our rx/tx buffers shared with the SPMC. + * + * ffa_page_count is the number of pages used in each of these buffers. + */ +static void *ffa_rx __read_mostly; +static void *ffa_tx __read_mostly; +static unsigned int ffa_page_count __read_mostly; + static bool ffa_get_version(uint32_t *vers) { const struct arm_smccc_1_2_regs arg = { @@ -217,6 +226,17 @@ static bool check_mandatory_feature(uint32_t id) return !ret; } +static int32_t ffa_rxtx_map(register_t tx_addr, register_t rx_addr, + uint32_t page_count) +{ + uint32_t fid = FFA_RXTX_MAP_32; + + if ( IS_ENABLED(CONFIG_ARM_64) ) + fid = FFA_RXTX_MAP_64; + + return ffa_simple_call(fid, tx_addr, rx_addr, page_count, 0); +} + static uint16_t get_vm_id(const struct domain *d) { /* +1 since 0 is reserved for the hypervisor in FF-A */ @@ -384,6 +404,7 @@ static int ffa_relinquish_resources(struct domain *d) static bool ffa_probe(void) { uint32_t vers; + int e; unsigned int major_vers; unsigned int minor_vers; @@ -426,12 +447,46 @@ static bool ffa_probe(void) printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", major_vers, minor_vers); - if ( !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) + if ( +#ifdef CONFIG_ARM_64 + !check_mandatory_feature(FFA_RXTX_MAP_64) || +#endif +#ifdef CONFIG_ARM_32 + !check_mandatory_feature(FFA_RXTX_MAP_32) || +#endif + !check_mandatory_feature(FFA_RXTX_UNMAP) || + !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) ) return false; + ffa_rx = alloc_xenheap_pages(0, 0); + if ( !ffa_rx ) + return false; + + ffa_tx = alloc_xenheap_pages(0, 0); + if ( !ffa_tx ) + goto err_free_ffa_rx; + + e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), 1); + if ( e ) + { + printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e); + goto err_free_ffa_tx; + } + ffa_page_count = 1; ffa_version = vers; return true; + +err_free_ffa_tx: + free_xenheap_pages(ffa_tx, 0); + ffa_tx = NULL; +err_free_ffa_rx: + free_xenheap_pages(ffa_rx, 0); + ffa_rx = NULL; + ffa_page_count = 0; + ffa_version = 0; + + return false; } static const struct tee_mediator_ops ffa_ops =
When initializing the FF-A mediator map the RX and TX buffers shared with the SPMC. These buffer are later used to to transmit data that cannot be passed in registers only. Adds a check that the SP supports the needed FF-A features FFA_RXTX_MAP_64 / FFA_RXTX_MAP_32 and FFA_RXTX_UNMAP. In 64-bit mode we must use FFA_RXTX_MAP_64 since registers are used to transmit the physical addresses of the RX/TX buffers. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> --- xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-)