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
