| Message ID | 20210930075223.860329-4-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. > 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 <oleksandr_andrushchenko@epam.com> > > --- > 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.
On 13.10.2021 15:51, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:15AM +0300, Oleksandr Andrushchenko wrote: >> --- 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. I think it was me who asked for const to be added in places like this one. vpci data hanging off of struct pci_dev is an implementation artifact imo, not an unavoidable connection. In principle the vpci data corresponding to a physical device could also be looked up using e.g. SBDF. Here the intention really is to leave the physical device unchanged; that's what the const documents (apart from enforcing). Jan
On Fri, Oct 15, 2021 at 08:04:56AM +0200, Jan Beulich wrote: > On 13.10.2021 15:51, Roger Pau Monné wrote: > > On Thu, Sep 30, 2021 at 10:52:15AM +0300, Oleksandr Andrushchenko wrote: > >> --- 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. > > I think it was me who asked for const to be added in places like this > one. vpci data hanging off of struct pci_dev is an implementation > artifact imo, not an unavoidable connection. In principle the vpci > data corresponding to a physical device could also be looked up using > e.g. SBDF. I was considering vPCI part an intrinsic part of the pci_dev, but I can see you thinking otherwise. We similarly have other pieces of data hanging off pci_dev, so I think it's hard to tell which ones as fine to have as part of the struct vs as pointer references. > Here the intention really is to leave the physical device unchanged; > that's what the const documents (apart from enforcing). Ack. I wouldn't have asked for those myself, but as said above I can see your point. Regards, Roger.
Hi, Roger! On 13.10.21 16:51, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. >> 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? It is possible > > 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. Ok, I won't introduce add_bar_handlers, thus rendering this patch useless. I'll drop it and re-work the upcoming patches with this respect Thank you, Oleksandr
Hi, Roger! On 27.10.21 13:17, Oleksandr Andrushchenko wrote: > Hi, Roger! > > On 13.10.21 16:51, Roger Pau Monné wrote: >> On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. >>> 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? > It is possible >> 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. > Ok, I won't introduce add_bar_handlers, thus rendering this patch useless. > I'll drop it and re-work the upcoming patches with this respect On the other hand after thinking a bit more. What actually init_bars do? 1. Runs once per each pdev (__init?) 2. Sizes the BARs and detects their type, sets up pdev->vpci->header BAR values 3. Adds register handlers. For DomU we only need 3), so we can setup guest handlers. So, from this POV either we need to have a yet another add_bar_handlers or similar for at least the guests and the case when pdev is assigned back to hwdom. So this can be a reason to defend the current approach with add_bar_handlers. Or? Do you have an idea how to do that some other way? > > Thank you, > Oleksandr
On Wed, Oct 27, 2021 at 11:59:47AM +0000, Oleksandr Andrushchenko wrote: > Hi, Roger! > > On 27.10.21 13:17, Oleksandr Andrushchenko wrote: > > Hi, Roger! > > > > On 13.10.21 16:51, Roger Pau Monné wrote: > >> On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. > >>> 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? > > It is possible > >> 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. > > Ok, I won't introduce add_bar_handlers, thus rendering this patch useless. > > I'll drop it and re-work the upcoming patches with this respect > On the other hand after thinking a bit more. > What actually init_bars do? > 1. Runs once per each pdev (__init?) > 2. Sizes the BARs and detects their type, sets up pdev->vpci->header BAR values > 3. Adds register handlers. > > For DomU we only need 3), so we can setup guest handlers. I think you assume that there will always be a hardware domain with vPCI enabled that will get the device assigned and thus init_bars will be executed prior to assigning to a domU. But what about dom0less, or when using a classic PV dom0? In that case the device won't get assigned to a hardware domain with vPCI support, so the vpci structure won't be allocated or filled, and hence init_bars would have to be executed when assigning to a domU. Thanks, Roger.
On 27.10.21 16:23, Roger Pau Monné wrote: > On Wed, Oct 27, 2021 at 11:59:47AM +0000, Oleksandr Andrushchenko wrote: >> Hi, Roger! >> >> On 27.10.21 13:17, Oleksandr Andrushchenko wrote: >>> Hi, Roger! >>> >>> On 13.10.21 16:51, Roger Pau Monné wrote: >>>> On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. >>>>> 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? >>> It is possible >>>> 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. >>> Ok, I won't introduce add_bar_handlers, thus rendering this patch useless. >>> I'll drop it and re-work the upcoming patches with this respect >> On the other hand after thinking a bit more. >> What actually init_bars do? >> 1. Runs once per each pdev (__init?) >> 2. Sizes the BARs and detects their type, sets up pdev->vpci->header BAR values >> 3. Adds register handlers. >> >> For DomU we only need 3), so we can setup guest handlers. > I think you assume that there will always be a hardware domain with > vPCI enabled that will get the device assigned and thus init_bars will > be executed prior to assigning to a domU. Yes, this is the current assumption... > > But what about dom0less, it was decided to put dom0less out of scope for now > or when using a classic PV dom0? I thought that vPCI is only used for PVH Dom0 and it is enough for now (yes, this is a weak argument, but we do not want PCI passthrough on Arm to become a never ending game... since 2015...) > In that case > the device won't get assigned to a hardware domain with vPCI support, > so the vpci structure won't be allocated or filled, Yes, this is true. But because of the 3 functionflities of the init_bars is doing it might still need some dis-aggregation, e.g. BAR sizing is not needed and might not be possible while assigning to a DomU. So, I think that init_bars will need to be split in any case. > and hence > init_bars would have to be executed when assigning to a domU. Please see above: not sure init_bars can exist in its form to achieve that. One of the steps this patch is doing is we split init_bars into a) register assignment b) all the reset: initial pdev's header initialization, sizing etc. The same is true for MSI/MSI-X. When we add support for MSI/MSI-X on Arm you will see the same: we need to split [1] (this is WIP). So, I am still convinced that we need add_bar_handlers in some form. > Thanks, Roger. > [1] https://gitlab.com/rahsingh/xen-integration/-/commit/7b898601261fc3ad834ac3d06cc4c784f33c95bb
On Wed, Oct 27, 2021 at 02:06:40PM +0000, Oleksandr Andrushchenko wrote: > > > On 27.10.21 16:23, Roger Pau Monné wrote: > > On Wed, Oct 27, 2021 at 11:59:47AM +0000, Oleksandr Andrushchenko wrote: > >> Hi, Roger! > >> > >> On 27.10.21 13:17, Oleksandr Andrushchenko wrote: > >>> Hi, Roger! > >>> > >>> On 13.10.21 16:51, Roger Pau Monné wrote: > >>>> On Thu, Sep 30, 2021 at 10:52:15AM +0300, 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. > >>>>> 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? > >>> It is possible > >>>> 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. > >>> Ok, I won't introduce add_bar_handlers, thus rendering this patch useless. > >>> I'll drop it and re-work the upcoming patches with this respect > >> On the other hand after thinking a bit more. > >> What actually init_bars do? > >> 1. Runs once per each pdev (__init?) > >> 2. Sizes the BARs and detects their type, sets up pdev->vpci->header BAR values > >> 3. Adds register handlers. > >> > >> For DomU we only need 3), so we can setup guest handlers. > > I think you assume that there will always be a hardware domain with > > vPCI enabled that will get the device assigned and thus init_bars will > > be executed prior to assigning to a domU. > Yes, this is the current assumption... > > > > But what about dom0less, > it was decided to put dom0less out of scope for now > > or when using a classic PV dom0? > I thought that vPCI is only used for PVH Dom0 and it is enough for now > (yes, this is a weak argument, but we do not want PCI passthrough on Arm > to become a never ending game... since 2015...) I understand that not everything will be supported, that's perfectly fine, but we should aim to not make supporting those use cases harder in the future. > > In that case > > the device won't get assigned to a hardware domain with vPCI support, > > so the vpci structure won't be allocated or filled, > Yes, this is true. But because of the 3 functionflities of the init_bars is > doing it might still need some dis-aggregation, e.g. BAR sizing > is not needed and might not be possible while assigning to a DomU. > So, I think that init_bars will need to be split in any case. I understand that BAR sizing will not be needed if the structure is pre-initialized, but I also cannot see why it would be impossible, at least on x86. > > and hence > > init_bars would have to be executed when assigning to a domU. > Please see above: not sure init_bars can exist in its form to achieve that. > One of the steps this patch is doing is we split init_bars into > a) register assignment > b) all the reset: initial pdev's header initialization, sizing etc. > > The same is true for MSI/MSI-X. When we add support for MSI/MSI-X on Arm > you will see the same: we need to split [1] (this is WIP). > > So, I am still convinced that we need add_bar_handlers in some form. I'm fine to split it if there's a hard requirement, but I'm afraid so far I'm not convinced it's required. Maybe if you could elaborate on why BAR sizing might not be possible when assigning to domU I could be convinced. Another option might be to just modify init_bars to have slightly different paths for dom0 vs domU. Thanks, Roger.
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) +{ + 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;