Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Cao jin]

Thanks for your time. I almost forget this one...

On 01/11/2016 05:20 PM, Paolo Bonzini wrote:
> On 11/01/2016 09:32, Michael Tokarev wrote:
> > > > +    assert(size > 0);
> > > > +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
> > > > +
> > I'd like to see some ACKs/Reviews for this one, in particular why
> > size should be != 0.
>
> In fact it should be >= 2, because two bytes are always written below:
>
>      config = pdev->config + offset;
>      config[PCI_CAP_LIST_ID] = cap_id;
>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
>
> > Also either move offset assert to the below
> > "else" clause or rewrite it to be offset == 0 instead if !offset :)
>
> Good idea to move it below, or even to add
>
>      assert(offset >= PCI_CONFIG_HEADER_SIZE);
>
> after the "if", before the "config" assignment.
>
> Paolo

Seems I missed that offset == 0 will lead to find a suitable space in 
pci_find_space, and ensure offset >= PCI_CONFIG_HEADER_SIZE. sorry for 
the carelessness mistake.

According to the spec(PCI local spec, chapter 6.3), capability structure 
should be at DWORD boundary and DWORD aligned, so in both 
condition(if...else...), it should follow the spec

if offset == 0, with following line[*], seems it is ok with align issue.

[*] memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));

The else-branch should ensure these too.

Another little question, shouldn`t we check size at first by:

   assert((size % 4) && (size > 0))  ?

I think if caller ensure the effective param maybe it is easier to read, 
so how about following:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..47cb509 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2144,6 +2144,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
     uint8_t *config;
     int i, overlapping_cap;

+    assert(!(size % 4) && (size > 0));
+
     if (!offset) {
         offset = pci_find_space(pdev, size);
         if (!offset) {
@@ -2155,6 +2157,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
          * depends on this check to verify that the device is not broken.
          * Should never trigger for emulated devices, but it's helpful
          * for debugging these. */
+        assert(!(offset % 4));
         for (i = offset; i < offset + size; i++) {
             overlapping_cap = pci_find_capability_at_offset(pdev, i);
             if (overlapping_cap) {
@@ -2174,7 +2177,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
     config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
     pdev->config[PCI_CAPABILITY_LIST] = offset;
     pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
-    memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
+    memset(pdev->used + offset, 0xFF, size);
     /* Make capability read-only by default */
     memset(pdev->wmask + offset, 0, size);
     /* Check capability by default */

--
Yours Sincerely,

Cao jin