linux-kernel - Re: Re: [PATCH] PCI: Remove redundant macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4c254df0.780.193cd0f0589.Coremail.zhangdongdong@eswincomputing.com>
Date: Mon, 16 Dec 2024 09:22:14 +0800 (GMT+08:00)
From: DongdongZhang <zhangdongdong@...incomputing.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>
Cc: alex.williamson@...hat.com, bhelgaas@...gle.com, yishaih@...dia.com,
	avihaih@...dia.com, yi.l.liu@...el.com, ankita@...dia.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: Re: [PATCH] PCI: Remove redundant macro


Hi Bjorn,  


Thank you for reviewing my patch and providing detailed feedback!  


I agree with your observation regarding the mismatch left between 
`PCI_VNDR_HEADER` and `PCI_VSEC_HDR_LEN_SHIFT`. Based on your suggestion, 
I plan to address this by:  


1. Removing `PCI_VSEC_HDR_LEN_SHIFT` entirely.  
2. Updating the VFIO PCI code to use `PCI_VNDR_HEADER_LEN()` instead, 
   ensuring consistent naming and functionality.  
3. Justifying the removal of these macros (`PCI_VSEC_HDR` and `PCI_VSEC_HDR_LEN_SHIFT`) 
   in `pci_regs.h`, despite it being part of `include/uapi/`. As you noted, 
   this is a niche case, and the impact on userspace should be minimal.  


I’ll send an updated patch shortly, incorporating these changes and 
testing to ensure everything works as expected.  


Thanks again for your insights! Please let me know if there’s anything 
else I should address in the revised patch.  


Best regards,  
Dongdong Zhang  



> -----原始邮件-----
> 发件人: "Bjorn Helgaas" <helgaas@...nel.org>
> 发送时间:2024-12-14 01:37:01 (星期六)
> 收件人: zhangdongdong@...incomputing.com
> 抄送: alex.williamson@...hat.com, bhelgaas@...gle.com, yishaih@...dia.com, avihaih@...dia.com, yi.l.liu@...el.com, ankita@...dia.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
> 主题: Re: [PATCH] PCI: Remove redundant macro
> 
> On Fri, Dec 13, 2024 at 05:46:17PM +0800, zhangdongdong@...incomputing.com wrote:
> > From: Dongdong Zhang <zhangdongdong@...incomputing.com>
> > 
> > Removed the duplicate macro definition PCI_VSEC_HDR from
> > pci_regs.h to avoid redundancy. Updated the VFIO PCI code
> > to use the existing `PCI_VNDR_HEADER` macro for consistency,
> > ensuring minimal changes to the codebase.
> > 
> > Signed-off-by: Dongdong Zhang <zhangdongdong@...incomputing.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_config.c | 3 ++-
> >  include/uapi/linux/pci_regs.h      | 1 -
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> > index ea2745c1ac5e..c30748912ff1 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -1389,7 +1389,8 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
> >  
> >  	switch (ecap) {
> >  	case PCI_EXT_CAP_ID_VNDR:
> > -		ret = pci_read_config_dword(pdev, epos + PCI_VSEC_HDR, &dword);
> > +		ret = pci_read_config_dword(pdev, epos + PCI_VNDR_HEADER,
> > +					    &dword);
> >  		if (ret)
> >  			return pcibios_err_to_errno(ret);
> >  
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index 1601c7ed5fab..7b6cad788de3 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -1001,7 +1001,6 @@
> >  #define PCI_ACS_CTRL		0x06	/* ACS Control Register */
> >  #define PCI_ACS_EGRESS_CTL_V	0x08	/* ACS Egress Control Vector */
> >  
> > -#define PCI_VSEC_HDR		4	/* extended cap - vendor-specific */
> >  #define  PCI_VSEC_HDR_LEN_SHIFT	20	/* shift for length field */
> 
> We should resolve the duplication of PCI_VSEC_HDR and PCI_VNDR_HEADER,
> but I don't like the fact that we're left with this dangling
> PCI_VSEC_HDR_LEN_SHIFT.
> 
> That leaves vfio using PCI_VNDR_HEADER and PCI_VSEC_HDR_LEN_SHIFT,
> which don't match at all.
> 
> I think you should remove PCI_VSEC_HDR_LEN_SHIFT as well and change
> vfio to use PCI_VNDR_HEADER_LEN() instead.
> 
> It's somewhat dicey removing things from pci_regs.h since it's in
> include/uapi/, but this is such a niche thing we might be able to get
> away with it.
> 
> Bjorn