[v1] pci: Disallow improper BAR registration for type 1

[PATCH RESEND] pci: Disallow improper BAR registration for type 1

Posted by Ben Widawsky 3 years, 6 months ago

This patch informs future developers working on root complexes, root
ports, or bridges that also wish to implement a BAR for those. PCI type
1 headers only support 2 base address registers. It is incorrect and
difficult to figure out what is wrong with the device when this mistake
is made. With this, it is immediate and obvious what has gone wrong.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 hw/pci/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3c8f10b461..55b0302c57 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1141,6 +1141,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     uint32_t addr; /* offset in pci config space */
     uint64_t wmask;
     pcibus_t size = memory_region_size(memory);
+    uint8_t hdr_type;
 
     assert(region_num >= 0);
     assert(region_num < PCI_NUM_REGIONS);
@@ -1150,6 +1151,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
         exit(1);
     }
 
+    hdr_type =
+        pci_dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+    if (hdr_type == PCI_HEADER_TYPE_BRIDGE && region_num > 1) {
+        error_report("ERROR: PCI Type 1 header only has 2 BARs "
+                     "requested BAR=%d",
+                     region_num);
+        exit(1);
+    }
+
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
-- 
2.28.0

Re: [PATCH RESEND] pci: Disallow improper BAR registration for type 1

Posted by Michael S. Tsirkin 3 years, 6 months ago

On Wed, Oct 14, 2020 at 10:18:53AM -0700, Ben Widawsky wrote:
> This patch informs future developers working on root complexes, root
> ports, or bridges that also wish to implement a BAR for those. PCI type
> 1 headers only support 2 base address registers. It is incorrect and
> difficult to figure out what is wrong with the device when this mistake
> is made. With this, it is immediate and obvious what has gone wrong.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

How about an assert + a comment?
This is how we usually handle developer bugs. error_report is
for user errors and similar issues ...

> ---
>  hw/pci/pci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 3c8f10b461..55b0302c57 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1141,6 +1141,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      uint32_t addr; /* offset in pci config space */
>      uint64_t wmask;
>      pcibus_t size = memory_region_size(memory);
> +    uint8_t hdr_type;
>  
>      assert(region_num >= 0);
>      assert(region_num < PCI_NUM_REGIONS);
> @@ -1150,6 +1151,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>          exit(1);
>      }
>  
> +    hdr_type =
> +        pci_dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    if (hdr_type == PCI_HEADER_TYPE_BRIDGE && region_num > 1) {
> +        error_report("ERROR: PCI Type 1 header only has 2 BARs "
> +                     "requested BAR=%d",
> +                     region_num);
> +        exit(1);
> +    }
> +
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
> -- 
> 2.28.0

Re: [PATCH RESEND] pci: Disallow improper BAR registration for type 1

Posted by Ben Widawsky 3 years, 6 months ago

On 20-10-14 13:52:29, Michael S. Tsirkin wrote:
> On Wed, Oct 14, 2020 at 10:18:53AM -0700, Ben Widawsky wrote:
> > This patch informs future developers working on root complexes, root
> > ports, or bridges that also wish to implement a BAR for those. PCI type
> > 1 headers only support 2 base address registers. It is incorrect and
> > difficult to figure out what is wrong with the device when this mistake
> > is made. With this, it is immediate and obvious what has gone wrong.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> How about an assert + a comment?
> This is how we usually handle developer bugs. error_report is
> for user errors and similar issues ...

Sure, I can do that. I was copying the error above mine which should probably
also be changed to an assert then, yes?

I can submit that as well with v2 of this if you'd like.

> 
> > ---
> >  hw/pci/pci.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 3c8f10b461..55b0302c57 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1141,6 +1141,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> >      uint32_t addr; /* offset in pci config space */
> >      uint64_t wmask;
> >      pcibus_t size = memory_region_size(memory);
> > +    uint8_t hdr_type;
> >  
> >      assert(region_num >= 0);
> >      assert(region_num < PCI_NUM_REGIONS);
> > @@ -1150,6 +1151,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> >          exit(1);
> >      }
> >  
> > +    hdr_type =
> > +        pci_dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > +    if (hdr_type == PCI_HEADER_TYPE_BRIDGE && region_num > 1) {
> > +        error_report("ERROR: PCI Type 1 header only has 2 BARs "
> > +                     "requested BAR=%d",
> > +                     region_num);
> > +        exit(1);
> > +    }
> > +
> >      r = &pci_dev->io_regions[region_num];
> >      r->addr = PCI_BAR_UNMAPPED;
> >      r->size = size;
> > -- 
> > 2.28.0
>

Re: [PATCH RESEND] pci: Disallow improper BAR registration for type 1

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 10/14/20 7:56 PM, Ben Widawsky wrote:
> On 20-10-14 13:52:29, Michael S. Tsirkin wrote:
>> On Wed, Oct 14, 2020 at 10:18:53AM -0700, Ben Widawsky wrote:
>>> This patch informs future developers working on root complexes, root
>>> ports, or bridges that also wish to implement a BAR for those. PCI type
>>> 1 headers only support 2 base address registers. It is incorrect and
>>> difficult to figure out what is wrong with the device when this mistake
>>> is made. With this, it is immediate and obvious what has gone wrong.
>>>
>>> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>>
>> How about an assert + a comment?
>> This is how we usually handle developer bugs. error_report is
>> for user errors and similar issues ...
> 
> Sure, I can do that. I was copying the error above mine which should probably
> also be changed to an assert then, yes?
> 
> I can submit that as well with v2 of this if you'd like.

Yes, and please don't start with "This patch ...":

https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

> 
>>
>>> ---
>>>   hw/pci/pci.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index 3c8f10b461..55b0302c57 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -1141,6 +1141,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>>>       uint32_t addr; /* offset in pci config space */
>>>       uint64_t wmask;
>>>       pcibus_t size = memory_region_size(memory);
>>> +    uint8_t hdr_type;
>>>   
>>>       assert(region_num >= 0);
>>>       assert(region_num < PCI_NUM_REGIONS);
>>> @@ -1150,6 +1151,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>>>           exit(1);
>>>       }
>>>   
>>> +    hdr_type =
>>> +        pci_dev->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
>>> +    if (hdr_type == PCI_HEADER_TYPE_BRIDGE && region_num > 1) {
>>> +        error_report("ERROR: PCI Type 1 header only has 2 BARs "
>>> +                     "requested BAR=%d",
>>> +                     region_num);
>>> +        exit(1);
>>> +    }
>>> +
>>>       r = &pci_dev->io_regions[region_num];
>>>       r->addr = PCI_BAR_UNMAPPED;
>>>       r->size = size;
>>> -- 
>>> 2.28.0
>>
>