On Thu, Sep 30, 2021 at 10:52:15AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <[email protected]> > > This is in preparation for dynamic assignment of the vPCI register > handlers depending on the domain: hwdom or guest. > The need for this step is that it is easier to have all related functionality > put at one place. When the subsequent patches add decisions on which > handlers to install, e.g. hwdom or guest handlers, then this is easily > achievable.
Won't it be possible to select the handlers to install in init_bars itself? Splitting it like that means you need to iterate over the numbers of BARs twice (one in add_bar_handlers and one in init_bars), which makes it more likely to introduce errors or divergences. Decoupling the filling of vpci_bar data with setting the handlers seems slightly confusing. > Signed-off-by: Oleksandr Andrushchenko <[email protected]> > > --- > Since v1: > - constify struct pci_dev where possible > - extend patch description > --- > xen/drivers/vpci/header.c | 83 ++++++++++++++++++++++++++------------- > 1 file changed, 56 insertions(+), 27 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index f8cd55e7c024..3d571356397a 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -445,6 +445,55 @@ static void rom_write(const struct pci_dev *pdev, > unsigned int reg, > rom->addr = val & PCI_ROM_ADDRESS_MASK; > } > > +static int add_bar_handlers(const struct pci_dev *pdev) Making this const is again misleading IMO, as you end up modifying fields inside the pdev, you get away with it because vpci data is stored in a pointer. > +{ > + unsigned int i; > + struct vpci_header *header = &pdev->vpci->header; > + struct vpci_bar *bars = header->bars; > + int rc; > + > + /* Setup a handler for the command register. */ > + rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, > PCI_COMMAND, > + 2, header); > + if ( rc ) > + return rc; > + > + if ( pdev->ignore_bars ) > + return 0; You can join both ifs above: if ( rc || pdev->ignore_bars ) return rc; > + > + for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ ) init_bars deals with both TYPE_NORMAL and TYPE_BRIDGE classes, yet you seem to unconditionally assume PCI_HEADER_NORMAL_NR_BARS here (even when below you take into account the different ROM BAR position). > + { > + if ( (bars[i].type == VPCI_BAR_IO) || (bars[i].type == > VPCI_BAR_EMPTY) ) > + continue; > + > + if ( bars[i].type == VPCI_BAR_ROM ) > + { > + unsigned int rom_reg; > + uint8_t header_type = pci_conf_read8(pdev->sbdf, > + PCI_HEADER_TYPE) & 0x7f; Missing newline, and unsigned int preferably for header_type. > + if ( header_type == PCI_HEADER_TYPE_NORMAL ) > + rom_reg = PCI_ROM_ADDRESS; > + else > + rom_reg = PCI_ROM_ADDRESS1; > + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, > + rom_reg, 4, &bars[i]); > + if ( rc ) > + return rc; > + } > + else > + { > + uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4; unsigned int please, we try to avoid using explicitly sized types unless strictly necessary (ie: when dealing with hardware values for example). > + > + /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */ > + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, > reg, > + 4, &bars[i]); > + if ( rc ) > + return rc; > + } > + } > + return 0; > +} > + > static int init_bars(struct pci_dev *pdev) > { > uint16_t cmd; > @@ -470,14 +519,8 @@ static int init_bars(struct pci_dev *pdev) > return -EOPNOTSUPP; > } > > - /* Setup a handler for the command register. */ > - rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, > PCI_COMMAND, > - 2, header); > - if ( rc ) > - return rc; I don't think you need to move the handler for the command register inside add_bar_handlers: for once it makes the function name not reflect what it actually does (as it then deals with both BARs and the command register), and it would also prevent you from having to call add_bar_handlers in if ignore_bars is set. Thanks, Roger.
