| Message ID | 20240424033449.168398-6-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remaining patches for dynamic node programming using overlay dtbo | expand |
On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote: > Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32. > This was done to allocate and assign IRQs to a running domain. > > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > tools/libs/light/libxl_arm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index dd5c9f4917..50dbd0f2a9 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > > LOG(DEBUG, "Configure the domain"); > > - config->arch.nr_spis = nr_spis; > + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */ > + config->arch.nr_spis = MAX(nr_spis, 160); Is there a way that that Xen or libxl could find out what the minimum number of SPI needs to be? Are we going to have to increase that minimum number every time a new platform comes along? It doesn't appear that libxl is using that `nr_spis` value and it is probably just given to Xen. So my guess is that Xen could simply take care of the minimum value, gic_number_lines() seems to be a Xen function. Thanks,
Hi Anthony, (+Arm maintainers) On 5/1/2024 9:58 PM, Anthony PERARD wrote: > On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote: >> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32. >> This was done to allocate and assign IRQs to a running domain. >> >> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> --- >> tools/libs/light/libxl_arm.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index dd5c9f4917..50dbd0f2a9 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> >> LOG(DEBUG, "Configure the domain"); >> >> - config->arch.nr_spis = nr_spis; >> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */ >> + config->arch.nr_spis = MAX(nr_spis, 160); > Is there a way that that Xen or libxl could find out what the minimum > number of SPI needs to be? I am afraid currently there is none. > Are we going to have to increase that minimum > number every time a new platform comes along? > > It doesn't appear that libxl is using that `nr_spis` value and it is > probably just given to Xen. So my guess is that Xen could simply take > care of the minimum value, gic_number_lines() seems to be a Xen > function. Xen will take care of the value of nr_spis for dom0 in create_dom0() dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; and also for dom0less domUs in create_domUs(). However, it looks like Xen will not take care of the mininum value for libxl guests, the value from config->arch.nr_spis in guest config file will be directly passed to the domain_vgic_init() function from arch_domain_create(). I agree with you that we shouldn't just bump the number everytime when we have a new platform. Therefore, would it be a good idea to move the logic in this patch to arch_sanitise_domain_config()? Kind regards, Henry > > Thanks, >
Hi, On 06/05/2024 06:17, Henry Wang wrote: > On 5/1/2024 9:58 PM, Anthony PERARD wrote: >> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote: >>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx >>> ZynqMP - 32. >>> This was done to allocate and assign IRQs to a running domain. >>> >>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> Signed-off-by: Henry Wang <xin.wang2@amd.com> >>> --- >>> tools/libs/light/libxl_arm.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >>> index dd5c9f4917..50dbd0f2a9 100644 >>> --- a/tools/libs/light/libxl_arm.c >>> +++ b/tools/libs/light/libxl_arm.c >>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >>> LOG(DEBUG, "Configure the domain"); >>> - config->arch.nr_spis = nr_spis; >>> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = >>> 192 - 32. */ >>> + config->arch.nr_spis = MAX(nr_spis, 160); >> Is there a way that that Xen or libxl could find out what the minimum >> number of SPI needs to be? > > I am afraid currently there is none. > >> Are we going to have to increase that minimum >> number every time a new platform comes along? >> >> It doesn't appear that libxl is using that `nr_spis` value and it is >> probably just given to Xen. So my guess is that Xen could simply take >> care of the minimum value, gic_number_lines() seems to be a Xen >> function. > > Xen will take care of the value of nr_spis for dom0 in create_dom0() > dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32; > and also for dom0less domUs in create_domUs(). > > However, it looks like Xen will not take care of the mininum value for > libxl guests, the value from config->arch.nr_spis in guest config file > will be directly passed to the domain_vgic_init() function from > arch_domain_create(). > > I agree with you that we shouldn't just bump the number everytime when > we have a new platform. Therefore, would it be a good idea to move the > logic in this patch to arch_sanitise_domain_config()? Xen domains are supposed to be platform agnostics and therefore the numbers of SPIs should not be based on the HW. Furthermore, with your proposal we would end up to allocate data structure for N SPIs when a domain may never needs any SPIs (such as if passthrough is not in-use). This is more likely for domain created by the toolstack than from Xen directly. Instead, we should introduce a new XL configuration to let the user decide the number of SPIs. I would suggest to name "nr_spis" to match the DT bindings. Cheers,
Hi Julien, On 5/7/2024 10:35 PM, Julien Grall wrote: > Hi, > > On 06/05/2024 06:17, Henry Wang wrote: >> On 5/1/2024 9:58 PM, Anthony PERARD wrote: >>> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote: >>>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx >>>> ZynqMP - 32. >>>> This was done to allocate and assign IRQs to a running domain. >>>> >>>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com> >>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>>> Signed-off-by: Henry Wang <xin.wang2@amd.com> >>>> --- >>>> tools/libs/light/libxl_arm.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/libs/light/libxl_arm.c >>>> b/tools/libs/light/libxl_arm.c >>>> index dd5c9f4917..50dbd0f2a9 100644 >>>> --- a/tools/libs/light/libxl_arm.c >>>> +++ b/tools/libs/light/libxl_arm.c >>>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc >>>> *gc, >>>> LOG(DEBUG, "Configure the domain"); >>>> - config->arch.nr_spis = nr_spis; >>>> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = >>>> 192 - 32. */ >>>> + config->arch.nr_spis = MAX(nr_spis, 160); >>> Is there a way that that Xen or libxl could find out what the minimum >>> number of SPI needs to be? >> >> I am afraid currently there is none. >> >>> Are we going to have to increase that minimum >>> number every time a new platform comes along? >>> >>> It doesn't appear that libxl is using that `nr_spis` value and it is >>> probably just given to Xen. So my guess is that Xen could simply take >>> care of the minimum value, gic_number_lines() seems to be a Xen >>> function. >> >> Xen will take care of the value of nr_spis for dom0 in create_dom0() >> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - >> 32; >> and also for dom0less domUs in create_domUs(). >> >> However, it looks like Xen will not take care of the mininum value >> for libxl guests, the value from config->arch.nr_spis in guest config >> file will be directly passed to the domain_vgic_init() function from >> arch_domain_create(). >> >> I agree with you that we shouldn't just bump the number everytime >> when we have a new platform. Therefore, would it be a good idea to >> move the logic in this patch to arch_sanitise_domain_config()? > > Xen domains are supposed to be platform agnostics and therefore the > numbers of SPIs should not be based on the HW. > > Furthermore, with your proposal we would end up to allocate data > structure for N SPIs when a domain may never needs any SPIs (such as > if passthrough is not in-use). This is more likely for domain created > by the toolstack than from Xen directly. Agreed on both comments. > Instead, we should introduce a new XL configuration to let the user > decide the number of SPIs. I would suggest to name "nr_spis" to match > the DT bindings. Sure, I will introduce a new xl config for this to replace this patch. Thank you for the suggestion. Kind regards, Henry > > Cheers, >
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index dd5c9f4917..50dbd0f2a9 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, LOG(DEBUG, "Configure the domain"); - config->arch.nr_spis = nr_spis; + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */ + config->arch.nr_spis = MAX(nr_spis, 160); LOG(DEBUG, " - Allocate %u SPIs", nr_spis); switch (d_config->b_info.arch_arm.gic_version) {