[Skiboot] [PATCH skiboot v2] npu2: Add XTS_BDF_MAP wildcard refcount

[Alexey Kardashevskiy]

On 21/11/2018 07:18, Reza Arbab wrote:
> On Tue, Nov 13, 2018 at 07:16:02PM +1100, Alexey Kardashevskiy wrote:
>> @@ -2097,6 +2096,11 @@ static int64_t opal_npu_init_context(uint64_t
>> phb_id, int pasid __unused,
>>     }
>>
>>     id = GETFIELD(NPU2_XTS_BDF_MAP_LPARSHORT, xts_bdf);
>> +    if (id >= NPU2_XTS_BDF_MAP_SIZE) {
>> +        NPU2ERR(p, "Out of bounds id=%d\n", id);
>> +        id = OPAL_INTERNAL_ERROR;
>> +        goto out;
>> +    }
>>     NPU2DBG(p, "Found LPARSHORT = 0x%x for BDF = 0x%03llx\n", id, bdf);
>>
>>     /* Enable this mapping for both real and virtual addresses */
> 
> It doesn't hurt to check, I guess, but I'm not sure how/if it's possible
> for this to happen.


Heh. True. Guaranteed to succeed here. Although it would be immediately
clear if NPU2_XTS_BDF_MAP_SIZE was defined via
NPU2_XTS_BDF_MAP_LPARSHORT - 16 vs. 4 bits is pretty bulletproof.


> 
>> @@ -2129,14 +2133,20 @@ static int64_t opal_npu_init_context(uint64_t
>> phb_id, int pasid __unused,
>>             GETFIELD(NPU2_XTS_PID_MAP_MSR, xts_bdf_pid)) {
>>             NPU2ERR(p, "%s: Unexpected MSR value\n", __func__);
>>             id = OPAL_PARAMETER;
>> +            goto out;
>> +        } else if (!p->ctx_ref[id]) {
>> +            NPU2ERR(p, "%s: Unexpected mapping\n", __func__);
>> +            id = OPAL_INTERNAL_ERROR;
>> +            goto out;
> 
> Same.


Not quite the same though. A counter vs. a register value. I'd rather
keep this check.


> 
>>         }
>> -
>> -        goto out;
>>     }
>>
>>     /* Write the entry */
>> -    NPU2DBG(p, "XTS_PID_MAP[%03d] = 0x%08llx\n", id, xts_bdf_pid);
>> -    npu2_write(p, NPU2_XTS_PID_MAP + id*0x20, xts_bdf_pid);
>> +    if (!p->ctx_ref[id]) {
>> +        NPU2DBG(p, "XTS_PID_MAP[%03d] = 0x%08llx\n", id, xts_bdf_pid);
>> +        npu2_write(p, NPU2_XTS_PID_MAP + id*0x20, xts_bdf_pid);
>> +    }
>> +    ++p->ctx_ref[id];
>>
>>     if (!GETFIELD(NPU2_XTS_BDF_MAP_VALID, xts_bdf)) {
>>         xts_bdf = SETFIELD(NPU2_XTS_BDF_MAP_VALID, xts_bdf, 1);
>> @@ -2155,7 +2165,7 @@ static int opal_npu_destroy_context(uint64_t
>> phb_id, uint64_t pid __unused,
>>     struct phb *phb = pci_get_phb(phb_id);
>>     struct npu2 *p;
>>     uint64_t xts_bdf;
>> -    int rc = 0;
>> +    int rc = 0, id;
>>
>>     if (!phb || phb->phb_type != phb_type_npu_v2)
>>         return OPAL_PARAMETER;
>> @@ -2172,10 +2182,24 @@ static int opal_npu_destroy_context(uint64_t
>> phb_id, uint64_t pid __unused,
>>     }
>>
>>     /*
>> -     * The bdf/pid table only contains wildcard entries, so we don't
>> -     * need to remove anything here.
>> +     * The bdf/pid table contains wildcard entries and MSR bits which
>> +     * we need to clear between switching a device from a host to a
>> guest
>> +     * or vice versa.
>>      */
>> -
>> +    id = GETFIELD(NPU2_XTS_BDF_MAP_LPARSHORT, xts_bdf);
>> +    if (id >= NPU2_XTS_BDF_MAP_SIZE) {
>> +        NPU2ERR(p, "Out of bounds id=%d\n", id);
>> +        rc = OPAL_INTERNAL_ERROR;
> 
> Same.

Yeah, I'll ditch this one.


> 
>> +    } else {
>> +        --p->ctx_ref[id];
>> +        rc = p->ctx_ref[id]; /* For debug */
>> +        if (p->ctx_ref[id] < 0) {
>> +            rc = OPAL_INTERNAL_ERROR; /* Sanity check */
> 
> If somebody (for whatever reason) calls opal_npu_destroy_context()
> repeatedly for an already-destroyed ID, you don't want the refcount to
> go more and more negative.
> 
> I think instead of checking if it is negative afterwards, maybe check to
> see if it is zero before, and return OPAL_PARAMETER.

True. Thanks for the review. Now please review the kernel patchset.
Thanks :)

> 
>> +        } else if (!p->ctx_ref[id]) {
>> +            NPU2DBG(p, "XTS_PID_MAP[%03d] = 0 (destroy)\n", id);
>> +            npu2_write(p, NPU2_XTS_PID_MAP + id*0x20, 0);
>> +        }
>> +    }
>>     unlock(&p->lock);
>>     return rc;
>> }
> 
> 

-- 
Alexey