| Message ID | 20211125110251.2877218-10-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Add basic emulation support for guests. At the moment only emulate > PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left > as TODO. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v3: > - gate more code on CONFIG_HAS_MSI > - removed logic for the case when MSI/MSI-X not enabled > --- > xen/drivers/vpci/header.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index b0499d32c5d8..2e44055946b0 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, > pci_conf_write16(pdev->sbdf, reg, cmd); > } > > +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, > + uint32_t cmd, void *data) > +{ > + /* TODO: Add proper emulation for all bits of the command register. */ > + > +#ifdef CONFIG_HAS_PCI_MSI > + if ( pdev->vpci->msi->enabled ) You need to check for MSI-X also, pdev->vpci->msix->enabled. > + { > + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ > + cmd |= PCI_COMMAND_INTX_DISABLE; You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the command register when attempting to enable MSI or MSIX capabilities. Thanks, Roger.
Hi, Roger! On 13.01.22 12:50, Roger Pau Monné wrote: > On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Add basic emulation support for guests. At the moment only emulate >> PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left >> as TODO. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> Since v3: >> - gate more code on CONFIG_HAS_MSI >> - removed logic for the case when MSI/MSI-X not enabled >> --- >> xen/drivers/vpci/header.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index b0499d32c5d8..2e44055946b0 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >> pci_conf_write16(pdev->sbdf, reg, cmd); >> } >> >> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >> + uint32_t cmd, void *data) >> +{ >> + /* TODO: Add proper emulation for all bits of the command register. */ >> + >> +#ifdef CONFIG_HAS_PCI_MSI >> + if ( pdev->vpci->msi->enabled ) > You need to check for MSI-X also, pdev->vpci->msix->enabled. Indeed, thank you > >> + { >> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >> + cmd |= PCI_COMMAND_INTX_DISABLE; > You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the > command register when attempting to enable MSI or MSIX capabilities. Isn't it enough that we just check above if MSI/MSI-X enabled then make sure INTX disabled? I am not following you here on what else needs to be done. > > Thanks, Roger. Thank you, Oleksandr
On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: > On 13.01.22 12:50, Roger Pau Monné wrote: >> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>> --- a/xen/drivers/vpci/header.c >>> +++ b/xen/drivers/vpci/header.c >>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>> pci_conf_write16(pdev->sbdf, reg, cmd); >>> } >>> >>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>> + uint32_t cmd, void *data) >>> +{ >>> + /* TODO: Add proper emulation for all bits of the command register. */ >>> + >>> +#ifdef CONFIG_HAS_PCI_MSI >>> + if ( pdev->vpci->msi->enabled ) >> You need to check for MSI-X also, pdev->vpci->msix->enabled. > Indeed, thank you >> >>> + { >>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>> + cmd |= PCI_COMMAND_INTX_DISABLE; >> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >> command register when attempting to enable MSI or MSIX capabilities. > Isn't it enough that we just check above if MSI/MSI-X enabled then make > sure INTX disabled? I am not following you here on what else needs to > be done. No, you need to deal with the potentially bad combination on both paths - command register writes (here) and MSI/MSI-X control register writes (which is what Roger points you at). I would like to suggest to consider simply forcing INTX_DISABLE on behind the guest's back for those other two paths. Jan
On 02.02.22 15:32, Jan Beulich wrote: > On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >> On 13.01.22 12:50, Roger Pau Monné wrote: >>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>> --- a/xen/drivers/vpci/header.c >>>> +++ b/xen/drivers/vpci/header.c >>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>> } >>>> >>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>> + uint32_t cmd, void *data) >>>> +{ >>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>> + >>>> +#ifdef CONFIG_HAS_PCI_MSI >>>> + if ( pdev->vpci->msi->enabled ) >>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >> Indeed, thank you >>>> + { >>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>> command register when attempting to enable MSI or MSIX capabilities. >> Isn't it enough that we just check above if MSI/MSI-X enabled then make >> sure INTX disabled? I am not following you here on what else needs to >> be done. > No, you need to deal with the potentially bad combination on both > paths - command register writes (here) and MSI/MSI-X control register > writes (which is what Roger points you at). I would like to suggest > to consider simply forcing INTX_DISABLE on behind the guest's back > for those other two paths. Do you suggest that we need to have some code which will write PCI_COMMAND while we write MSI/MSI-X control register for that kind of consistency? E.g. control register handler will need to write to PCI_COMMAND and go through emulation for guests? If so, why didn't we have that before? If it was ok before, then I guess the code I add does ensure INTX is set if pdev->vpci->msi->enabled || pdev->vpci->msix->enabled which is enough at least for PCI_COMMAND writes. Sorry if I still didn't get to the point how to do that > > Jan > Thank you, Oleksandr
On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>> --- a/xen/drivers/vpci/header.c >>>>> +++ b/xen/drivers/vpci/header.c >>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>> } >>>>> >>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>> + uint32_t cmd, void *data) >>>>> +{ >>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>> + >>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>> + if ( pdev->vpci->msi->enabled ) >>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>> Indeed, thank you >>>>> + { >>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>> command register when attempting to enable MSI or MSIX capabilities. >>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>> sure INTX disabled? I am not following you here on what else needs to >>> be done. >> No, you need to deal with the potentially bad combination on both >> paths - command register writes (here) and MSI/MSI-X control register >> writes (which is what Roger points you at). I would like to suggest >> to consider simply forcing INTX_DISABLE on behind the guest's back >> for those other two paths. > Do you suggest that we need to have some code which will > write PCI_COMMAND while we write MSI/MSI-X control register > for that kind of consistency? E.g. control register handler will > need to write to PCI_COMMAND and go through emulation for > guests? Either check or write, yes. Since you're setting the bit here behind the guest's back, setting it on the other paths as well would only look consistent to me. > If so, why didn't we have that before? Because we assume Dom0 to be behaving itself. Jan
On 02.02.22 16:18, Jan Beulich wrote: > On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>>> --- a/xen/drivers/vpci/header.c >>>>>> +++ b/xen/drivers/vpci/header.c >>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>>> } >>>>>> >>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>> + uint32_t cmd, void *data) >>>>>> +{ >>>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>>> + >>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>> + if ( pdev->vpci->msi->enabled ) >>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>>> Indeed, thank you >>>>>> + { >>>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>>> command register when attempting to enable MSI or MSIX capabilities. >>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>>> sure INTX disabled? I am not following you here on what else needs to >>>> be done. >>> No, you need to deal with the potentially bad combination on both >>> paths - command register writes (here) and MSI/MSI-X control register >>> writes (which is what Roger points you at). I would like to suggest >>> to consider simply forcing INTX_DISABLE on behind the guest's back >>> for those other two paths. >> Do you suggest that we need to have some code which will >> write PCI_COMMAND while we write MSI/MSI-X control register >> for that kind of consistency? E.g. control register handler will >> need to write to PCI_COMMAND and go through emulation for >> guests? > Either check or write, yes. Since you're setting the bit here behind > the guest's back, setting it on the other paths as well would only > look consistent to me. I can't find any access to PCI_COMMAND register from vMSI/vMSI-X code, so what's the concern? This seems to be the only place in vPCI which touches PCI_COMMAND register. > >> If so, why didn't we have that before? > Because we assume Dom0 to be behaving itself. ok... > > Jan > Thank you, Oleksandr
On 02.02.2022 15:26, Oleksandr Andrushchenko wrote: > > > On 02.02.22 16:18, Jan Beulich wrote: >> On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >>>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>>>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>>>> } >>>>>>> >>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>> + uint32_t cmd, void *data) >>>>>>> +{ >>>>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>>>> + >>>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>>>> Indeed, thank you >>>>>>> + { >>>>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>>>> command register when attempting to enable MSI or MSIX capabilities. >>>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>>>> sure INTX disabled? I am not following you here on what else needs to >>>>> be done. >>>> No, you need to deal with the potentially bad combination on both >>>> paths - command register writes (here) and MSI/MSI-X control register >>>> writes (which is what Roger points you at). I would like to suggest >>>> to consider simply forcing INTX_DISABLE on behind the guest's back >>>> for those other two paths. >>> Do you suggest that we need to have some code which will >>> write PCI_COMMAND while we write MSI/MSI-X control register >>> for that kind of consistency? E.g. control register handler will >>> need to write to PCI_COMMAND and go through emulation for >>> guests? >> Either check or write, yes. Since you're setting the bit here behind >> the guest's back, setting it on the other paths as well would only >> look consistent to me. > I can't find any access to PCI_COMMAND register from vMSI/vMSI-X > code, so what's the concern? Again: Only one of INTX, MSI, or MSI-X may be enabled at a time. This needs to be checked whenever any one of the three is about to change state. Since failing config space writes isn't really an option (there's no error code to hand back and raising an exception is nothing real hardware would do), adjusting state to be sane behind the back of the guest looks to be the least bad option. > This seems to be the only place in vPCI which touches PCI_COMMAND register. How is this relevant? Jan
On 02.02.22 16:31, Jan Beulich wrote: > On 02.02.2022 15:26, Oleksandr Andrushchenko wrote: >> >> On 02.02.22 16:18, Jan Beulich wrote: >>> On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >>>>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>>>>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>>>>> } >>>>>>>> >>>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>> + uint32_t cmd, void *data) >>>>>>>> +{ >>>>>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>>>>> + >>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>>>>> Indeed, thank you >>>>>>>> + { >>>>>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>>>>> command register when attempting to enable MSI or MSIX capabilities. >>>>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>>>>> sure INTX disabled? I am not following you here on what else needs to >>>>>> be done. >>>>> No, you need to deal with the potentially bad combination on both >>>>> paths - command register writes (here) and MSI/MSI-X control register >>>>> writes (which is what Roger points you at). I would like to suggest >>>>> to consider simply forcing INTX_DISABLE on behind the guest's back >>>>> for those other two paths. >>>> Do you suggest that we need to have some code which will >>>> write PCI_COMMAND while we write MSI/MSI-X control register >>>> for that kind of consistency? E.g. control register handler will >>>> need to write to PCI_COMMAND and go through emulation for >>>> guests? >>> Either check or write, yes. Since you're setting the bit here behind >>> the guest's back, setting it on the other paths as well would only >>> look consistent to me. >> I can't find any access to PCI_COMMAND register from vMSI/vMSI-X >> code, so what's the concern? > Again: Only one of INTX, MSI, or MSI-X may be enabled at a time. This is clear and I don't question that > This needs to be checked whenever any one of the three is about > to change state. Since failing config space writes isn't really > an option (there's no error code to hand back and raising an > exception is nothing real hardware would do), adjusting state to > be sane behind the back of the guest looks to be the least bad > option. Would it be enough if I read PCI_MSIX_FLAGS_ENABLE and PCI_MSI_FLAGS_ENABLE in guest_cmd_write to make a decision on INTX? On the other hand msi->enabled and msix->enabled already have this information if I understand the MSI/MSI-X code correctly. Or do we want some additional code in MSI/MSI-X's control_write functions to set INTX bit there as well? I mean that in this guest_cmd_write handler we can only see if we write a consistent wrt MSI/MSI-X PCI_COMMAND value If we want some more checks when we alter PCI_MSIX_FLAGS_ENABLE and/or PCI_MSI_FLAGS_ENABLE bits, this means we need a relevant PCI_COMMAND write there to be added (which doesn't exist now) to make sure INTX bit is set. Please help me understand how you gentlemen want it > >> This seems to be the only place in vPCI which touches PCI_COMMAND register. > How is this relevant? > > Jan > Thank you, Oleksandr
On 02.02.2022 16:04, Oleksandr Andrushchenko wrote: > > > On 02.02.22 16:31, Jan Beulich wrote: >> On 02.02.2022 15:26, Oleksandr Andrushchenko wrote: >>> >>> On 02.02.22 16:18, Jan Beulich wrote: >>>> On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >>>>>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>>>>>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>>>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>>> + uint32_t cmd, void *data) >>>>>>>>> +{ >>>>>>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>>>>>> + >>>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>>>>>> Indeed, thank you >>>>>>>>> + { >>>>>>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>>>>>> command register when attempting to enable MSI or MSIX capabilities. >>>>>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>>>>>> sure INTX disabled? I am not following you here on what else needs to >>>>>>> be done. >>>>>> No, you need to deal with the potentially bad combination on both >>>>>> paths - command register writes (here) and MSI/MSI-X control register >>>>>> writes (which is what Roger points you at). I would like to suggest >>>>>> to consider simply forcing INTX_DISABLE on behind the guest's back >>>>>> for those other two paths. >>>>> Do you suggest that we need to have some code which will >>>>> write PCI_COMMAND while we write MSI/MSI-X control register >>>>> for that kind of consistency? E.g. control register handler will >>>>> need to write to PCI_COMMAND and go through emulation for >>>>> guests? >>>> Either check or write, yes. Since you're setting the bit here behind >>>> the guest's back, setting it on the other paths as well would only >>>> look consistent to me. >>> I can't find any access to PCI_COMMAND register from vMSI/vMSI-X >>> code, so what's the concern? >> Again: Only one of INTX, MSI, or MSI-X may be enabled at a time. > This is clear and I don't question that >> This needs to be checked whenever any one of the three is about >> to change state. Since failing config space writes isn't really >> an option (there's no error code to hand back and raising an >> exception is nothing real hardware would do), adjusting state to >> be sane behind the back of the guest looks to be the least bad >> option. > Would it be enough if I read PCI_MSIX_FLAGS_ENABLE and > PCI_MSI_FLAGS_ENABLE in guest_cmd_write to make a > decision on INTX? > > On the other hand msi->enabled and msix->enabled > already have this information if I understand the > MSI/MSI-X code correctly. > > Or do we want some additional code in MSI/MSI-X's control_write > functions to set INTX bit there as well? Well, yes, this is what Roger and I have been asking you to add. > I mean that in this guest_cmd_write handler we can only see > if we write a consistent wrt MSI/MSI-X PCI_COMMAND value > > If we want some more checks when we alter PCI_MSIX_FLAGS_ENABLE > and/or PCI_MSI_FLAGS_ENABLE bits, this means we need a relevant > PCI_COMMAND write there to be added (which doesn't exist now) > to make sure INTX bit is set. Exactly. Jan
On 02.02.22 17:08, Jan Beulich wrote: > On 02.02.2022 16:04, Oleksandr Andrushchenko wrote: >> >> On 02.02.22 16:31, Jan Beulich wrote: >>> On 02.02.2022 15:26, Oleksandr Andrushchenko wrote: >>>> On 02.02.22 16:18, Jan Beulich wrote: >>>>> On 02.02.2022 14:47, Oleksandr Andrushchenko wrote: >>>>>>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote: >>>>>>>> On 13.01.22 12:50, Roger Pau Monné wrote: >>>>>>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote: >>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>>>> pci_conf_write16(pdev->sbdf, reg, cmd); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, >>>>>>>>>> + uint32_t cmd, void *data) >>>>>>>>>> +{ >>>>>>>>>> + /* TODO: Add proper emulation for all bits of the command register. */ >>>>>>>>>> + >>>>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI >>>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled. >>>>>>>> Indeed, thank you >>>>>>>>>> + { >>>>>>>>>> + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the >>>>>>>>> command register when attempting to enable MSI or MSIX capabilities. >>>>>>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make >>>>>>>> sure INTX disabled? I am not following you here on what else needs to >>>>>>>> be done. >>>>>>> No, you need to deal with the potentially bad combination on both >>>>>>> paths - command register writes (here) and MSI/MSI-X control register >>>>>>> writes (which is what Roger points you at). I would like to suggest >>>>>>> to consider simply forcing INTX_DISABLE on behind the guest's back >>>>>>> for those other two paths. >>>>>> Do you suggest that we need to have some code which will >>>>>> write PCI_COMMAND while we write MSI/MSI-X control register >>>>>> for that kind of consistency? E.g. control register handler will >>>>>> need to write to PCI_COMMAND and go through emulation for >>>>>> guests? >>>>> Either check or write, yes. Since you're setting the bit here behind >>>>> the guest's back, setting it on the other paths as well would only >>>>> look consistent to me. >>>> I can't find any access to PCI_COMMAND register from vMSI/vMSI-X >>>> code, so what's the concern? >>> Again: Only one of INTX, MSI, or MSI-X may be enabled at a time. >> This is clear and I don't question that >>> This needs to be checked whenever any one of the three is about >>> to change state. Since failing config space writes isn't really >>> an option (there's no error code to hand back and raising an >>> exception is nothing real hardware would do), adjusting state to >>> be sane behind the back of the guest looks to be the least bad >>> option. >> Would it be enough if I read PCI_MSIX_FLAGS_ENABLE and >> PCI_MSI_FLAGS_ENABLE in guest_cmd_write to make a >> decision on INTX? >> >> On the other hand msi->enabled and msix->enabled >> already have this information if I understand the >> MSI/MSI-X code correctly. >> >> Or do we want some additional code in MSI/MSI-X's control_write >> functions to set INTX bit there as well? > Well, yes, this is what Roger and I have been asking you to add. Do we only want this for !is_hardware_domain(d) or unconditionally? > >> I mean that in this guest_cmd_write handler we can only see >> if we write a consistent wrt MSI/MSI-X PCI_COMMAND value >> >> If we want some more checks when we alter PCI_MSIX_FLAGS_ENABLE >> and/or PCI_MSI_FLAGS_ENABLE bits, this means we need a relevant >> PCI_COMMAND write there to be added (which doesn't exist now) >> to make sure INTX bit is set. > Exactly. Ok > > Jan > Thank you, Oleksandr
On 02.02.2022 16:12, Oleksandr Andrushchenko wrote: > On 02.02.22 17:08, Jan Beulich wrote: >> On 02.02.2022 16:04, Oleksandr Andrushchenko wrote: >>> Or do we want some additional code in MSI/MSI-X's control_write >>> functions to set INTX bit there as well? >> Well, yes, this is what Roger and I have been asking you to add. > Do we only want this for !is_hardware_domain(d) or unconditionally? To keep present behavior unaltered, I'd suggest to do it only conditionally. Jan
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index b0499d32c5d8..2e44055946b0 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg, pci_conf_write16(pdev->sbdf, reg, cmd); } +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg, + uint32_t cmd, void *data) +{ + /* TODO: Add proper emulation for all bits of the command register. */ + +#ifdef CONFIG_HAS_PCI_MSI + if ( pdev->vpci->msi->enabled ) + { + /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ + cmd |= PCI_COMMAND_INTX_DISABLE; + } +#endif + + cmd_write(pdev, reg, cmd, data); +} + static void bar_write(const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data) { @@ -663,8 +679,9 @@ static int init_bars(struct pci_dev *pdev) } /* Setup a handler for the command register. */ - rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND, - 2, header); + rc = vpci_add_register(pdev->vpci, vpci_hw_read16, + is_hwdom ? cmd_write : guest_cmd_write, + PCI_COMMAND, 2, header); if ( rc ) return rc;