| Message ID | 5694A9B1.4090707@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 12/01/2016 08:22, Cao jin wrote: > 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 */ I don't know; I would have to check all calls to pci_add_capability2. The other patch you had instead was easier to review. I would start with a simpler patch that adds "assert(size >= 2)" and "assert(offset >= PCI_CONFIG_HEADER_SIZE)"; that's the bare minimum needed to avoid messing up the capability list. Paolo
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);