On Fri, 2023-06-16 at 10:13 +0300, Ilpo Järvinen wrote:
> On Thu, 15 Jun 2023, Srinivas Pandruvada wrote:
>
> >
[...]
> > + /* set command id to 0x10 for TPMI_GET_STATE */
> > + data = TPMI_GET_STATE_CMD;
> > + /* 32 bits for DATA offset and +8 for feature_id field */
> > + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET +
> > TPMI_GET_STATE_CMD_DATA_OFFSET));
>
> This looks like you should add the GENMASK_ULL() for the fields and
> use
> FIELD_PREP() instead of adding all those OFFSET defines + custom
> shifting.
You mean, I should change one shift instruction, to FIELD_PREP()
which will use three instructions to shift, sub and AND?
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);
>
> > +
> > + /* Write at command offset for qword access */
> > + writeq(data, tpmi_info->tpmi_control_mem +
> > TPMI_COMMAND_OFFSET);
> > +
> > + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND);
> > + if (ret)
> > + goto err_unlock;
> > +
> > + /* Set Run Busy and packet length of 2 dwords */
> > + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN <<
> > TPMI_CMD_PKT_LEN_OFFSET),
>
> Define using BIT_ULL(0) instead. Use FIELD_PREP().
This code will run only on X86 64 bit, not a common device driver which
will run in any architecture.
Please let me know why FIELD_PREP() is better.
>
> I'd drop _BIT from the define name but I leave it up to you, it just
> makes your lines longer w/o much added value.
>
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > +
> > + ret = read_poll_timeout(readq, control, !(control &
> > BIT_ULL(TPMI_CONTROL_RB_BIT)),
> > + TPMI_RB_TIMEOUT_US,
> > TPMI_RB_TIMEOUT_MAX_US, false,
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > + if (ret)
> > + goto done_proc;
> > +
> > + control = FIELD_GET(TPMI_GENMASK_STATUS, control);
> > + if (control != TPMI_CMD_STATUS_SUCCESS) {
> > + ret = -EBUSY;
> > + goto done_proc;
> > + }
> > +
> > + data = readq(tpmi_info->tpmi_control_mem +
> > TPMI_COMMAND_OFFSET);
> > + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for
> > TPMI_DATA */
>
> Define the field with GENMASK() and use FIELD_GET().
>
Again 3 instructions instead of 1.
> > +
> > + *disabled = 0;
> > + *locked = 0;
> > +
> > + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE)))
>
> Put BIT_ULL() into the define.
Good idea.
>
> Perhaps drop _BIT_ from the name.
I can do that.
>
> > + *disabled = 1;
> > +
> > + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED))
>
> Ditto.
>
> > + *locked = 1;
> > +
> > + ret = 0;
> > +
> > +done_proc:
> > + /* SET CPL "completion"bit */
>
> Missing space.
>
OK
> > + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT),
>
> BIT_ULL() to define.
>
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > +
> > +err_unlock:
> > + mutex_unlock(&tpmi_dev_lock);
> > +
> > + return ret;
> > +}
> > +
> > +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int
> > feature_id,
> > + int *locked, int *disabled)
> > +{
> > + struct intel_vsec_device *intel_vsec_dev =
> > dev_to_ivdev(auxdev->dev.parent);
> > + struct intel_tpmi_info *tpmi_info =
> > auxiliary_get_drvdata(&intel_vsec_dev->auxdev);
> > +
> > + return tpmi_read_feature_status(tpmi_info, feature_id,
> > locked, disabled);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI);
> > +
> > +static void tpmi_set_control_base(struct auxiliary_device *auxdev,
> > + struct intel_tpmi_info
> > *tpmi_info,
> > + struct intel_tpmi_pm_feature
> > *pfs)
> > +{
> > + void __iomem *mem;
> > + u16 size;
> > +
> > + size = pfs->pfs_header.num_entries * pfs-
> > >pfs_header.entry_size * 4;
>
> Can this overflow u16? Where does pfs_header content originate from?
We can add a check, but this is coming from a trusted and validated x86
core (Not an add on IP), which not only driver uses but all PM IP in
the hardware.
Thanks,
Srinivas
> If
> from HW, how is it the input validated?
>
