| Message ID | 20230409181918.29270-1-quic_nkela@quicinc.com |
|---|---|
| Headers | show |
| Series | Allow parameter in smc/hvc calls | expand |
On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote: > Currently, smc/hvc calls are made with parameters set > to zeros. We are using multiple scmi instances within > a VM and hypervisor associates a tag with each instance > and expects the tag in hvc calls from that instance, while > sharing the same smc-id(func_id) among the instances. > > This patch series introduces new optional dtb bindings which > can be used to pass parameters to smc/hvc calls. > Just to be sure that I understood the problem(as your 2 parameters confused me), this is just to help hypervisor/secure side to identify the right channel/shared memory when the same SMC/HVC function id is shared right ? If that is the case, why can't we just pass the shmem address as the parameter ? I would like to avoid fragmentation here with every vendor trying to do different things to achieve the same. I would just change the driver to do that. Not sure if it breaks any existing implementation or not. If it does, we can add another compatible to identify one needing this fixed(shmem address) as additional parameter. Does that make sense at all ? Or am I missing some/all of the requirements here ?
On 4/11/2023 6:01 AM, Sudeep Holla wrote: > On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote: >> Currently, smc/hvc calls are made with parameters set >> to zeros. We are using multiple scmi instances within >> a VM and hypervisor associates a tag with each instance >> and expects the tag in hvc calls from that instance, while >> sharing the same smc-id(func_id) among the instances. >> >> This patch series introduces new optional dtb bindings which >> can be used to pass parameters to smc/hvc calls. >> > Just to be sure that I understood the problem(as your 2 parameters confused > me), this is just to help hypervisor/secure side to identify the right > channel/shared memory when the same SMC/HVC function id is shared right ? that's somewhat correct. Our hypervisor allocates 64bit ids on every boot for each scmi instance which it uses to identify the scmi instance. Additionally, our hypervisor also categorizes events within each instance by an event-id that gets passed to hvc call as second parameter. So we can pass two parameters in addition to function_id. > > If that is the case, why can't we just pass the shmem address as the > parameter ? I would like to avoid fragmentation here with every vendor > trying to do different things to achieve the same. that's a good suggestion. Any solution you propose shouldn't just limit to only one parameter. IMO, there should be some way to pass all 6 parameters since we do have a use case of at least two parameters. > > I would just change the driver to do that. Not sure if it breaks any existing > implementation or not. If it does, we can add another compatible to identify > one needing this fixed(shmem address) as additional parameter. > > Does that make sense at all ? Or am I missing some/all of the requirements > here ? The shmem proposal is fine however please also incorporate passing of other parameters. >
On 4/12/2023 1:37 AM, Sudeep Holla wrote: > On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote: > >> that's a good suggestion. Any solution you propose shouldn't just limit to >> only one parameter. IMO, there should be some way to pass all 6 parameters >> since we do have a use case of at least two parameters. > Please elaborate on your use-case. Based on your comments below, we will change our hypervisor to make use of shmem. > >> The shmem proposal is fine however please also incorporate passing of other >> parameters. > You are missing the point here. SMC/HVC is just a doorbell and the main point > I made earlier is that there is no need for vendors to try colourful things > here if it is not necessary. So no, I don't want any extra bindings or more > than one param is that is not needed. I will wait for the reason as requested > above. ok, understood. In that case, we will change our hypervisor to use shmem address as instance identifier. Please add support for one param, thanks!
On 4/17/23 10:44, Nikunj Kela wrote: > This patch add support for passing shmem channel address as parameter > in smc/hvc call. This patch is useful when multiple scmi instances are > using same smc-id and firmware needs to distiguish among the instances. Typo: distinguish. It really would have been a lot clearer and made a whole lot more sense to encode a VM ID/channel number within some of the SMCCC parameters, possibly as part of the function ID itself. > > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > drivers/firmware/arm_scmi/driver.c | 1 + > drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index e7d97b59963b..b5957cc12fee 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = { > #endif > #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC > { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc}, > + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc}, > #endif > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO > { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc}, > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c > index 93272e4bbd12..e28387346d33 100644 > --- a/drivers/firmware/arm_scmi/smc.c > +++ b/drivers/firmware/arm_scmi/smc.c > @@ -20,6 +20,9 @@ > > #include "common.h" > > +#define lower32(x) ((u32)((x) & 0xffffffff)) > +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff)) Cannot you use the existing lower_32_bits and upper_32_bits macros from kernel.h here? > + > /** > * struct scmi_smc - Structure representing a SCMI smc transport > * > @@ -30,6 +33,8 @@ > * @inflight: Atomic flag to protect access to Tx/Rx shared memory area. > * Used when operating in atomic mode. > * @func_id: smc/hvc call function id > + * @is_smc64: smc/hvc calling convention type 64 vs 32 > + * @param: physical address of the shmem channel > */ > > struct scmi_smc { > @@ -40,6 +45,8 @@ struct scmi_smc { > #define INFLIGHT_NONE MSG_TOKEN_MAX > atomic_t inflight; > u32 func_id; > + bool is_smc64; > + phys_addr_t param; > }; > > static irqreturn_t smc_msg_done_isr(int irq, void *data) > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > if (ret < 0) > return ret; > > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) > + scmi_info->param = res.start; There is not even a check that this is going to be part of the kernel's view of memory, that seems a bit brittle and possibly a security hole, too. Your hypervisor presumably needs to have carved out some amount of memory in order for the messages to be written to/read from, and so would the VM kernel, so eventually we should have a 'reserved-memory' entry of some sort, no? > /* > * If there is an interrupt named "a2p", then the service and > * completion of a message is signaled by an interrupt rather than by > @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > } > > scmi_info->func_id = func_id; > + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); > scmi_info->cinfo = cinfo; > smc_channel_lock_init(scmi_info); > cinfo->transport_info = scmi_info; > @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo, > > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); > > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); > +#ifdef CONFIG_ARM64 > + /* > + * if SMC32 convention is used, pass 64 bit address in > + * two parameters > + */ > + if (!scmi_info->is_smc64) There is no need for scmi_info to store is_smc64, just check the func_id here and declare is_smc64 as a local variable to the function. Also, another way to approach this would be to encode the parameters region in 4KB units such that event on a 32-bit system with LPAE you are guaranteed to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE are indistinguishable on real CPUs? > + arm_smccc_1_1_invoke(scmi_info->func_id, > + lower32(scmi_info->param), > + upper32(scmi_info->param), > + 0, 0, 0, 0, 0, &res); > + else > +#endif > + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param, > + 0, 0, 0, 0, 0, 0, &res); > > /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */ > if (res.a0) {
On 4/17/2023 11:01 AM, Florian Fainelli wrote: > On 4/17/23 10:44, Nikunj Kela wrote: >> This patch add support for passing shmem channel address as parameter >> in smc/hvc call. This patch is useful when multiple scmi instances are >> using same smc-id and firmware needs to distiguish among the instances. > > Typo: distinguish. > Will fix it. > It really would have been a lot clearer and made a whole lot more > sense to encode a VM ID/channel number within some of the SMCCC > parameters, possibly as part of the function ID itself. smc-id(func-id) is 32 bit long and the spec doesn't define any such provisions in it. Having said that, there are optional parameters as session-id and client-id(secureOS-id) that can be passed in w6/r6 and w7/r7 registers. If maintainers are OK to use dtb to pass them instead, I can rework the patches. > >> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >> --- >> drivers/firmware/arm_scmi/driver.c | 1 + >> drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++- >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/arm_scmi/driver.c >> b/drivers/firmware/arm_scmi/driver.c >> index e7d97b59963b..b5957cc12fee 100644 >> --- a/drivers/firmware/arm_scmi/driver.c >> +++ b/drivers/firmware/arm_scmi/driver.c >> @@ -2914,6 +2914,7 @@ static const struct of_device_id >> scmi_of_match[] = { >> #endif >> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC >> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc}, >> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc}, >> #endif >> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO >> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc}, >> diff --git a/drivers/firmware/arm_scmi/smc.c >> b/drivers/firmware/arm_scmi/smc.c >> index 93272e4bbd12..e28387346d33 100644 >> --- a/drivers/firmware/arm_scmi/smc.c >> +++ b/drivers/firmware/arm_scmi/smc.c >> @@ -20,6 +20,9 @@ >> #include "common.h" >> +#define lower32(x) ((u32)((x) & 0xffffffff)) >> +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff)) > > Cannot you use the existing lower_32_bits and upper_32_bits macros > from kernel.h here? > >> + >> /** >> * struct scmi_smc - Structure representing a SCMI smc transport >> * >> @@ -30,6 +33,8 @@ >> * @inflight: Atomic flag to protect access to Tx/Rx shared memory >> area. >> * Used when operating in atomic mode. >> * @func_id: smc/hvc call function id >> + * @is_smc64: smc/hvc calling convention type 64 vs 32 >> + * @param: physical address of the shmem channel >> */ >> struct scmi_smc { >> @@ -40,6 +45,8 @@ struct scmi_smc { >> #define INFLIGHT_NONE MSG_TOKEN_MAX >> atomic_t inflight; >> u32 func_id; >> + bool is_smc64; >> + phys_addr_t param; >> }; >> static irqreturn_t smc_msg_done_isr(int irq, void *data) >> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info >> *cinfo, struct device *dev, >> if (ret < 0) >> return ret; >> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) >> + scmi_info->param = res.start; > > There is not even a check that this is going to be part of the > kernel's view of memory, that seems a bit brittle and possibly a > security hole, too. Your hypervisor presumably needs to have carved > out some amount of memory in order for the messages to be written > to/read from, and so would the VM kernel, so eventually we should have > a 'reserved-memory' entry of some sort, no? > >> /* >> * If there is an interrupt named "a2p", then the service and >> * completion of a message is signaled by an interrupt rather >> than by >> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info >> *cinfo, struct device *dev, >> } >> scmi_info->func_id = func_id; >> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); >> scmi_info->cinfo = cinfo; >> smc_channel_lock_init(scmi_info); >> cinfo->transport_info = scmi_info; >> @@ -188,7 +198,20 @@ static int smc_send_message(struct >> scmi_chan_info *cinfo, >> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); >> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, >> &res); >> +#ifdef CONFIG_ARM64 >> + /* >> + * if SMC32 convention is used, pass 64 bit address in >> + * two parameters >> + */ >> + if (!scmi_info->is_smc64) > > There is no need for scmi_info to store is_smc64, just check the > func_id here and declare is_smc64 as a local variable to the function. > > Also, another way to approach this would be to encode the parameters > region in 4KB units such that event on a 32-bit system with LPAE you > are guaranteed to fit the region into a 32-bit unsigned long. AFAIR > virtualization and LPAE are indistinguishable on real CPUs? > >> + arm_smccc_1_1_invoke(scmi_info->func_id, >> + lower32(scmi_info->param), >> + upper32(scmi_info->param), >> + 0, 0, 0, 0, 0, &res); >> + else >> +#endif >> + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param, >> + 0, 0, 0, 0, 0, 0, &res); >> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */ >> if (res.a0) { >
On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote: > On 4/17/23 10:44, Nikunj Kela wrote: > > This patch add support for passing shmem channel address as parameter > > in smc/hvc call. This patch is useful when multiple scmi instances are > > using same smc-id and firmware needs to distiguish among the instances. > > Typo: distinguish. > > It really would have been a lot clearer and made a whole lot more sense to > encode a VM ID/channel number within some of the SMCCC parameters, possibly > as part of the function ID itself. > Yes I was about to suggest this but then remembered one main reason I have been given in the past against that: If the system launches high number of VMs then that means loads of FID needs to be reserved for SCMI and the hypervisor needs to support it. Basically it is not scalable which I agree but not sure if such large number of VMs are used in reality with SCMI. But I agree with the technical reasoning. The other reason why I preferred the shmem address itself instead of some custom VM ID/channel number is that it can easily becomes vendor specific for no real good reason and then we need to add support for each such different parameters. Nikunj suggested getting them from DT which I really don't like if the sole reason is just to identify the channel. We don't have standard SCMI SMC/HVC but allowing such deviations with params from DT will just explode with various combinations for silly/no reason. [...] > > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > > if (ret < 0) > > return ret; > > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) > > + scmi_info->param = res.start; > > There is not even a check that this is going to be part of the kernel's view > of memory, that seems a bit brittle and possibly a security hole, too. Your > hypervisor presumably needs to have carved out some amount of memory in > order for the messages to be written to/read from, and so would the VM > kernel, so eventually we should have a 'reserved-memory' entry of some sort, > no? > Not disagreeing here. Just checking if my understanding is correct or not. IIUC, we need reserved-memory if it is part of the memory presented to the OS right ? You don't need that if it is dedicated memory like part of SRAM or something similar. > > /* > > * If there is an interrupt named "a2p", then the service and > > * completion of a message is signaled by an interrupt rather than by > > @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > > } > > scmi_info->func_id = func_id; > > + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); > > scmi_info->cinfo = cinfo; > > smc_channel_lock_init(scmi_info); > > cinfo->transport_info = scmi_info; > > @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo, > > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); > > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); > > +#ifdef CONFIG_ARM64 > > + /* > > + * if SMC32 convention is used, pass 64 bit address in > > + * two parameters > > + */ > > + if (!scmi_info->is_smc64) > > There is no need for scmi_info to store is_smc64, just check the func_id > here and declare is_smc64 as a local variable to the function. > +1 > Also, another way to approach this would be to encode the parameters region > in 4KB units such that event on a 32-bit system with LPAE you are guaranteed > to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE > are indistinguishable on real CPUs? > Agree with the idea. But can a single 4kB be used for multiple shmem across VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible practically. -- Regards, Sudeep
On 4/18/2023 2:58 AM, Sudeep Holla wrote: > On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote: >> On 4/17/23 10:44, Nikunj Kela wrote: >>> This patch add support for passing shmem channel address as parameter >>> in smc/hvc call. This patch is useful when multiple scmi instances are >>> using same smc-id and firmware needs to distiguish among the instances. >> Typo: distinguish. >> >> It really would have been a lot clearer and made a whole lot more sense to >> encode a VM ID/channel number within some of the SMCCC parameters, possibly >> as part of the function ID itself. >> > Yes I was about to suggest this but then remembered one main reason I have > been given in the past against that: > If the system launches high number of VMs then that means loads of FID > needs to be reserved for SCMI and the hypervisor needs to support it. > Basically it is not scalable which I agree but not sure if such large > number of VMs are used in reality with SCMI. But I agree with the technical > reasoning. > > The other reason why I preferred the shmem address itself instead of some > custom VM ID/channel number is that it can easily becomes vendor specific > for no real good reason and then we need to add support for each such > different parameters. Nikunj suggested getting them from DT which I really > don't like if the sole reason is just to identify the channel. We don't > have standard SCMI SMC/HVC but allowing such deviations with params from > DT will just explode with various combinations for silly/no reason. > Would you be ok to pass the smc/hvc parameters via kernel parameters in commandline? If so, I can implement that. I just wanted to keep everything in one place hence suggested using DTB node. > [...] > >>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, >>> if (ret < 0) >>> return ret; >>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) >>> + scmi_info->param = res.start; >> There is not even a check that this is going to be part of the kernel's view >> of memory, that seems a bit brittle and possibly a security hole, too. Your >> hypervisor presumably needs to have carved out some amount of memory in >> order for the messages to be written to/read from, and so would the VM >> kernel, so eventually we should have a 'reserved-memory' entry of some sort, >> no? >> > Not disagreeing here. Just checking if my understanding is correct or not. > IIUC, we need reserved-memory if it is part of the memory presented to the > OS right ? You don't need that if it is dedicated memory like part of SRAM > or something similar. We are not using reserved-memory node. Instead using sram-mmio to carve out shmem for scmi instances. >>> /* >>> * If there is an interrupt named "a2p", then the service and >>> * completion of a message is signaled by an interrupt rather than by >>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, >>> } >>> scmi_info->func_id = func_id; >>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); >>> scmi_info->cinfo = cinfo; >>> smc_channel_lock_init(scmi_info); >>> cinfo->transport_info = scmi_info; >>> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo, >>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); >>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); >>> +#ifdef CONFIG_ARM64 >>> + /* >>> + * if SMC32 convention is used, pass 64 bit address in >>> + * two parameters >>> + */ >>> + if (!scmi_info->is_smc64) >> There is no need for scmi_info to store is_smc64, just check the func_id >> here and declare is_smc64 as a local variable to the function. >> > +1 ACK! >> Also, another way to approach this would be to encode the parameters region >> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed >> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE >> are indistinguishable on real CPUs? >> > Agree with the idea. But can a single 4kB be used for multiple shmem across > VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible > practically. We have multiple VMs and each VM has multiple instances. We will have quite a few domains and I am not sure if single page will suffice. > -- > Regards, > Sudeep
On 4/18/23 07:20, Nikunj Kela wrote: > > On 4/18/2023 2:58 AM, Sudeep Holla wrote: >> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote: >>> On 4/17/23 10:44, Nikunj Kela wrote: >>>> This patch add support for passing shmem channel address as parameter >>>> in smc/hvc call. This patch is useful when multiple scmi instances are >>>> using same smc-id and firmware needs to distiguish among the instances. >>> Typo: distinguish. >>> >>> It really would have been a lot clearer and made a whole lot more >>> sense to >>> encode a VM ID/channel number within some of the SMCCC parameters, >>> possibly >>> as part of the function ID itself. >>> >> Yes I was about to suggest this but then remembered one main reason I >> have >> been given in the past against that: >> If the system launches high number of VMs then that means loads of FID >> needs to be reserved for SCMI and the hypervisor needs to support it. >> Basically it is not scalable which I agree but not sure if such large >> number of VMs are used in reality with SCMI. But I agree with the >> technical >> reasoning. >> >> The other reason why I preferred the shmem address itself instead of some >> custom VM ID/channel number is that it can easily becomes vendor specific >> for no real good reason and then we need to add support for each such >> different parameters. Nikunj suggested getting them from DT which I >> really >> don't like if the sole reason is just to identify the channel. We don't >> have standard SCMI SMC/HVC but allowing such deviations with params from >> DT will just explode with various combinations for silly/no reason. >> > Would you be ok to pass the smc/hvc parameters via kernel parameters in > commandline? If so, I can implement that. I just wanted to keep > everything in one place hence suggested using DTB node. Command line arguments seem a bit unnecessary here and it would force you to invent a scheme to control per SCMI device/instance parameters. > >> [...] >> >>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info >>>> *cinfo, struct device *dev, >>>> if (ret < 0) >>>> return ret; >>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) >>>> + scmi_info->param = res.start; >>> There is not even a check that this is going to be part of the >>> kernel's view >>> of memory, that seems a bit brittle and possibly a security hole, >>> too. Your >>> hypervisor presumably needs to have carved out some amount of memory in >>> order for the messages to be written to/read from, and so would the VM >>> kernel, so eventually we should have a 'reserved-memory' entry of >>> some sort, >>> no? >>> >> Not disagreeing here. Just checking if my understanding is correct or >> not. >> IIUC, we need reserved-memory if it is part of the memory presented to >> the >> OS right ? You don't need that if it is dedicated memory like part of >> SRAM >> or something similar. > We are not using reserved-memory node. Instead using sram-mmio to carve > out shmem for scmi instances. OK, that works too. >>>> /* >>>> * If there is an interrupt named "a2p", then the service and >>>> * completion of a message is signaled by an interrupt rather >>>> than by >>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info >>>> *cinfo, struct device *dev, >>>> } >>>> scmi_info->func_id = func_id; >>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); >>>> scmi_info->cinfo = cinfo; >>>> smc_channel_lock_init(scmi_info); >>>> cinfo->transport_info = scmi_info; >>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct >>>> scmi_chan_info *cinfo, >>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); >>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, >>>> &res); >>>> +#ifdef CONFIG_ARM64 >>>> + /* >>>> + * if SMC32 convention is used, pass 64 bit address in >>>> + * two parameters >>>> + */ >>>> + if (!scmi_info->is_smc64) >>> There is no need for scmi_info to store is_smc64, just check the func_id >>> here and declare is_smc64 as a local variable to the function. >>> >> +1 > ACK! >>> Also, another way to approach this would be to encode the parameters >>> region >>> in 4KB units such that event on a 32-bit system with LPAE you are >>> guaranteed >>> to fit the region into a 32-bit unsigned long. AFAIR virtualization >>> and LPAE >>> are indistinguishable on real CPUs? >>> >> Agree with the idea. But can a single 4kB be used for multiple shmem >> across >> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is >> possible >> practically. > We have multiple VMs and each VM has multiple instances. We will have > quite a few domains and I am not sure if single page will suffice. I did not make myself clear enough, you can encode an offset into the shared memory area, and however big that shared memory area will be, that offset can be in a 4KB size. So for instance if you have your MMIO SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the second VM can use 0x8000_1000 to 0x8000_1fff and so on and so forth. Even if you need more than 4KB per VM, you can put that information into the two additional parameters you pass through the SMC/HVC call.
On 4/18/2023 9:33 AM, Florian Fainelli wrote: > On 4/18/23 07:20, Nikunj Kela wrote: >> >> On 4/18/2023 2:58 AM, Sudeep Holla wrote: >>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote: >>>> On 4/17/23 10:44, Nikunj Kela wrote: >>>>> This patch add support for passing shmem channel address as parameter >>>>> in smc/hvc call. This patch is useful when multiple scmi instances >>>>> are >>>>> using same smc-id and firmware needs to distiguish among the >>>>> instances. >>>> Typo: distinguish. >>>> >>>> It really would have been a lot clearer and made a whole lot more >>>> sense to >>>> encode a VM ID/channel number within some of the SMCCC parameters, >>>> possibly >>>> as part of the function ID itself. >>>> >>> Yes I was about to suggest this but then remembered one main reason >>> I have >>> been given in the past against that: >>> If the system launches high number of VMs then that means loads of FID >>> needs to be reserved for SCMI and the hypervisor needs to support it. >>> Basically it is not scalable which I agree but not sure if such large >>> number of VMs are used in reality with SCMI. But I agree with the >>> technical >>> reasoning. >>> >>> The other reason why I preferred the shmem address itself instead of >>> some >>> custom VM ID/channel number is that it can easily becomes vendor >>> specific >>> for no real good reason and then we need to add support for each such >>> different parameters. Nikunj suggested getting them from DT which I >>> really >>> don't like if the sole reason is just to identify the channel. We don't >>> have standard SCMI SMC/HVC but allowing such deviations with params >>> from >>> DT will just explode with various combinations for silly/no reason. >>> >> Would you be ok to pass the smc/hvc parameters via kernel parameters >> in commandline? If so, I can implement that. I just wanted to keep >> everything in one place hence suggested using DTB node. > > Command line arguments seem a bit unnecessary here and it would force > you to invent a scheme to control per SCMI device/instance parameters. > Agreed! >> >>> [...] >>> >>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct >>>>> scmi_chan_info *cinfo, struct device *dev, >>>>> if (ret < 0) >>>>> return ret; >>>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) >>>>> + scmi_info->param = res.start; >>>> There is not even a check that this is going to be part of the >>>> kernel's view >>>> of memory, that seems a bit brittle and possibly a security hole, >>>> too. Your >>>> hypervisor presumably needs to have carved out some amount of >>>> memory in >>>> order for the messages to be written to/read from, and so would the VM >>>> kernel, so eventually we should have a 'reserved-memory' entry of >>>> some sort, >>>> no? >>>> >>> Not disagreeing here. Just checking if my understanding is correct >>> or not. >>> IIUC, we need reserved-memory if it is part of the memory presented >>> to the >>> OS right ? You don't need that if it is dedicated memory like part >>> of SRAM >>> or something similar. >> We are not using reserved-memory node. Instead using sram-mmio to >> carve out shmem for scmi instances. > > OK, that works too. > >>>>> /* >>>>> * If there is an interrupt named "a2p", then the service and >>>>> * completion of a message is signaled by an interrupt >>>>> rather than by >>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct >>>>> scmi_chan_info *cinfo, struct device *dev, >>>>> } >>>>> scmi_info->func_id = func_id; >>>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id); >>>>> scmi_info->cinfo = cinfo; >>>>> smc_channel_lock_init(scmi_info); >>>>> cinfo->transport_info = scmi_info; >>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct >>>>> scmi_chan_info *cinfo, >>>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); >>>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, >>>>> &res); >>>>> +#ifdef CONFIG_ARM64 >>>>> + /* >>>>> + * if SMC32 convention is used, pass 64 bit address in >>>>> + * two parameters >>>>> + */ >>>>> + if (!scmi_info->is_smc64) >>>> There is no need for scmi_info to store is_smc64, just check the >>>> func_id >>>> here and declare is_smc64 as a local variable to the function. >>>> >>> +1 >> ACK! >>>> Also, another way to approach this would be to encode the >>>> parameters region >>>> in 4KB units such that event on a 32-bit system with LPAE you are >>>> guaranteed >>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization >>>> and LPAE >>>> are indistinguishable on real CPUs? >>>> >>> Agree with the idea. But can a single 4kB be used for multiple shmem >>> across >>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it >>> is possible >>> practically. >> We have multiple VMs and each VM has multiple instances. We will have >> quite a few domains and I am not sure if single page will suffice. > > I did not make myself clear enough, you can encode an offset into the > shared memory area, and however big that shared memory area will be, > that offset can be in a 4KB size. So for instance if you have your > MMIO SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the > second VM can use 0x8000_1000 to 0x8000_1fff and so on and so forth. > > Even if you need more than 4KB per VM, you can put that information > into the two additional parameters you pass through the SMC/HVC call. Okay. I will float another version, first parameter of smc/hvc call will be pfn and second the offset!