| Message ID | 20210930075223.860329-9-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:20AM +0300, 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> > Reviewed-by: Michal Orzel <michal.orzel@arm.com> > --- > New in v2 > --- > xen/drivers/vpci/header.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index f23c956cde6c..754aeb5a584f 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -451,6 +451,32 @@ 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. */ > + > + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > + { > + /* > + * Guest wants to enable INTx. It can't be enabled if: > + * - host has INTx disabled > + * - MSI/MSI-X enabled > + */ > + if ( pdev->vpci->msi->enabled ) > + cmd |= PCI_COMMAND_INTX_DISABLE; > + else > + { > + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); > + > + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) > + cmd |= PCI_COMMAND_INTX_DISABLE; > + } This last part should be Arm specific. On other architectures we likely want the guest to modify INTx disable in order to select the interrupt delivery mode for the device. I really wonder if we should allow the guest to play with any other bit apart from INTx disable and memory and IO decoding on the command register. Thanks, Roger.
Hi, Roger! On 26.10.21 13:52, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:20AM +0300, 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> >> Reviewed-by: Michal Orzel <michal.orzel@arm.com> >> --- >> New in v2 >> --- >> xen/drivers/vpci/header.c | 35 ++++++++++++++++++++++++++++++++--- >> 1 file changed, 32 insertions(+), 3 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index f23c956cde6c..754aeb5a584f 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -451,6 +451,32 @@ 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. */ >> + >> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >> + { >> + /* >> + * Guest wants to enable INTx. It can't be enabled if: >> + * - host has INTx disabled >> + * - MSI/MSI-X enabled >> + */ >> + if ( pdev->vpci->msi->enabled ) >> + cmd |= PCI_COMMAND_INTX_DISABLE; >> + else >> + { >> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >> + >> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >> + cmd |= PCI_COMMAND_INTX_DISABLE; >> + } > This last part should be Arm specific. On other architectures we > likely want the guest to modify INTx disable in order to select the > interrupt delivery mode for the device. This is not arch specific as we just do not allow INTx to be enabled if MSI/MSI-X has been enabled before. This was discussed previously (Jan) and this was pointed as an acceptable approach to limit the guest from having inconsistent configuration > > I really wonder if we should allow the guest to play with any other > bit apart from INTx disable and memory and IO decoding on the command > register. This needs to be implemented one day when we understand what this emulation should look like. This is why I have a "TODO" above. > > Thanks, Roger. Thank you, Oleksandr
On 26.10.2021 12:52, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -451,6 +451,32 @@ 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. */ >> + >> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >> + { >> + /* >> + * Guest wants to enable INTx. It can't be enabled if: >> + * - host has INTx disabled >> + * - MSI/MSI-X enabled >> + */ >> + if ( pdev->vpci->msi->enabled ) >> + cmd |= PCI_COMMAND_INTX_DISABLE; >> + else >> + { >> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >> + >> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >> + cmd |= PCI_COMMAND_INTX_DISABLE; >> + } > > This last part should be Arm specific. On other architectures we > likely want the guest to modify INTx disable in order to select the > interrupt delivery mode for the device. We cannot allow a guest to clear the bit when it has MSI / MSI-X enabled - only one of the three is supposed to be active at a time. (IOW similarly we cannot allow a guest to enable MSI / MSI-X when the bit is clear.) Jan
On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: > On 26.10.2021 12:52, Roger Pau Monné wrote: > > On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: > >> --- a/xen/drivers/vpci/header.c > >> +++ b/xen/drivers/vpci/header.c > >> @@ -451,6 +451,32 @@ 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. */ > >> + > >> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > >> + { > >> + /* > >> + * Guest wants to enable INTx. It can't be enabled if: > >> + * - host has INTx disabled > >> + * - MSI/MSI-X enabled > >> + */ > >> + if ( pdev->vpci->msi->enabled ) > >> + cmd |= PCI_COMMAND_INTX_DISABLE; > >> + else > >> + { > >> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); > >> + > >> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) > >> + cmd |= PCI_COMMAND_INTX_DISABLE; > >> + } > > > > This last part should be Arm specific. On other architectures we > > likely want the guest to modify INTx disable in order to select the > > interrupt delivery mode for the device. > > We cannot allow a guest to clear the bit when it has MSI / MSI-X > enabled - only one of the three is supposed to be active at a time. > (IOW similarly we cannot allow a guest to enable MSI / MSI-X when > the bit is clear.) Sure, but this code is making the bit sticky, by not allowing INTX_DISABLE to be cleared once set. We do not want that behavior on x86, as a guest can decide to use MSI or INTx. The else branch needs to be Arm only. Regards, Roger.
On 02.11.2021 12:50, Roger Pau Monné wrote: > On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >> On 26.10.2021 12:52, Roger Pau Monné wrote: >>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>> --- a/xen/drivers/vpci/header.c >>>> +++ b/xen/drivers/vpci/header.c >>>> @@ -451,6 +451,32 @@ 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. */ >>>> + >>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>> + { >>>> + /* >>>> + * Guest wants to enable INTx. It can't be enabled if: >>>> + * - host has INTx disabled >>>> + * - MSI/MSI-X enabled >>>> + */ >>>> + if ( pdev->vpci->msi->enabled ) >>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>> + else >>>> + { >>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>> + >>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>> + } >>> >>> This last part should be Arm specific. On other architectures we >>> likely want the guest to modify INTx disable in order to select the >>> interrupt delivery mode for the device. >> >> We cannot allow a guest to clear the bit when it has MSI / MSI-X >> enabled - only one of the three is supposed to be active at a time. >> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >> the bit is clear.) > > Sure, but this code is making the bit sticky, by not allowing > INTX_DISABLE to be cleared once set. We do not want that behavior on > x86, as a guest can decide to use MSI or INTx. The else branch needs > to be Arm only. Isn't the "else" part questionable even on Arm? Jan
On 02.11.21 15:54, Jan Beulich wrote: > On 02.11.2021 12:50, Roger Pau Monné wrote: >> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>> --- a/xen/drivers/vpci/header.c >>>>> +++ b/xen/drivers/vpci/header.c >>>>> @@ -451,6 +451,32 @@ 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. */ >>>>> + >>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>> + { >>>>> + /* >>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>> + * - host has INTx disabled >>>>> + * - MSI/MSI-X enabled >>>>> + */ >>>>> + if ( pdev->vpci->msi->enabled ) >>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>> + else >>>>> + { >>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>> + >>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>> + } >>>> This last part should be Arm specific. On other architectures we >>>> likely want the guest to modify INTx disable in order to select the >>>> interrupt delivery mode for the device. >>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>> enabled - only one of the three is supposed to be active at a time. >>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>> the bit is clear.) >> Sure, but this code is making the bit sticky, by not allowing >> INTX_DISABLE to be cleared once set. We do not want that behavior on >> x86, as a guest can decide to use MSI or INTx. The else branch needs >> to be Arm only. > Isn't the "else" part questionable even on Arm? It is. Once fixed I can't see anything Arm specific here > > Jan >
Hi Roger, On 02/11/2021 11:50, Roger Pau Monné wrote: > On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >> On 26.10.2021 12:52, Roger Pau Monné wrote: >>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>> --- a/xen/drivers/vpci/header.c >>>> +++ b/xen/drivers/vpci/header.c >>>> @@ -451,6 +451,32 @@ 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. */ >>>> + >>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>> + { >>>> + /* >>>> + * Guest wants to enable INTx. It can't be enabled if: >>>> + * - host has INTx disabled >>>> + * - MSI/MSI-X enabled >>>> + */ >>>> + if ( pdev->vpci->msi->enabled ) >>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>> + else >>>> + { >>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>> + >>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>> + } >>> >>> This last part should be Arm specific. On other architectures we >>> likely want the guest to modify INTx disable in order to select the >>> interrupt delivery mode for the device. >> >> We cannot allow a guest to clear the bit when it has MSI / MSI-X >> enabled - only one of the three is supposed to be active at a time. >> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >> the bit is clear.) > > Sure, but this code is making the bit sticky, by not allowing > INTX_DISABLE to be cleared once set. We do not want that behavior on > x86, as a guest can decide to use MSI or INTx. On Arm, I am aware of some hosbridges (e.g. Thunder-X) where legacy interrupts are not supported. Do such hostbridges exist x86? Cheers,
On 02.11.21 16:10, Oleksandr Andrushchenko wrote: > > On 02.11.21 15:54, Jan Beulich wrote: >> On 02.11.2021 12:50, Roger Pau Monné wrote: >>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>> --- a/xen/drivers/vpci/header.c >>>>>> +++ b/xen/drivers/vpci/header.c >>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>> + >>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>> + { >>>>>> + /* >>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>> + * - host has INTx disabled >>>>>> + * - MSI/MSI-X enabled >>>>>> + */ >>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>> + else >>>>>> + { >>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>> + >>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>> + } >>>>> This last part should be Arm specific. On other architectures we >>>>> likely want the guest to modify INTx disable in order to select the >>>>> interrupt delivery mode for the device. >>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>> enabled - only one of the three is supposed to be active at a time. >>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>> the bit is clear.) >>> Sure, but this code is making the bit sticky, by not allowing >>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>> to be Arm only. >> Isn't the "else" part questionable even on Arm? > It is. Once fixed I can't see anything Arm specific here Well, I have looked at the code one more time and everything seems to be ok wrt that sticky bit: we have 2 handlers which are cmd_write and guest_cmd_write. The former is used for the hardware domain and has *no restrictions* on writing PCI_COMMAND register contents and the later is only used for guests and which does have restrictions applied in emulate_cmd_reg function. So, for the hardware domain, there is no "sticky" bit possible and for the guest domains if the physical contents of the PCI_COMMAND register has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to use PCI_COMMAND_INTX_DISABLE bit set. So, from hardware domain POV, this should not be a problem, but from guests view it can. Let's imagine that the hardware domain can handle all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware domain can decide what can be used for the interrupt source (again, no restriction here) and program PCI_COMMAND accordingly. Guest domains need to align with this configuration, e.g. if INTx was disabled by the hardware domain then INTx cannot be enabled for guests: yes, this doesn't cover dom0less etc. so we do rely on some entity before the guest to set the PCI_COMMAND correctly. This is how it is implemented in the patch. Please also see the discussion we had before [1]. What is not now covered is that if there is a hardware domain and the same PCI device is first passed to one of the guests and then assigned to another. In this case: hwdom (or any other entity) programs PCI_COMMAND assign domU1 deassign domU1 *assign domIO* assign domU2 So in this scenario the host assigned value is lost after assigning to domU1 and domU2 will use the value used by domU1. So, it seems that this is the only use-case not covered by the patch. Jan [1]: "In the absence of Dom0 controlling the device, I think we ought to take Xen's view as the "host" one. Which will want the bit set at least as long as either MSI or MSI-X is enabled for the device." So, for the PCI_COMMAND register we might want to have a reference value to be stored so we can restore it while assigning the PCI device to a guest. For the current implementation the best I can probably do is to read this value in init_bars when it is called for the hardware domain: if ( is_hardware_domain(d) ) vpci->pci_command_reference = pci_read(PCI_COMMAND) And when I want to reset PCI_COMMAND while assigning to a guest I will use it instead of 0 as it is now. >> Jan >> [1] https://lore.kernel.org/xen-devel/20210903100831.177748-9-andr2000@gmail.com/
On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: > > > On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >> >> On 02.11.21 15:54, Jan Beulich wrote: >>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>> + >>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>> + { >>>>>>> + /* >>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>> + * - host has INTx disabled >>>>>>> + * - MSI/MSI-X enabled >>>>>>> + */ >>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>> + else >>>>>>> + { >>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>> + >>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>> + } >>>>>> This last part should be Arm specific. On other architectures we >>>>>> likely want the guest to modify INTx disable in order to select the >>>>>> interrupt delivery mode for the device. >>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>> enabled - only one of the three is supposed to be active at a time. >>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>> the bit is clear.) >>>> Sure, but this code is making the bit sticky, by not allowing >>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>> to be Arm only. >>> Isn't the "else" part questionable even on Arm? >> It is. Once fixed I can't see anything Arm specific here > Well, I have looked at the code one more time and everything seems to > be ok wrt that sticky bit: we have 2 handlers which are cmd_write and > guest_cmd_write. The former is used for the hardware domain and has > *no restrictions* on writing PCI_COMMAND register contents and the later > is only used for guests and which does have restrictions applied in > emulate_cmd_reg function. > > So, for the hardware domain, there is no "sticky" bit possible and for the > guest domains if the physical contents of the PCI_COMMAND register > has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to > use PCI_COMMAND_INTX_DISABLE bit set. > > So, from hardware domain POV, this should not be a problem, but from > guests view it can. Let's imagine that the hardware domain can handle > all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware > domain can decide what can be used for the interrupt source (again, no > restriction here) and program PCI_COMMAND accordingly. > Guest domains need to align with this configuration, e.g. if INTx was disabled > by the hardware domain then INTx cannot be enabled for guests Why? It's the DomU that's in control of the device, so it ought to be able to pick any of the three. I don't think Dom0 is involved in handling of interrupts from the device, and hence its own "dislike" of INTx ought to only extend to the period of time where Dom0 is controlling the device. This would be different if Xen's view was different, but as we seem to agree Xen's role here is solely to prevent invalid combinations getting established in hardware. Jan
On 03.11.21 11:11, Jan Beulich wrote: > On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >> >> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>> On 02.11.21 15:54, Jan Beulich wrote: >>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>> + >>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>> + { >>>>>>>> + /* >>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>> + * - host has INTx disabled >>>>>>>> + * - MSI/MSI-X enabled >>>>>>>> + */ >>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>> + else >>>>>>>> + { >>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>> + >>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>> + } >>>>>>> This last part should be Arm specific. On other architectures we >>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>> interrupt delivery mode for the device. >>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>> the bit is clear.) >>>>> Sure, but this code is making the bit sticky, by not allowing >>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>> to be Arm only. >>>> Isn't the "else" part questionable even on Arm? >>> It is. Once fixed I can't see anything Arm specific here >> Well, I have looked at the code one more time and everything seems to >> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >> guest_cmd_write. The former is used for the hardware domain and has >> *no restrictions* on writing PCI_COMMAND register contents and the later >> is only used for guests and which does have restrictions applied in >> emulate_cmd_reg function. >> >> So, for the hardware domain, there is no "sticky" bit possible and for the >> guest domains if the physical contents of the PCI_COMMAND register >> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >> use PCI_COMMAND_INTX_DISABLE bit set. >> >> So, from hardware domain POV, this should not be a problem, but from >> guests view it can. Let's imagine that the hardware domain can handle >> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >> domain can decide what can be used for the interrupt source (again, no >> restriction here) and program PCI_COMMAND accordingly. >> Guest domains need to align with this configuration, e.g. if INTx was disabled >> by the hardware domain then INTx cannot be enabled for guests > Why? It's the DomU that's in control of the device, so it ought to > be able to pick any of the three. I don't think Dom0 is involved in > handling of interrupts from the device, and hence its own "dislike" > of INTx ought to only extend to the period of time where Dom0 is > controlling the device. This would be different if Xen's view was > different, but as we seem to agree Xen's role here is solely to > prevent invalid combinations getting established in hardware. On top of a PCI device there is a physical host bridge and physical bus topology which may impose restrictions from Dom0 POV on that particular device. So, every PCI device being passed through to a DomU may have different INTx settings which do depend on Dom0 in our case. > > Jan > >
On 03.11.2021 10:18, Oleksandr Andrushchenko wrote: > > > On 03.11.21 11:11, Jan Beulich wrote: >> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >>> >>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>>> On 02.11.21 15:54, Jan Beulich wrote: >>>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>>> + >>>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>>> + { >>>>>>>>> + /* >>>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>>> + * - host has INTx disabled >>>>>>>>> + * - MSI/MSI-X enabled >>>>>>>>> + */ >>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>> + else >>>>>>>>> + { >>>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>>> + >>>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>> + } >>>>>>>> This last part should be Arm specific. On other architectures we >>>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>>> interrupt delivery mode for the device. >>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>>> the bit is clear.) >>>>>> Sure, but this code is making the bit sticky, by not allowing >>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>>> to be Arm only. >>>>> Isn't the "else" part questionable even on Arm? >>>> It is. Once fixed I can't see anything Arm specific here >>> Well, I have looked at the code one more time and everything seems to >>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >>> guest_cmd_write. The former is used for the hardware domain and has >>> *no restrictions* on writing PCI_COMMAND register contents and the later >>> is only used for guests and which does have restrictions applied in >>> emulate_cmd_reg function. >>> >>> So, for the hardware domain, there is no "sticky" bit possible and for the >>> guest domains if the physical contents of the PCI_COMMAND register >>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >>> use PCI_COMMAND_INTX_DISABLE bit set. >>> >>> So, from hardware domain POV, this should not be a problem, but from >>> guests view it can. Let's imagine that the hardware domain can handle >>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >>> domain can decide what can be used for the interrupt source (again, no >>> restriction here) and program PCI_COMMAND accordingly. >>> Guest domains need to align with this configuration, e.g. if INTx was disabled >>> by the hardware domain then INTx cannot be enabled for guests >> Why? It's the DomU that's in control of the device, so it ought to >> be able to pick any of the three. I don't think Dom0 is involved in >> handling of interrupts from the device, and hence its own "dislike" >> of INTx ought to only extend to the period of time where Dom0 is >> controlling the device. This would be different if Xen's view was >> different, but as we seem to agree Xen's role here is solely to >> prevent invalid combinations getting established in hardware. > On top of a PCI device there is a physical host bridge and > physical bus topology which may impose restrictions from > Dom0 POV on that particular device. Well, such physical restrictions may mean INTx doesn't actually work, but this won't mean the DomU isn't free in choosing the bit's setting. The bit merely controls whether the device is allowed to assert its interrupt pin. Hence ... > So, every PCI device > being passed through to a DomU may have different INTx > settings which do depend on Dom0 in our case. ... I'm still unconvinced of this. Jan
On 03.11.21 11:24, Jan Beulich wrote: > On 03.11.2021 10:18, Oleksandr Andrushchenko wrote: >> >> On 03.11.21 11:11, Jan Beulich wrote: >>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>>>> On 02.11.21 15:54, Jan Beulich wrote: >>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>>>> + >>>>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>>>> + { >>>>>>>>>> + /* >>>>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>>>> + * - host has INTx disabled >>>>>>>>>> + * - MSI/MSI-X enabled >>>>>>>>>> + */ >>>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>> + else >>>>>>>>>> + { >>>>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>>>> + >>>>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>> + } >>>>>>>>> This last part should be Arm specific. On other architectures we >>>>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>>>> interrupt delivery mode for the device. >>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>>>> the bit is clear.) >>>>>>> Sure, but this code is making the bit sticky, by not allowing >>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>>>> to be Arm only. >>>>>> Isn't the "else" part questionable even on Arm? >>>>> It is. Once fixed I can't see anything Arm specific here >>>> Well, I have looked at the code one more time and everything seems to >>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >>>> guest_cmd_write. The former is used for the hardware domain and has >>>> *no restrictions* on writing PCI_COMMAND register contents and the later >>>> is only used for guests and which does have restrictions applied in >>>> emulate_cmd_reg function. >>>> >>>> So, for the hardware domain, there is no "sticky" bit possible and for the >>>> guest domains if the physical contents of the PCI_COMMAND register >>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >>>> use PCI_COMMAND_INTX_DISABLE bit set. >>>> >>>> So, from hardware domain POV, this should not be a problem, but from >>>> guests view it can. Let's imagine that the hardware domain can handle >>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >>>> domain can decide what can be used for the interrupt source (again, no >>>> restriction here) and program PCI_COMMAND accordingly. >>>> Guest domains need to align with this configuration, e.g. if INTx was disabled >>>> by the hardware domain then INTx cannot be enabled for guests >>> Why? It's the DomU that's in control of the device, so it ought to >>> be able to pick any of the three. I don't think Dom0 is involved in >>> handling of interrupts from the device, and hence its own "dislike" >>> of INTx ought to only extend to the period of time where Dom0 is >>> controlling the device. This would be different if Xen's view was >>> different, but as we seem to agree Xen's role here is solely to >>> prevent invalid combinations getting established in hardware. >> On top of a PCI device there is a physical host bridge and >> physical bus topology which may impose restrictions from >> Dom0 POV on that particular device. > Well, such physical restrictions may mean INTx doesn't actually work, > but this won't mean the DomU isn't free in choosing the bit's setting. > The bit merely controls whether the device is allowed to assert its > interrupt pin. Hence ... > >> So, every PCI device >> being passed through to a DomU may have different INTx >> settings which do depend on Dom0 in our case. > ... I'm still unconvinced of this. Ok, so I can accept any suggestion how to solve this. It seems that we already have number of no go scenarios here, but still it is not clear to me what could be an acceptable approach here. Namely: what do we do with INTx bit for guests? 1. I can leave it as is in the patch 2. I can remove INTx emulation and let the guest decide and program INTx 3. What else can I do? > > Jan > > Thank you, Oleksandr
On Wed, Nov 03, 2021 at 09:18:03AM +0000, Oleksandr Andrushchenko wrote: > > > On 03.11.21 11:11, Jan Beulich wrote: > > On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: > >> > >> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: > >>> On 02.11.21 15:54, Jan Beulich wrote: > >>>> On 02.11.2021 12:50, Roger Pau Monné wrote: > >>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: > >>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: > >>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: > >>>>>>>> --- a/xen/drivers/vpci/header.c > >>>>>>>> +++ b/xen/drivers/vpci/header.c > >>>>>>>> @@ -451,6 +451,32 @@ 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. */ > >>>>>>>> + > >>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > >>>>>>>> + { > >>>>>>>> + /* > >>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: > >>>>>>>> + * - host has INTx disabled > >>>>>>>> + * - MSI/MSI-X enabled > >>>>>>>> + */ > >>>>>>>> + if ( pdev->vpci->msi->enabled ) > >>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; > >>>>>>>> + else > >>>>>>>> + { > >>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); > >>>>>>>> + > >>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) > >>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; > >>>>>>>> + } > >>>>>>> This last part should be Arm specific. On other architectures we > >>>>>>> likely want the guest to modify INTx disable in order to select the > >>>>>>> interrupt delivery mode for the device. > >>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X > >>>>>> enabled - only one of the three is supposed to be active at a time. > >>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when > >>>>>> the bit is clear.) > >>>>> Sure, but this code is making the bit sticky, by not allowing > >>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on > >>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs > >>>>> to be Arm only. > >>>> Isn't the "else" part questionable even on Arm? > >>> It is. Once fixed I can't see anything Arm specific here > >> Well, I have looked at the code one more time and everything seems to > >> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and > >> guest_cmd_write. The former is used for the hardware domain and has > >> *no restrictions* on writing PCI_COMMAND register contents and the later > >> is only used for guests and which does have restrictions applied in > >> emulate_cmd_reg function. > >> > >> So, for the hardware domain, there is no "sticky" bit possible and for the > >> guest domains if the physical contents of the PCI_COMMAND register > >> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to > >> use PCI_COMMAND_INTX_DISABLE bit set. > >> > >> So, from hardware domain POV, this should not be a problem, but from > >> guests view it can. Let's imagine that the hardware domain can handle > >> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware > >> domain can decide what can be used for the interrupt source (again, no > >> restriction here) and program PCI_COMMAND accordingly. > >> Guest domains need to align with this configuration, e.g. if INTx was disabled > >> by the hardware domain then INTx cannot be enabled for guests > > Why? It's the DomU that's in control of the device, so it ought to > > be able to pick any of the three. I don't think Dom0 is involved in > > handling of interrupts from the device, and hence its own "dislike" > > of INTx ought to only extend to the period of time where Dom0 is > > controlling the device. This would be different if Xen's view was > > different, but as we seem to agree Xen's role here is solely to > > prevent invalid combinations getting established in hardware. > On top of a PCI device there is a physical host bridge and > physical bus topology which may impose restrictions from > Dom0 POV on that particular device. So, every PCI device > being passed through to a DomU may have different INTx > settings which do depend on Dom0 in our case. Hm, it's kind of weird. What happens if you play with this bit and the bridge doesn't support it? Also note that your current code would allow a domU to set the bit if previously unset, but it then won't allow the domU to clear it, which doesn't seem to be exactly what you are aiming for. Thanks, Roger.
On 03.11.2021 10:30, Oleksandr Andrushchenko wrote: > > > On 03.11.21 11:24, Jan Beulich wrote: >> On 03.11.2021 10:18, Oleksandr Andrushchenko wrote: >>> >>> On 03.11.21 11:11, Jan Beulich wrote: >>>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >>>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>>>>> On 02.11.21 15:54, Jan Beulich wrote: >>>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>>>>> + >>>>>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>>>>> + { >>>>>>>>>>> + /* >>>>>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>>>>> + * - host has INTx disabled >>>>>>>>>>> + * - MSI/MSI-X enabled >>>>>>>>>>> + */ >>>>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>>> + else >>>>>>>>>>> + { >>>>>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>>>>> + >>>>>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>>> + } >>>>>>>>>> This last part should be Arm specific. On other architectures we >>>>>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>>>>> interrupt delivery mode for the device. >>>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>>>>> the bit is clear.) >>>>>>>> Sure, but this code is making the bit sticky, by not allowing >>>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>>>>> to be Arm only. >>>>>>> Isn't the "else" part questionable even on Arm? >>>>>> It is. Once fixed I can't see anything Arm specific here >>>>> Well, I have looked at the code one more time and everything seems to >>>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >>>>> guest_cmd_write. The former is used for the hardware domain and has >>>>> *no restrictions* on writing PCI_COMMAND register contents and the later >>>>> is only used for guests and which does have restrictions applied in >>>>> emulate_cmd_reg function. >>>>> >>>>> So, for the hardware domain, there is no "sticky" bit possible and for the >>>>> guest domains if the physical contents of the PCI_COMMAND register >>>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >>>>> use PCI_COMMAND_INTX_DISABLE bit set. >>>>> >>>>> So, from hardware domain POV, this should not be a problem, but from >>>>> guests view it can. Let's imagine that the hardware domain can handle >>>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >>>>> domain can decide what can be used for the interrupt source (again, no >>>>> restriction here) and program PCI_COMMAND accordingly. >>>>> Guest domains need to align with this configuration, e.g. if INTx was disabled >>>>> by the hardware domain then INTx cannot be enabled for guests >>>> Why? It's the DomU that's in control of the device, so it ought to >>>> be able to pick any of the three. I don't think Dom0 is involved in >>>> handling of interrupts from the device, and hence its own "dislike" >>>> of INTx ought to only extend to the period of time where Dom0 is >>>> controlling the device. This would be different if Xen's view was >>>> different, but as we seem to agree Xen's role here is solely to >>>> prevent invalid combinations getting established in hardware. >>> On top of a PCI device there is a physical host bridge and >>> physical bus topology which may impose restrictions from >>> Dom0 POV on that particular device. >> Well, such physical restrictions may mean INTx doesn't actually work, >> but this won't mean the DomU isn't free in choosing the bit's setting. >> The bit merely controls whether the device is allowed to assert its >> interrupt pin. Hence ... >> >>> So, every PCI device >>> being passed through to a DomU may have different INTx >>> settings which do depend on Dom0 in our case. >> ... I'm still unconvinced of this. > Ok, so I can accept any suggestion how to solve this. It seems that > we already have number of no go scenarios here, but still it is not > clear to me what could be an acceptable approach here. Namely: > what do we do with INTx bit for guests? > 1. I can leave it as is in the patch > 2. I can remove INTx emulation and let the guest decide and program INTx > 3. What else can I do? Aiui you want to prevent the guest from clearing the bit if either MSI or MSI-X are in use. Symmetrically, when the guest enables MSI or MSI-X, you will want to force the bit set (which may well be in a separate, future patch). Jan
On 03.11.21 11:39, Roger Pau Monné wrote: > On Wed, Nov 03, 2021 at 09:18:03AM +0000, Oleksandr Andrushchenko wrote: >> >> On 03.11.21 11:11, Jan Beulich wrote: >>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>>>> On 02.11.21 15:54, Jan Beulich wrote: >>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>>>> + >>>>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>>>> + { >>>>>>>>>> + /* >>>>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>>>> + * - host has INTx disabled >>>>>>>>>> + * - MSI/MSI-X enabled >>>>>>>>>> + */ >>>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>> + else >>>>>>>>>> + { >>>>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>>>> + >>>>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>> + } >>>>>>>>> This last part should be Arm specific. On other architectures we >>>>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>>>> interrupt delivery mode for the device. >>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>>>> the bit is clear.) >>>>>>> Sure, but this code is making the bit sticky, by not allowing >>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>>>> to be Arm only. >>>>>> Isn't the "else" part questionable even on Arm? >>>>> It is. Once fixed I can't see anything Arm specific here >>>> Well, I have looked at the code one more time and everything seems to >>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >>>> guest_cmd_write. The former is used for the hardware domain and has >>>> *no restrictions* on writing PCI_COMMAND register contents and the later >>>> is only used for guests and which does have restrictions applied in >>>> emulate_cmd_reg function. >>>> >>>> So, for the hardware domain, there is no "sticky" bit possible and for the >>>> guest domains if the physical contents of the PCI_COMMAND register >>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >>>> use PCI_COMMAND_INTX_DISABLE bit set. >>>> >>>> So, from hardware domain POV, this should not be a problem, but from >>>> guests view it can. Let's imagine that the hardware domain can handle >>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >>>> domain can decide what can be used for the interrupt source (again, no >>>> restriction here) and program PCI_COMMAND accordingly. >>>> Guest domains need to align with this configuration, e.g. if INTx was disabled >>>> by the hardware domain then INTx cannot be enabled for guests >>> Why? It's the DomU that's in control of the device, so it ought to >>> be able to pick any of the three. I don't think Dom0 is involved in >>> handling of interrupts from the device, and hence its own "dislike" >>> of INTx ought to only extend to the period of time where Dom0 is >>> controlling the device. This would be different if Xen's view was >>> different, but as we seem to agree Xen's role here is solely to >>> prevent invalid combinations getting established in hardware. >> On top of a PCI device there is a physical host bridge and >> physical bus topology which may impose restrictions from >> Dom0 POV on that particular device. So, every PCI device >> being passed through to a DomU may have different INTx >> settings which do depend on Dom0 in our case. > Hm, it's kind of weird. What happens if you play with this bit and the > bridge doesn't support it? For that reason I think it is enough to relay on some reference value which shows if INTx can be used. For that I suggest we depend on Dom0 for now and read this reference PCI_COMMAND value while in init_bars + is_hardware_domain. Then this can be used to feed the initial value of the PCI_COMMAND for guests. This way Dom0 solves the problem "what is supported for this PCI device with respect to the bus topology and host bridge" > > Also note that your current code would allow a domU to set the bit if > previously unset, but it then won't allow the domU to clear it, which > doesn't seem to be exactly what you are aiming for. That was noted before. If we use the reference value and use it as an initial value of the PCI_COMMAND for the guests (remember I use 0 in the patch which resets PCI_COMMAND for the guests and check the real PCI_COMMAND contents to decide on INTx). So, this reference value can be used in checks: if ( pdev->vpci->msi->enabled ) cmd |= PCI_COMMAND_INTX_DISABLE; else { if ( pdev->cmd_ref_value & PCI_COMMAND_INTX_DISABLE ) ^^^^^^^^^^^^^^ cmd |= PCI_COMMAND_INTX_DISABLE; } init_bars: if (hwdom) pdev->cmd_ref_value = read(PCI_COMMAND) > > Thanks, Roger. >
On 03.11.21 11:49, Jan Beulich wrote: > On 03.11.2021 10:30, Oleksandr Andrushchenko wrote: >> >> On 03.11.21 11:24, Jan Beulich wrote: >>> On 03.11.2021 10:18, Oleksandr Andrushchenko wrote: >>>> On 03.11.21 11:11, Jan Beulich wrote: >>>>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote: >>>>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote: >>>>>>> On 02.11.21 15:54, Jan Beulich wrote: >>>>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote: >>>>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote: >>>>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote: >>>>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote: >>>>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>>>> @@ -451,6 +451,32 @@ 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. */ >>>>>>>>>>>> + >>>>>>>>>>>> + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>>>>>>>> + { >>>>>>>>>>>> + /* >>>>>>>>>>>> + * Guest wants to enable INTx. It can't be enabled if: >>>>>>>>>>>> + * - host has INTx disabled >>>>>>>>>>>> + * - MSI/MSI-X enabled >>>>>>>>>>>> + */ >>>>>>>>>>>> + if ( pdev->vpci->msi->enabled ) >>>>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>>>> + else >>>>>>>>>>>> + { >>>>>>>>>>>> + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); >>>>>>>>>>>> + >>>>>>>>>>>> + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) >>>>>>>>>>>> + cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>>>>>>>> + } >>>>>>>>>>> This last part should be Arm specific. On other architectures we >>>>>>>>>>> likely want the guest to modify INTx disable in order to select the >>>>>>>>>>> interrupt delivery mode for the device. >>>>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X >>>>>>>>>> enabled - only one of the three is supposed to be active at a time. >>>>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when >>>>>>>>>> the bit is clear.) >>>>>>>>> Sure, but this code is making the bit sticky, by not allowing >>>>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on >>>>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs >>>>>>>>> to be Arm only. >>>>>>>> Isn't the "else" part questionable even on Arm? >>>>>>> It is. Once fixed I can't see anything Arm specific here >>>>>> Well, I have looked at the code one more time and everything seems to >>>>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and >>>>>> guest_cmd_write. The former is used for the hardware domain and has >>>>>> *no restrictions* on writing PCI_COMMAND register contents and the later >>>>>> is only used for guests and which does have restrictions applied in >>>>>> emulate_cmd_reg function. >>>>>> >>>>>> So, for the hardware domain, there is no "sticky" bit possible and for the >>>>>> guest domains if the physical contents of the PCI_COMMAND register >>>>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to >>>>>> use PCI_COMMAND_INTX_DISABLE bit set. >>>>>> >>>>>> So, from hardware domain POV, this should not be a problem, but from >>>>>> guests view it can. Let's imagine that the hardware domain can handle >>>>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware >>>>>> domain can decide what can be used for the interrupt source (again, no >>>>>> restriction here) and program PCI_COMMAND accordingly. >>>>>> Guest domains need to align with this configuration, e.g. if INTx was disabled >>>>>> by the hardware domain then INTx cannot be enabled for guests >>>>> Why? It's the DomU that's in control of the device, so it ought to >>>>> be able to pick any of the three. I don't think Dom0 is involved in >>>>> handling of interrupts from the device, and hence its own "dislike" >>>>> of INTx ought to only extend to the period of time where Dom0 is >>>>> controlling the device. This would be different if Xen's view was >>>>> different, but as we seem to agree Xen's role here is solely to >>>>> prevent invalid combinations getting established in hardware. >>>> On top of a PCI device there is a physical host bridge and >>>> physical bus topology which may impose restrictions from >>>> Dom0 POV on that particular device. >>> Well, such physical restrictions may mean INTx doesn't actually work, >>> but this won't mean the DomU isn't free in choosing the bit's setting. >>> The bit merely controls whether the device is allowed to assert its >>> interrupt pin. Hence ... >>> >>>> So, every PCI device >>>> being passed through to a DomU may have different INTx >>>> settings which do depend on Dom0 in our case. >>> ... I'm still unconvinced of this. >> Ok, so I can accept any suggestion how to solve this. It seems that >> we already have number of no go scenarios here, but still it is not >> clear to me what could be an acceptable approach here. Namely: >> what do we do with INTx bit for guests? >> 1. I can leave it as is in the patch >> 2. I can remove INTx emulation and let the guest decide and program INTx >> 3. What else can I do? > Aiui you want to prevent the guest from clearing the bit if either > MSI or MSI-X are in use. Symmetrically, when the guest enables MSI > or MSI-X, you will want to force the bit set (which may well be in > a separate, future patch). static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) { /* TODO: Add proper emulation for all bits of the command register. */ if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) { /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ #ifdef CONFIG_HAS_PCI_MSI if ( pdev->vpci->msi->enabled ) cmd |= PCI_COMMAND_INTX_DISABLE; #endif } return cmd; } Is this what you mean? > Jan > Thank you, Oleksandr
On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: > On 03.11.21 11:49, Jan Beulich wrote: >> Aiui you want to prevent the guest from clearing the bit if either >> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI >> or MSI-X, you will want to force the bit set (which may well be in >> a separate, future patch). > static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) > { > /* TODO: Add proper emulation for all bits of the command register. */ > > if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > { > /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ > #ifdef CONFIG_HAS_PCI_MSI > if ( pdev->vpci->msi->enabled ) > cmd |= PCI_COMMAND_INTX_DISABLE; > #endif > } > > return cmd; > } > > Is this what you mean? Something along these lines, yes. I'd omit the outer if() for clarity / brevity. Jan
On 03.11.21 12:34, Jan Beulich wrote: > On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: >> On 03.11.21 11:49, Jan Beulich wrote: >>> Aiui you want to prevent the guest from clearing the bit if either >>> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI >>> or MSI-X, you will want to force the bit set (which may well be in >>> a separate, future patch). >> static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) >> { >> /* TODO: Add proper emulation for all bits of the command register. */ >> >> if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >> { >> /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >> #ifdef CONFIG_HAS_PCI_MSI >> if ( pdev->vpci->msi->enabled ) >> cmd |= PCI_COMMAND_INTX_DISABLE; >> #endif >> } >> >> return cmd; >> } >> >> Is this what you mean? > Something along these lines, yes. I'd omit the outer if() for clarity / > brevity. Sure, thank you! @Roger are you ok with this approach? > > Jan > >
On Wed, Nov 03, 2021 at 10:36:36AM +0000, Oleksandr Andrushchenko wrote: > > > On 03.11.21 12:34, Jan Beulich wrote: > > On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: > >> On 03.11.21 11:49, Jan Beulich wrote: > >>> Aiui you want to prevent the guest from clearing the bit if either > >>> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI > >>> or MSI-X, you will want to force the bit set (which may well be in > >>> a separate, future patch). > >> static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) > >> { > >> /* TODO: Add proper emulation for all bits of the command register. */ > >> > >> if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > >> { > >> /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ > >> #ifdef CONFIG_HAS_PCI_MSI > >> if ( pdev->vpci->msi->enabled ) > >> cmd |= PCI_COMMAND_INTX_DISABLE; > >> #endif > >> } > >> > >> return cmd; > >> } > >> > >> Is this what you mean? > > Something along these lines, yes. I'd omit the outer if() for clarity / > > brevity. > Sure, thank you! > @Roger are you ok with this approach? Sure, I would even do: #ifdef CONFIG_HAS_PCI_MSI if ( !(cmd & PCI_COMMAND_INTX_DISABLE) && 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 There's no need for the outer check if there's no support for MSI. Thanks, Roger.
On 03.11.21 13:01, Roger Pau Monné wrote: > On Wed, Nov 03, 2021 at 10:36:36AM +0000, Oleksandr Andrushchenko wrote: >> >> On 03.11.21 12:34, Jan Beulich wrote: >>> On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: >>>> On 03.11.21 11:49, Jan Beulich wrote: >>>>> Aiui you want to prevent the guest from clearing the bit if either >>>>> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI >>>>> or MSI-X, you will want to force the bit set (which may well be in >>>>> a separate, future patch). >>>> static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) >>>> { >>>> /* TODO: Add proper emulation for all bits of the command register. */ >>>> >>>> if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>> { >>>> /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>> #ifdef CONFIG_HAS_PCI_MSI >>>> if ( pdev->vpci->msi->enabled ) >>>> cmd |= PCI_COMMAND_INTX_DISABLE; >>>> #endif >>>> } >>>> >>>> return cmd; >>>> } >>>> >>>> Is this what you mean? >>> Something along these lines, yes. I'd omit the outer if() for clarity / >>> brevity. >> Sure, thank you! >> @Roger are you ok with this approach? > Sure, I would even do: > > #ifdef CONFIG_HAS_PCI_MSI > if ( !(cmd & PCI_COMMAND_INTX_DISABLE) && 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 > > There's no need for the outer check if there's no support for MSI. Ok, sounds good! Thank you both!! > > Thanks, Roger.
On Wed, Nov 03, 2021 at 11:02:37AM +0000, Oleksandr Andrushchenko wrote: > > > On 03.11.21 13:01, Roger Pau Monné wrote: > > On Wed, Nov 03, 2021 at 10:36:36AM +0000, Oleksandr Andrushchenko wrote: > >> > >> On 03.11.21 12:34, Jan Beulich wrote: > >>> On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: > >>>> On 03.11.21 11:49, Jan Beulich wrote: > >>>>> Aiui you want to prevent the guest from clearing the bit if either > >>>>> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI > >>>>> or MSI-X, you will want to force the bit set (which may well be in > >>>>> a separate, future patch). > >>>> static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) > >>>> { > >>>> /* TODO: Add proper emulation for all bits of the command register. */ > >>>> > >>>> if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) > >>>> { > >>>> /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ > >>>> #ifdef CONFIG_HAS_PCI_MSI > >>>> if ( pdev->vpci->msi->enabled ) > >>>> cmd |= PCI_COMMAND_INTX_DISABLE; > >>>> #endif > >>>> } > >>>> > >>>> return cmd; > >>>> } > >>>> > >>>> Is this what you mean? > >>> Something along these lines, yes. I'd omit the outer if() for clarity / > >>> brevity. > >> Sure, thank you! > >> @Roger are you ok with this approach? > > Sure, I would even do: > > > > #ifdef CONFIG_HAS_PCI_MSI > > if ( !(cmd & PCI_COMMAND_INTX_DISABLE) && 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 > > > > There's no need for the outer check if there's no support for MSI. > Ok, sounds good! > Thank you both!! In fact you could even remove the check for !(cmd & PCI_COMMAND_INTX_DISABLE) and always set PCI_COMMAND_INTX_DISABLE if MSI is enabled, which I think is what Jan was pointing to in his previous reply. Regards, Roger.
On 03.11.21 13:26, Roger Pau Monné wrote: > On Wed, Nov 03, 2021 at 11:02:37AM +0000, Oleksandr Andrushchenko wrote: >> >> On 03.11.21 13:01, Roger Pau Monné wrote: >>> On Wed, Nov 03, 2021 at 10:36:36AM +0000, Oleksandr Andrushchenko wrote: >>>> On 03.11.21 12:34, Jan Beulich wrote: >>>>> On 03.11.2021 11:24, Oleksandr Andrushchenko wrote: >>>>>> On 03.11.21 11:49, Jan Beulich wrote: >>>>>>> Aiui you want to prevent the guest from clearing the bit if either >>>>>>> MSI or MSI-X are in use. Symmetrically, when the guest enables MSI >>>>>>> or MSI-X, you will want to force the bit set (which may well be in >>>>>>> a separate, future patch). >>>>>> static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd) >>>>>> { >>>>>> /* TODO: Add proper emulation for all bits of the command register. */ >>>>>> >>>>>> if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) >>>>>> { >>>>>> /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */ >>>>>> #ifdef CONFIG_HAS_PCI_MSI >>>>>> if ( pdev->vpci->msi->enabled ) >>>>>> cmd |= PCI_COMMAND_INTX_DISABLE; >>>>>> #endif >>>>>> } >>>>>> >>>>>> return cmd; >>>>>> } >>>>>> >>>>>> Is this what you mean? >>>>> Something along these lines, yes. I'd omit the outer if() for clarity / >>>>> brevity. >>>> Sure, thank you! >>>> @Roger are you ok with this approach? >>> Sure, I would even do: >>> >>> #ifdef CONFIG_HAS_PCI_MSI >>> if ( !(cmd & PCI_COMMAND_INTX_DISABLE) && 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 >>> >>> There's no need for the outer check if there's no support for MSI. >> Ok, sounds good! >> Thank you both!! > In fact you could even remove the check for !(cmd & > PCI_COMMAND_INTX_DISABLE) and always set PCI_COMMAND_INTX_DISABLE if > MSI is enabled, which I think is what Jan was pointing to in his > previous reply. Ok, I will > > Regards, Roger. >
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index f23c956cde6c..754aeb5a584f 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -451,6 +451,32 @@ 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. */ + + if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 ) + { + /* + * Guest wants to enable INTx. It can't be enabled if: + * - host has INTx disabled + * - MSI/MSI-X enabled + */ + if ( pdev->vpci->msi->enabled ) + cmd |= PCI_COMMAND_INTX_DISABLE; + else + { + uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg); + + if ( current_cmd & PCI_COMMAND_INTX_DISABLE ) + cmd |= PCI_COMMAND_INTX_DISABLE; + } + } + + cmd_write(pdev, reg, cmd, data); +} + static void bar_write(const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data) { @@ -598,9 +624,12 @@ static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom) struct vpci_bar *bars = header->bars; int rc; - /* Setup a handler for the command register: same for hwdom and guests. */ - rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND, - 2, header); + if ( is_hwdom ) + rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, + PCI_COMMAND, 2, header); + else + rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write, + PCI_COMMAND, 2, header); if ( rc ) return rc;