[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 3/4] xen/vpci: header: status register handler
On 9/11/23 07:10, Jan Beulich wrote: > On 09.09.2023 04:16, Stewart Hildebrand wrote: >> @@ -544,6 +545,18 @@ static int cf_check init_bars(struct pci_dev *pdev) >> if ( rc ) >> return rc; >> >> + /* >> + * If mask_cap_list is true, PCI_STATUS_CAP_LIST will be set in both >> + * rsvdz_mask and ro_mask, and thus will effectively behave as rsvdp >> + * (reserved/RAZ and preserved on write). >> + */ >> + rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16, >> + PCI_STATUS, 2, header, >> PCI_STATUS_RSVDZ_MASK | >> + (mask_cap_list ? PCI_STATUS_CAP_LIST : 0), >> + PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK); > > While not formally disallowed by ./CODING_STYLE, I think this kind of argument > wrapping is bad practice. If an argument is too long for the current line, it > should start on a fresh one. Otherwise at the very least I think we'd want the > continuation to stand out, by parenthesizing the entire argument, thus leading > to one deeper indentation on the continuing line(s). (This may even be useful > to do when starting on a fresh line.) I will change it to this: /* * Utilize rsvdz_mask to hide PCI_STATUS_CAP_LIST from the guest for now. If * support for rsvdp (reserved & preserved) is added in the future, the * rsvdp mask should be used instead. */ rc = vpci_add_register_mask(pdev->vpci, vpci_hw_read16, vpci_hw_write16, PCI_STATUS, 2, NULL, PCI_STATUS_RSVDZ_MASK | (mask_cap_list ? PCI_STATUS_CAP_LIST : 0), PCI_STATUS_RO_MASK & ~(mask_cap_list ? PCI_STATUS_CAP_LIST : 0), PCI_STATUS_RW1C_MASK); >> @@ -155,7 +165,8 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t >> *read_handler, >> /* Some sanity checks. */ >> if ( (size != 1 && size != 2 && size != 4) || >> offset >= PCI_CFG_SPACE_EXP_SIZE || (offset & (size - 1)) || >> - (!read_handler && !write_handler) ) >> + (!read_handler && !write_handler) || (ro_mask & rw1c_mask) || >> + (rsvdz_mask & rw1c_mask) ) >> return -EINVAL; > > What about rsvdz_mask and ro_mask? They better wouldn't overlap either > (requiring an adjustment to the code you add to init_bars()). I will add a case for (rsvdz_mask & ro_mask). See above for adjustment in init_bars(). >> @@ -407,26 +439,25 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, >> unsigned int size) >> >> /* >> * Perform a maybe partial write to a register. >> - * >> - * Note that this will only work for simple registers, if Xen needs to >> - * trap accesses to rw1c registers (like the status PCI header register) >> - * the logic in vpci_write will have to be expanded in order to correctly >> - * deal with them. >> */ >> static void vpci_write_helper(const struct pci_dev *pdev, >> const struct vpci_register *r, unsigned int >> size, >> unsigned int offset, uint32_t data) >> { >> + uint32_t val = 0; >> + >> ASSERT(size <= r->size); >> >> - if ( size != r->size ) >> + if ( (size != r->size) || r->ro_mask ) >> { >> - uint32_t val; >> - >> val = r->read(pdev, r->offset, r->private); >> + val &= ~r->rw1c_mask; >> data = merge_result(val, data, size, offset); >> } >> >> + data &= ~r->rsvdz_mask & ~r->ro_mask; > > ~(r->rsvdz_mask | r->ro_mask) ? But maybe that's indeed just me ... I will make this change.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |