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
