Re: [PATCH 1/2] platform/x86/intel/tpmi: Read feature control status

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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?
>