[Skiboot] [PATCH skiboot v2] npu2: Add XTS_BDF_MAP wildcard refcount
Alexey Kardashevskiy
aik at ozlabs.ru
Thu Nov 29 14:26:15 AEDT 2018
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
More information about the Skiboot
mailing list