Re: [PATCH v4 5/6] iommu/amd: Modify clear_dte_entry() to avoid in-place update

[Suthikulpanit, Suravee]

On 9/27/2024 2:54 AM, Jason Gunthorpe wrote:
> On Mon, Sep 16, 2024 at 05:18:04PM +0000, Suravee Suthikulpanit wrote:
>
> ....
>
> I suggest you change this slightly so the flow is more like
>
> make_clear_dte(..., struct dev_table_entry *entry) {..}
>
> Which would have most of the above. Then:
>
> clear_dte_entry()
> {
>      struct dev_table_entry target;
>
>      make_clear_dte(.., &target);
>      update_dte256(iommu, dev_data, &new);
> }
>
> And then in the prior patches you can write like:
>
> static void make_dte_gcr3_table(struct amd_iommu *iommu,
>                                struct iommu_dev_data *dev_data,
>                                struct dev_table_entry *target)
> {
>      make_clear_dte(.., &target);
>      ...
> }
>
> And drop all the wild masking:
>
> +       /* First mask out possible old values for GCR3 table */
> +       tmp = DTE_GCR3_VAL_A(~0ULL) << DTE_GCR3_SHIFT_A;
> +       target->data[0] &= ~tmp;
> +       tmp = DTE_GCR3_VAL_B(~0ULL) << DTE_GCR3_SHIFT_B;
> +       tmp |= DTE_GCR3_VAL_C(~0ULL) << DTE_GCR3_SHIFT_C;
> +       target->data[1] &= ~tmp;
>
> Since make_clear_dte() already zero'd these fields.

Thanks for suggestion. I'll clean this up.

Suravee