Re: [PATCH 03/11] PCI: endpoint: pci-epf-vntb: Add packed MW layout handling

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:
- [Medium] The packed_mws feature is entirely unreachable as it cannot be c=
onfigured by userspace.
- [Medium] Outbound ATU mapping is completely broken for packed memory wind=
ows due to explicit rejection.
- [Medium] Inbound translations for packed configurations are unusable and =
dynamically corrupt the shared BAR.
--

--- Patch [3]: [PATCH 03/11] PCI: endpoint: pci-epf-vntb: Add packed MW lay=
out handling ---
commit b1065455da6e47589efee078ad0c8d01095b3f1a
Author: Koichiro Den <[email protected]>
PCI: endpoint: pci-epf-vntb: Add packed MW layout handling

Teach pci-epf-vntb to build a packed layout and advertise its logical
MWs as one version 1 translation group. Non-packed configurations keep
the version 0 layout and one MW per BAR.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/=
endpoint/functions/pci-epf-vntb.c
> index c3caec927d748..f22459e5a8c69 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -129,17 +132,27 @@ struct epf_ntb_ctrl {
>  	u32 db_entry_size;
>  	u32 db_data[MAX_DB_COUNT];
>  	u32 db_offset[MAX_DB_COUNT];
> +	u32 mw_bar;
> +	u32 mw_group_size;
>  } __packed;
> =20
> +struct epf_ntb_mw_layout {
> +	enum pci_barno barno;
> +	u64 offset;
> +	u64 size;
> +};
> +
>  struct epf_ntb {
>  	struct ntb_dev ntb;
>  	struct pci_epf *epf;
>  	struct config_group group;
> =20
>  	u32 num_mws;
> +	u32 packed_mws;

[Severity: Medium]
Will userspace be able to configure packed_mws?

It appears that there is no EPF_NTB_W(packed_mws) macro or a corresponding
entry in the epf_ntb_attrs[] configfs array, which means this field might
not be writable by userspace.

Additionally, looking at epf_ntb_num_mws_store(), it restricts num_mws to
MAX_MW (4), so the new EPF_NTB_MAX_MW (16) limit for packed configurations
cannot be reached.

[ ... ]

> @@ -227,13 +254,23 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb=
, u32 mw)
>  {
>  	phys_addr_t phys_addr;
>  	u8 func_no, vfunc_no;
> +	unsigned int count;
>  	u64 addr, size;
> -	int ret =3D 0;
> +	int ret;
> +
> +	ret =3D epf_ntb_get_mw_group(ntb, mw, NULL, &count);
> +	if (ret)
> +		return ret;
> +	if (count > 1)
> +		return -EOPNOTSUPP;

[Severity: Medium]
How will outbound memory windows be mapped for packed layouts?

If count > 1 is explicitly rejected here with -EOPNOTSUPP, and there is no
COMMAND_CONFIGURE_MW_GROUP handler in epf_ntb_cmd_handler() to manage packed
layouts, will the NTB host always receive a COMMAND_STATUS_ERROR when
attempting to map an outbound grouped window?

[ ... ]

> @@ -1656,6 +1838,7 @@ static const struct ntb_dev_ops vntb_epf_ops =3D {
>  	.db_vector_count	=3D vntb_epf_db_vector_count,
>  	.db_vector_mask		=3D vntb_epf_db_vector_mask,
>  	.db_set_mask		=3D vntb_epf_db_set_mask,
> +	.mw_get_trans_group	=3D vntb_epf_mw_get_trans_group,

[Severity: Medium]
Are the .mw_set_trans_group and .mw_clear_trans_group callbacks missing?

Without the group callbacks, the NTB subsystem falls back to using
.mw_set_trans (vntb_epf_mw_set_trans), which locklessly overwrites
epf_bar->phys_addr and epf_bar->size with a single segment's attributes.

Because all segments of a packed MW share the same BAR, won't calling this
for each segment continuously overwrite the entire BAR's inbound translation
and corrupt the mappings for all other windows in the group?

>  	.mw_set_trans		=3D vntb_epf_mw_set_trans,
>  	.mw_clear_trans		=3D vntb_epf_mw_clear_trans,
>  	.peer_mw_get_addr	=3D vntb_epf_peer_mw_get_addr,

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803180439.1963=
[email protected]?part=3D3