On 30/07/2018 15:47, Michael Ellerman wrote:
> Hi Laurent,
>
> Just one comment below.
>
> Laurent Dufour <lduf...@linux.vnet.ibm.com> writes:
>> diff --git a/arch/powerpc/platforms/pseries/lpar.c
>> b/arch/powerpc/platforms/pseries/lpar.c
>> index 96b8cd8a802d..41ed03245eb4 100644
>> --- a/arch/powerpc/platforms/pseries/lpar.c
>> +++ b/arch/powerpc/platforms/pseries/lpar.c
>> @@ -418,6 +418,73 @@ static void pSeries_lpar_hpte_invalidate(unsigned long
>> slot, unsigned long vpn,
>> BUG_ON(lpar_rc != H_SUCCESS);
>> }
>>
>> +
>> +/*
>> + * As defined in the PAPR's section 14.5.4.1.8
>> + * The control mask doesn't include the returned reference and change bit
>> from
>> + * the processed PTE.
>> + */
>> +#define HBLKR_AVPN 0x0100000000000000UL
>> +#define HBLKR_CTRL_MASK 0xf800000000000000UL
>> +#define HBLKR_CTRL_SUCCESS 0x8000000000000000UL
>> +#define HBLKR_CTRL_ERRNOTFOUND 0x8800000000000000UL
>> +#define HBLKR_CTRL_ERRBUSY 0xa000000000000000UL
>> +
>> +/**
>> + * H_BLOCK_REMOVE caller.
>> + * @idx should point to the latest @param entry set with a PTEX.
>> + * If PTE cannot be processed because another CPUs has already locked that
>> + * group, those entries are put back in @param starting at index 1.
>> + * If entries has to be retried and @retry_busy is set to true, these
>> entries
>> + * are retried until success. If @retry_busy is set to false, the returned
>> + * is the number of entries yet to process.
>> + */
>> +static unsigned long call_block_remove(unsigned long idx, unsigned long
>> *param,
>> + bool retry_busy)
>> +{
>> + unsigned long i, rc, new_idx;
>> + unsigned long retbuf[PLPAR_HCALL9_BUFSIZE];
>> +
>> +again:
>> + new_idx = 0;
>> + BUG_ON((idx < 2) || (idx > PLPAR_HCALL9_BUFSIZE));
>
> I count 1 ..
>
>> + if (idx < PLPAR_HCALL9_BUFSIZE)
>> + param[idx] = HBR_END;
>> +
>> + rc = plpar_hcall9(H_BLOCK_REMOVE, retbuf,
>> + param[0], /* AVA */
>> + param[1], param[2], param[3], param[4], /* TS0-7 */
>> + param[5], param[6], param[7], param[8]);
>> + if (rc == H_SUCCESS)
>> + return 0;
>> +
>> + BUG_ON(rc != H_PARTIAL);
>
> 2 ...
>
>> + /* Check that the unprocessed entries were 'not found' or 'busy' */
>> + for (i = 0; i < idx-1; i++) {
>> + unsigned long ctrl = retbuf[i] & HBLKR_CTRL_MASK;
>> +
>> + if (ctrl == HBLKR_CTRL_ERRBUSY) {
>> + param[++new_idx] = param[i+1];
>> + continue;
>> + }
>> +
>> + BUG_ON(ctrl != HBLKR_CTRL_SUCCESS
>> + && ctrl != HBLKR_CTRL_ERRNOTFOUND);
>
> 3 ...
>
> BUG_ON()s.
>
> I know the code in this file is already pretty liberal with the use of
> BUG_ON() but I'd prefer if we don't make it any worse.
The first one is clearly not required. But I would keep the following twos
because this call is not expected to fail except if there is a discrepancy
between the linux kernel HASH views and the hypervisor's one, which could be
dramatic in the consequences.
>
> Given this is an optimisation it seems like we should be able to fall
> back to the existing implementation in the case of error (which will
> probably then BUG_ON() 😂)
I don't think falling back to the H_BULK call will be helpfull since it is
doing the same so the same errors are expected. Furthermore, this hcall can do
a partial work which means complex code to fallback on H_BULK as we should
identify to already processed entries.
> If there's some reason we can't then I guess I can live with it.
I'm proposing to send a new series with _only_ 2 calls to BUG_ON().
Furthermore this patch is not correct on the way the huge pages are managed. I
was too hurry to push it last time.
Cheers,
Laurent.
