| Message ID | 20210903100831.177748-4-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 03.09.2021 12:08, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > This is in preparation for dynamic assignment of the vpci register > handlers depending on the domain: hwdom or guest. I guess why exactly this is going to help is going to be seen in subsequent patches. To aid review (i.e. to not force reviewers to peek ahead) it would imo be helpful if you outlined how the result is going to help. After all ... > --- 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(struct pci_dev *pdev) ... this function name, for example, isn't Dom0-specific, so one might expect the function body to gain conditionals. Yet then the question is why these conditionals can't live in the original function. > +{ > + 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; > + > + for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ ) > + { > + 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; > + 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; I'm not the maintainer of this code, but if I was I'd ask for this and ... > + } > + else > + { > + uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4; > + > + /* 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; ... this to be moved ... > + } ... here to reduce redundancy. > @@ -553,11 +580,13 @@ static int init_bars(struct pci_dev *pdev) > rom->addr = addr; > header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) & > PCI_ROM_ADDRESS_ENABLE; > + } > > - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, rom_reg, > - 4, rom); > - if ( rc ) > - rom->type = VPCI_BAR_EMPTY; > + rc = add_bar_handlers(pdev); > + if ( rc ) > + { > + pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); > + return rc; > } Seeing this moved (hence perhaps more a question to Roger than to you) restoring of the command register - why is it that the error path(s) here care(s) about restoring this, but ... > return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0; ... ones in modify_bars() (and downwards) don't? I was wondering whether the restore could actually be done prior to the two calls (or, in the original code, the one call), or perhaps even right after the last call to pci_size_mem_bar(). At the very least the comment further up suggests memory decode only gets disabled for sizing BARs, which we're done with at this point. Jan
On 06.09.21 16:53, Jan Beulich wrote: > On 03.09.2021 12:08, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> This is in preparation for dynamic assignment of the vpci register >> handlers depending on the domain: hwdom or guest. > I guess why exactly this is going to help is going to be seen in > subsequent patches. To aid review (i.e. to not force reviewers to > peek ahead) it would imo be helpful if you outlined how the result > is going to help. Sure, will do next time. The need for this step is that is it easier to have all related functionality (BARs here) put at one place and when the subsequent patches add decisions on which handlers to install, e.g. hwdom or guest handlers, this function is extended to accept a one more parameter, is_hwdom, and all the assignment logic is put here. Of course I could have all the "if (is_hwdom)"'s put at the original location, but dedicated function looked cleaner to me. > After all ... > >> --- 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(struct pci_dev *pdev) > ... this function name, for example, isn't Dom0-specific, so one > might expect the function body to gain conditionals. Yet then the > question is why these conditionals can't live in the original > function. Answered above. I think it makes code cleaner and easier for modification as handlers' assignment for BARs becomes grouped as it is done for MSI/MSI-X. > >> +{ >> + 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; >> + >> + for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ ) >> + { >> + 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; >> + 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; > I'm not the maintainer of this code, but if I was I'd ask for this and ... > >> + } >> + else >> + { >> + uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4; >> + >> + /* 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; > ... this to be moved ... > >> + } > ... here to reduce redundancy. > >> @@ -553,11 +580,13 @@ static int init_bars(struct pci_dev *pdev) >> rom->addr = addr; >> header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) & >> PCI_ROM_ADDRESS_ENABLE; >> + } >> >> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, rom_reg, >> - 4, rom); >> - if ( rc ) >> - rom->type = VPCI_BAR_EMPTY; >> + rc = add_bar_handlers(pdev); >> + if ( rc ) >> + { >> + pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); >> + return rc; >> } > Seeing this moved (hence perhaps more a question to Roger than to > you) restoring of the command register - why is it that the error > path(s) here care(s) about restoring this, but ... > >> return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0; > ... ones in modify_bars() (and downwards) don't? I was wondering > whether the restore could actually be done prior to the two calls > (or, in the original code, the one call), or perhaps even right > after the last call to pci_size_mem_bar(). At the very least the > comment further up suggests memory decode only gets disabled for > sizing BARs, which we're done with at this point. For all the above: what this patch does is a pure code move. I had no intention to alter it in any other way rather than that. If you think the code needs to be functionally modified I think this deserves a dedicated work to be submitted, but IMO this patch shouldn't touch anything. > > Jan > Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index f8cd55e7c024..31bca7a12942 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(struct pci_dev *pdev) +{ + 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; + + for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ ) + { + 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; + 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; + + /* 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; - if ( pdev->ignore_bars ) - return 0; + return add_bar_handlers(pdev); /* Disable memory decoding before sizing. */ cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND); @@ -492,14 +535,6 @@ static int init_bars(struct pci_dev *pdev) if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO ) { bars[i].type = VPCI_BAR_MEM64_HI; - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, - 4, &bars[i]); - if ( rc ) - { - pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); - return rc; - } - continue; } @@ -532,14 +567,6 @@ static int init_bars(struct pci_dev *pdev) bars[i].addr = addr; bars[i].size = size; bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH; - - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4, - &bars[i]); - if ( rc ) - { - pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); - return rc; - } } /* Check expansion ROM. */ @@ -553,11 +580,13 @@ static int init_bars(struct pci_dev *pdev) rom->addr = addr; header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) & PCI_ROM_ADDRESS_ENABLE; + } - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, rom_reg, - 4, rom); - if ( rc ) - rom->type = VPCI_BAR_EMPTY; + rc = add_bar_handlers(pdev); + if ( rc ) + { + pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); + return rc; } return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;