Re: [PATCH v2] powerpc: increase MIN RMA size for CAS negotiation

[Avnish Chouhan <avnish@linux.ibm.com>]

Thank you Daniel!

As suggested, I will rephrase the comment and re submit the patch!

On 2025-03-11 19:14, Daniel Kiper wrote:
> On Tue, Mar 11, 2025 at 02:59:35PM +0530, Avnish Chouhan wrote:
>> Hi Daniel,
>> Thank you for your response!
>> 
>> --------------
>> Condition before the patch:
>> 
>> if (err == GRUB_ERR_NONE && rmo_top < (512 * 1024 * 1024))
>>   grub_ieee1275_ibm_cas ();
>> 
>> Condition after the patch:
>> 
>> if (!ibm_ca_support_reboot && err == GRUB_ERR_NONE && rmo_top < (512 * 
>> 1024 * 1024))
>>   grub_ieee1275_ibm_cas ();
> 
> Avnish, I understand the code. The problem is the commit message and
> comment are partially unreadable/incomprehensible. So, that is why
> I am asking you to rephrase them. Now they are better but still...
> 
>> --------------
>> 
>> We have added just one extra check in the code 
>> "!ibm_ca_support_reboot" to
>> check whether the reboot is a CAS reboot or not!
>> 
>> And these are below comments in the patch which are in question:
>> 
>> +      /*
>> +       * If we have an error or the reboot is detected as CAS reboot,
>> +       * don't call CAS, just hope for the best.
>> +       * Along with the above, if the rmo_top is 512 MB or above. We
>> +       * will skip the CAS call. Though if we call CAS, the rmo_top 
>> will
>> +       * be set to 768 MB via CAS Vector2. This condition is required 
>> to avoid the
>> +       * issue where the older Linux kernels are still using rmo_top 
>> as 512 MB.
>> +       * If we call CAS where rmo_top is less then 768 MB, this will 
>> result in an issue
>> +       * due to IBM CAS reboot feature and we won't be able to boot 
>> the newer kernel.
>> +       * The machine will boot with the last booted kernel which has 
>> rmo_top as 512 MB.
>> +       */
>> 
>> I'm tried to explain in the comment on when the CAS will be called. 
>> And why
>> we need to use this old condition "rmo_top < 512 MB" and not "rmo_top 
>> < 768
>> MB".
>> 
>> +      if (!ibm_ca_support_reboot && err == GRUB_ERR_NONE && rmo_top < 
>> (512
>> * 1024 * 1024))
>> +        grub_ieee1275_ibm_cas ();
>>      }
>> 
>> 
>> Condition 1: (!ibm_ca_support_reboot)
>> 
>> This condition checks whether the last reboot is caused by CAS. If the
>> reboot is detected as a CAS reboot, the GRUB will skip the CAS call. 
>> As the
>> CAS has already been called earlier and it's not required to call even 
>> if
>> the other conditions are met!
>> 
>> Condition 2: (rmo_top < (512 * 1024 * 1024))
>> 
>> If the machine detects rmo_top as less than 512 MB, the CAS will be 
>> called.
>> 
>> Why we need this condition:
>> 
>> Logically as we are changing MIN_RMA as 768 MB in GRUB 
>> Options_vector2. We
>> should check "rmo_top < (768 * 1024 * 1024)" and not "rmo_top < (512 * 
>> 1024
>> * 1024)".
>> 
>> In the patch, whenever we are calling CAS. We set MIN_RMA as 768 MB. 
>> But we
>> decide when to call CAS is based on old condition rmo_top < 512 MB.
>> Logically it should be 768 MB. But we can't do this right now due to 
>> the
>> below scenarios. We will change this condition to "rmo_top < (768 * 
>> 1024 *
>> 1024)" in the future.
>> 
>> *****
>> Scenario 1:
>> In kernel prom_init.c file. The Options_vector2 is using 512 MB as 
>> MIN_RMA.
>> And GRUB is using "rmo_top < (768 * 1024 * 1024)" to call CAS.
>> 
>> 1. Machine boots, GRUB detects rmo_top as less than 512 MB.
>>    GRUB calls CAS and sets MIN_RMA as 768MB.
>>    The machine reboots after the CAS call. (Every CAS call will result 
>> in a reboot)
>> 2. Machine boots, GRUB detects rmo_top is not as less than 512 MB.
>>    GRUB skips CAS call.
>> 3. After this kernel boots and detects MIN_RMA as other than its 512 
>> MB required value.
>>    It calls CAS and makes the MIN_RMA again to 512 MB.
>>    As the CAS is called, the machine will go for a reboot again.
>> 
>> 4. Now GRUB will again detects rmo_top as less than 512 MB (changed by 
>> kernel).
>>    And then we will again go back to step 1.
>> 
>> And machine will keep doing the CAS calls and change MIN_RMA from 512 
>> to 768
>> to 512 to 768 and so on. With this, the machine will stuck in this 
>> stage
>> forever!
>> *****
>> 
>> In the above scenario 1, with (!ibm_ca_support_reboot) condition in 
>> place.
>> We will avoid this CAS reboot loop. But if we use "rmo_top < (768 * 
>> 1024 *
>> 1024)". The machine will never get stuck in reboot loop, but as the 
>> CAS is
>> called from GRUB (currently all the powerpc machines has rmo_top is 
>> 512 MB).
>> The IBM CAS reboot feature will not allow us to boot with the newer 
>> kernel!
>> 
>> IBM CAS reboot feature:
>> 
>> Whenever a reboot is detected as the CAS reboot by GRUB. It will boot 
>> the
>> machine with the last booted kernel by reading the variable
>> "boot-last-label" that has the info related to the last boot. This is
>> specific to IBM powerpc and no other architecture  has this.
>> 
>> *****
>> Scenario 2:
>> In kernel prom_init.c file. The Options_vector2 is using 768 MB as 
>> MIN_RMA.
>> And GRUB is using "rmo_top < (768 * 1024 * 1024)" to call CAS.
>> 
>> 1. Machine boots, GRUB detects rmo_top as less than 512 MB.
>>    GRUB calls CAS and sets MIN_RMA as 768MB.
>>    The machine reboots after the CAS call. (Every CAS call will result 
>> in a reboot)
>> 2. Machine boots, GRUB detects rmo_top is not as less than 512 MB.
>>    GRUB skips CAS call.
>> 3. But as the last boot was a CAS reboot, the machine will boot with 
>> the
>> last booted kernel having MIN_RMA as 512 MB. We will not see an option 
>> to
>> choose which kernel a user like to boot to.
>> *****
>> 
>> _________________
>> 
>> Please let me know if you feel I need to change or add any content in 
>> my
>> "comment" in the patch. I have tried my best to explain and cover 
>> these
>> above scenarios in simple and short message.
>> And let me know if you have any queries on this!
> 
> I think something like that, maybe a bit shortened by dropping some
> obvious things, should land in the comment...
> 
> Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel