* [PATCH] vfio-pci: Mask cap zero @ 2020-05-01 21:41 Alex Williamson 2020-05-04 16:09 ` Cornelia Huck 0 siblings, 1 reply; 7+ messages in thread From: Alex Williamson @ 2020-05-01 21:41 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, cohuck There is no PCI spec defined capability with ID 0, therefore we don't expect to find it in a capability chain and we use this index in an internal array for tracking the sizes of various capabilities to handle standard config space. Therefore if a device does present us with a capability ID 0, we mark our capability map with nonsense that can trigger conflicts with other capabilities in the chain. Ignore ID 0 when walking the capability chain, handling it as a hidden capability. Seen on an NVIDIA Tesla T4. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/vfio/pci/vfio_pci_config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c index 87d0cc8c86ad..5935a804cb88 100644 --- a/drivers/vfio/pci/vfio_pci_config.c +++ b/drivers/vfio/pci/vfio_pci_config.c @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) if (ret) return ret; - if (cap <= PCI_CAP_ID_MAX) { + if (cap && cap <= PCI_CAP_ID_MAX) { len = pci_cap_length[cap]; if (len == 0xFF) { /* Variable length */ len = vfio_cap_len(vdev, cap, pos); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-01 21:41 [PATCH] vfio-pci: Mask cap zero Alex Williamson @ 2020-05-04 16:09 ` Cornelia Huck 2020-05-04 18:52 ` Alex Williamson 0 siblings, 1 reply; 7+ messages in thread From: Cornelia Huck @ 2020-05-04 16:09 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, linux-kernel On Fri, 01 May 2020 15:41:24 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > There is no PCI spec defined capability with ID 0, therefore we don't > expect to find it in a capability chain and we use this index in an > internal array for tracking the sizes of various capabilities to handle > standard config space. Therefore if a device does present us with a > capability ID 0, we mark our capability map with nonsense that can > trigger conflicts with other capabilities in the chain. Ignore ID 0 > when walking the capability chain, handling it as a hidden capability. > > Seen on an NVIDIA Tesla T4. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/vfio/pci/vfio_pci_config.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > index 87d0cc8c86ad..5935a804cb88 100644 > --- a/drivers/vfio/pci/vfio_pci_config.c > +++ b/drivers/vfio/pci/vfio_pci_config.c > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > if (ret) > return ret; > > - if (cap <= PCI_CAP_ID_MAX) { Maybe add a comment: /* no PCI spec defined capability with ID 0: hide it */ ? > + if (cap && cap <= PCI_CAP_ID_MAX) { > len = pci_cap_length[cap]; > if (len == 0xFF) { /* Variable length */ > len = vfio_cap_len(vdev, cap, pos); > Is there a requirement for caps to be strictly ordered? If not, could len hold a residual value from a previous iteration? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-04 16:09 ` Cornelia Huck @ 2020-05-04 18:52 ` Alex Williamson 2020-05-04 22:08 ` Neo Jia 0 siblings, 1 reply; 7+ messages in thread From: Alex Williamson @ 2020-05-04 18:52 UTC (permalink / raw) To: Cornelia Huck; +Cc: kvm, linux-kernel On Mon, 4 May 2020 18:09:16 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 01 May 2020 15:41:24 -0600 > Alex Williamson <alex.williamson@redhat.com> wrote: > > > There is no PCI spec defined capability with ID 0, therefore we don't > > expect to find it in a capability chain and we use this index in an > > internal array for tracking the sizes of various capabilities to handle > > standard config space. Therefore if a device does present us with a > > capability ID 0, we mark our capability map with nonsense that can > > trigger conflicts with other capabilities in the chain. Ignore ID 0 > > when walking the capability chain, handling it as a hidden capability. > > > > Seen on an NVIDIA Tesla T4. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > drivers/vfio/pci/vfio_pci_config.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > index 87d0cc8c86ad..5935a804cb88 100644 > > --- a/drivers/vfio/pci/vfio_pci_config.c > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > > if (ret) > > return ret; > > > > - if (cap <= PCI_CAP_ID_MAX) { > > Maybe add a comment: > > /* no PCI spec defined capability with ID 0: hide it */ > Sure. > > > + if (cap && cap <= PCI_CAP_ID_MAX) { > > len = pci_cap_length[cap]; > > if (len == 0xFF) { /* Variable length */ > > len = vfio_cap_len(vdev, cap, pos); > > > > Is there a requirement for caps to be strictly ordered? If not, could > len hold a residual value from a previous iteration? There is no ordering requirement for capabilities, but len is declared non-static with an initial value within the scope of the loop, it's reset every iteration. Thanks, Alex ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-04 18:52 ` Alex Williamson @ 2020-05-04 22:08 ` Neo Jia 2020-05-04 23:03 ` Alex Williamson 0 siblings, 1 reply; 7+ messages in thread From: Neo Jia @ 2020-05-04 22:08 UTC (permalink / raw) To: Alex Williamson; +Cc: Cornelia Huck, kvm, linux-kernel On Mon, May 04, 2020 at 12:52:53PM -0600, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Mon, 4 May 2020 18:09:16 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Fri, 01 May 2020 15:41:24 -0600 > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > There is no PCI spec defined capability with ID 0, therefore we don't > > > expect to find it in a capability chain and we use this index in an > > > internal array for tracking the sizes of various capabilities to handle > > > standard config space. Therefore if a device does present us with a > > > capability ID 0, we mark our capability map with nonsense that can > > > trigger conflicts with other capabilities in the chain. Ignore ID 0 > > > when walking the capability chain, handling it as a hidden capability. > > > > > > Seen on an NVIDIA Tesla T4. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > drivers/vfio/pci/vfio_pci_config.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > > index 87d0cc8c86ad..5935a804cb88 100644 > > > --- a/drivers/vfio/pci/vfio_pci_config.c > > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > > > if (ret) > > > return ret; > > > > > > - if (cap <= PCI_CAP_ID_MAX) { > > > > Maybe add a comment: > > > > /* no PCI spec defined capability with ID 0: hide it */ Hi Alex, I think this is NULL Capability defined in Codes and IDs spec, probably we should just add a new enum to represent that? Thanks, Neo > > > > Sure. > > > > > > + if (cap && cap <= PCI_CAP_ID_MAX) { > > > len = pci_cap_length[cap]; > > > if (len == 0xFF) { /* Variable length */ > > > len = vfio_cap_len(vdev, cap, pos); > > > > > > > Is there a requirement for caps to be strictly ordered? If not, could > > len hold a residual value from a previous iteration? > > There is no ordering requirement for capabilities, but len is declared > non-static with an initial value within the scope of the loop, it's > reset every iteration. Thanks, > > Alex > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-04 22:08 ` Neo Jia @ 2020-05-04 23:03 ` Alex Williamson 2020-05-05 6:09 ` Cornelia Huck 0 siblings, 1 reply; 7+ messages in thread From: Alex Williamson @ 2020-05-04 23:03 UTC (permalink / raw) To: Neo Jia; +Cc: Cornelia Huck, kvm, linux-kernel On Mon, 4 May 2020 15:08:08 -0700 Neo Jia <cjia@nvidia.com> wrote: > On Mon, May 04, 2020 at 12:52:53PM -0600, Alex Williamson wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, 4 May 2020 18:09:16 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > On Fri, 01 May 2020 15:41:24 -0600 > > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > > > There is no PCI spec defined capability with ID 0, therefore we don't > > > > expect to find it in a capability chain and we use this index in an > > > > internal array for tracking the sizes of various capabilities to handle > > > > standard config space. Therefore if a device does present us with a > > > > capability ID 0, we mark our capability map with nonsense that can > > > > trigger conflicts with other capabilities in the chain. Ignore ID 0 > > > > when walking the capability chain, handling it as a hidden capability. > > > > > > > > Seen on an NVIDIA Tesla T4. > > > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > --- > > > > drivers/vfio/pci/vfio_pci_config.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > > > index 87d0cc8c86ad..5935a804cb88 100644 > > > > --- a/drivers/vfio/pci/vfio_pci_config.c > > > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > > > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > > > > if (ret) > > > > return ret; > > > > > > > > - if (cap <= PCI_CAP_ID_MAX) { > > > > > > Maybe add a comment: > > > > > > /* no PCI spec defined capability with ID 0: hide it */ > > Hi Alex, > > I think this is NULL Capability defined in Codes and IDs spec, probably we > should just add a new enum to represent that? Yes, it looks like the 1.1 version of that specification from June 2015 changed ID 0 from reserved to a NULL capability. So my description and this comment are wrong, but I wonder if we should did anything different with the handling of this capability. It's specified to contain only the ID and next pointer, so I'd expect it's primarily a mechanism for hardware vendors to blow fuses in config space to maintain a capability chain while maybe hiding a feature not supported by the product sku. Hiding the capability in vfio is trivial, exposing it implies some changes to our config space map that might be more subtle. I'm inclined to stick with this solution for now. Thanks, Alex > > > > Sure. > > > > > > > > > + if (cap && cap <= PCI_CAP_ID_MAX) { > > > > len = pci_cap_length[cap]; > > > > if (len == 0xFF) { /* Variable length */ > > > > len = vfio_cap_len(vdev, cap, pos); > > > > > > > > > > Is there a requirement for caps to be strictly ordered? If not, could > > > len hold a residual value from a previous iteration? > > > > There is no ordering requirement for capabilities, but len is declared > > non-static with an initial value within the scope of the loop, it's > > reset every iteration. Thanks, > > > > Alex > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-04 23:03 ` Alex Williamson @ 2020-05-05 6:09 ` Cornelia Huck 2020-05-05 21:58 ` Neo Jia 0 siblings, 1 reply; 7+ messages in thread From: Cornelia Huck @ 2020-05-05 6:09 UTC (permalink / raw) To: Alex Williamson; +Cc: Neo Jia, kvm, linux-kernel On Mon, 4 May 2020 17:03:54 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 4 May 2020 15:08:08 -0700 > Neo Jia <cjia@nvidia.com> wrote: > > > On Mon, May 04, 2020 at 12:52:53PM -0600, Alex Williamson wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On Mon, 4 May 2020 18:09:16 +0200 > > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > On Fri, 01 May 2020 15:41:24 -0600 > > > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > > > > > There is no PCI spec defined capability with ID 0, therefore we don't > > > > > expect to find it in a capability chain and we use this index in an > > > > > internal array for tracking the sizes of various capabilities to handle > > > > > standard config space. Therefore if a device does present us with a > > > > > capability ID 0, we mark our capability map with nonsense that can > > > > > trigger conflicts with other capabilities in the chain. Ignore ID 0 > > > > > when walking the capability chain, handling it as a hidden capability. > > > > > > > > > > Seen on an NVIDIA Tesla T4. > > > > > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > > --- > > > > > drivers/vfio/pci/vfio_pci_config.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > > > > index 87d0cc8c86ad..5935a804cb88 100644 > > > > > --- a/drivers/vfio/pci/vfio_pci_config.c > > > > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > > > > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > - if (cap <= PCI_CAP_ID_MAX) { > > > > > > > > Maybe add a comment: > > > > > > > > /* no PCI spec defined capability with ID 0: hide it */ > > > > Hi Alex, > > > > I think this is NULL Capability defined in Codes and IDs spec, probably we > > should just add a new enum to represent that? > > Yes, it looks like the 1.1 version of that specification from June 2015 > changed ID 0 from reserved to a NULL capability. So my description and > this comment are wrong, but I wonder if we should did anything > different with the handling of this capability. It's specified to > contain only the ID and next pointer, so I'd expect it's primarily a > mechanism for hardware vendors to blow fuses in config space to > maintain a capability chain while maybe hiding a feature not supported > by the product sku. Hiding the capability in vfio is trivial, exposing > it implies some changes to our config space map that might be more > subtle. I'm inclined to stick with this solution for now. Thanks, > > Alex From this description, I also think that we should simply hide these NULL capabilities. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfio-pci: Mask cap zero 2020-05-05 6:09 ` Cornelia Huck @ 2020-05-05 21:58 ` Neo Jia 0 siblings, 0 replies; 7+ messages in thread From: Neo Jia @ 2020-05-05 21:58 UTC (permalink / raw) To: Cornelia Huck; +Cc: Alex Williamson, kvm, linux-kernel On Tue, May 05, 2020 at 08:09:39AM +0200, Cornelia Huck wrote: > External email: Use caution opening links or attachments > > > On Mon, 4 May 2020 17:03:54 -0600 > Alex Williamson <alex.williamson@redhat.com> wrote: > > > On Mon, 4 May 2020 15:08:08 -0700 > > Neo Jia <cjia@nvidia.com> wrote: > > > > > On Mon, May 04, 2020 at 12:52:53PM -0600, Alex Williamson wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Mon, 4 May 2020 18:09:16 +0200 > > > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > > > On Fri, 01 May 2020 15:41:24 -0600 > > > > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > > > > > > > There is no PCI spec defined capability with ID 0, therefore we don't > > > > > > expect to find it in a capability chain and we use this index in an > > > > > > internal array for tracking the sizes of various capabilities to handle > > > > > > standard config space. Therefore if a device does present us with a > > > > > > capability ID 0, we mark our capability map with nonsense that can > > > > > > trigger conflicts with other capabilities in the chain. Ignore ID 0 > > > > > > when walking the capability chain, handling it as a hidden capability. > > > > > > > > > > > > Seen on an NVIDIA Tesla T4. > > > > > > > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > > > --- > > > > > > drivers/vfio/pci/vfio_pci_config.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c > > > > > > index 87d0cc8c86ad..5935a804cb88 100644 > > > > > > --- a/drivers/vfio/pci/vfio_pci_config.c > > > > > > +++ b/drivers/vfio/pci/vfio_pci_config.c > > > > > > @@ -1487,7 +1487,7 @@ static int vfio_cap_init(struct vfio_pci_device *vdev) > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > - if (cap <= PCI_CAP_ID_MAX) { > > > > > > > > > > Maybe add a comment: > > > > > > > > > > /* no PCI spec defined capability with ID 0: hide it */ > > > > > > Hi Alex, > > > > > > I think this is NULL Capability defined in Codes and IDs spec, probably we > > > should just add a new enum to represent that? > > > > Yes, it looks like the 1.1 version of that specification from June 2015 > > changed ID 0 from reserved to a NULL capability. So my description and > > this comment are wrong, but I wonder if we should did anything > > different with the handling of this capability. It's specified to > > contain only the ID and next pointer, so I'd expect it's primarily a > > mechanism for hardware vendors to blow fuses in config space to > > maintain a capability chain while maybe hiding a feature not supported > > by the product sku. Hiding the capability in vfio is trivial, exposing > > it implies some changes to our config space map that might be more > > subtle. I'm inclined to stick with this solution for now. Thanks, > > > > Alex > > From this description, I also think that we should simply hide these > NULL capabilities. I don't have a strong preference either way, the current implementation looks fine. Thanks, Neo > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-05 21:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-01 21:41 [PATCH] vfio-pci: Mask cap zero Alex Williamson 2020-05-04 16:09 ` Cornelia Huck 2020-05-04 18:52 ` Alex Williamson 2020-05-04 22:08 ` Neo Jia 2020-05-04 23:03 ` Alex Williamson 2020-05-05 6:09 ` Cornelia Huck 2020-05-05 21:58 ` Neo Jia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.