Message ID | 20220204063459.680961-10-andr2000@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | PCI devices passthrough on Arm, part 3 | expand |
On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -454,6 +454,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 || pdev->vpci->msix->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); > +} It's not really clear to me whether the TODO warrants this being a separate function. Personally I'd find it preferable if the logic was folded into cmd_write(). With this and ... > --- a/xen/drivers/vpci/msi.c > +++ b/xen/drivers/vpci/msi.c > @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, > > if ( vpci_msi_arch_enable(msi, pdev, vectors) ) > return; > + > + /* Make sure guest doesn't enable INTx while enabling MSI. */ > + if ( !is_hardware_domain(pdev->domain) ) > + pci_intx(pdev, false); > } > else > vpci_msi_arch_disable(msi, pdev); > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, > for ( i = 0; i < msix->max_entries; i++ ) > if ( !msix->entries[i].masked && msix->entries[i].updated ) > update_entry(&msix->entries[i], pdev, i); > + > + /* Make sure guest doesn't enable INTx while enabling MSI-X. */ > + if ( !is_hardware_domain(pdev->domain) ) > + pci_intx(pdev, false); > } > else if ( !new_enabled && msix->enabled ) > { ... this done (as requested) behind the back of the guest, what's the idea wrt the guest reading the command register? That continues to be wired to vpci_hw_read16() (and hence accesses the underlying hardware value irrespective of what patch 4 did). Jan
On 04.02.22 16:25, Jan Beulich wrote: > On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -454,6 +454,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 || pdev->vpci->msix->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); >> +} > It's not really clear to me whether the TODO warrants this being a > separate function. Personally I'd find it preferable if the logic > was folded into cmd_write(). Not sure cmd_write needs to have guest's logic. And what's the profit? Later on, when we decide how PCI_COMMAND can be emulated this code will live in guest_cmd_write anyways > > With this and ... > >> --- a/xen/drivers/vpci/msi.c >> +++ b/xen/drivers/vpci/msi.c >> @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, >> >> if ( vpci_msi_arch_enable(msi, pdev, vectors) ) >> return; >> + >> + /* Make sure guest doesn't enable INTx while enabling MSI. */ >> + if ( !is_hardware_domain(pdev->domain) ) >> + pci_intx(pdev, false); >> } >> else >> vpci_msi_arch_disable(msi, pdev); >> --- a/xen/drivers/vpci/msix.c >> +++ b/xen/drivers/vpci/msix.c >> @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, >> for ( i = 0; i < msix->max_entries; i++ ) >> if ( !msix->entries[i].masked && msix->entries[i].updated ) >> update_entry(&msix->entries[i], pdev, i); >> + >> + /* Make sure guest doesn't enable INTx while enabling MSI-X. */ >> + if ( !is_hardware_domain(pdev->domain) ) >> + pci_intx(pdev, false); >> } >> else if ( !new_enabled && msix->enabled ) >> { > ... this done (as requested) behind the back of the guest, what's the > idea wrt the guest reading the command register? That continues to be > wired to vpci_hw_read16() (and hence accesses the underlying hardware > value irrespective of what patch 4 did). Yes, good point. We need to add guest_cmd_read counterpart, so we can also implement the same logic as in guest_cmd_write wrt to INTx bit. > > Jan > Thank you, Oleksandr
On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: > On 04.02.22 16:25, Jan Beulich wrote: >> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>> --- a/xen/drivers/vpci/header.c >>> +++ b/xen/drivers/vpci/header.c >>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>> +} >> It's not really clear to me whether the TODO warrants this being a >> separate function. Personally I'd find it preferable if the logic >> was folded into cmd_write(). > Not sure cmd_write needs to have guest's logic. And what's the > profit? Later on, when we decide how PCI_COMMAND can be emulated > this code will live in guest_cmd_write anyways Why "will"? There's nothing conceptually wrong with putting all the emulation logic into cmd_write(), inside an if(!hwdom) conditional. If and when we gain CET-IBT support on the x86 side (and I'm told there's an Arm equivalent of this), then to make this as useful as possible it is going to be desirable to limit the number of functions called through function pointers. You may have seen Andrew's huge "x86: Support for CET Indirect Branch Tracking" series. We want to keep down the number of such annotations; the vast part of the series is about adding of such. Jan
On 08.02.22 11:33, Jan Beulich wrote: > On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: >> On 04.02.22 16:25, Jan Beulich wrote: >>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>> --- a/xen/drivers/vpci/header.c >>>> +++ b/xen/drivers/vpci/header.c >>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>>> +} >>> It's not really clear to me whether the TODO warrants this being a >>> separate function. Personally I'd find it preferable if the logic >>> was folded into cmd_write(). >> Not sure cmd_write needs to have guest's logic. And what's the >> profit? Later on, when we decide how PCI_COMMAND can be emulated >> this code will live in guest_cmd_write anyways > Why "will"? There's nothing conceptually wrong with putting all the > emulation logic into cmd_write(), inside an if(!hwdom) conditional. > If and when we gain CET-IBT support on the x86 side (and I'm told > there's an Arm equivalent of this), then to make this as useful as > possible it is going to be desirable to limit the number of functions > called through function pointers. You may have seen Andrew's huge > "x86: Support for CET Indirect Branch Tracking" series. We want to > keep down the number of such annotations; the vast part of the series > is about adding of such. Well, while I see nothing bad with that, from the code organization it would look a bit strange: we don't differentiate hwdom in vpci handlers, but instead provide one for hwdom and one for guests. While I understand your concern I still think that at the moment it will be more in line with the existing code if we provide a dedicated handler. Once we are all set with the handlers we may want performing a refactoring with limiting the number of register handlers. @Roger, what's your view on this? > Jan > > Thank you, Oleksandr
On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: > > > On 08.02.22 11:33, Jan Beulich wrote: >> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: >>> On 04.02.22 16:25, Jan Beulich wrote: >>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>> --- a/xen/drivers/vpci/header.c >>>>> +++ b/xen/drivers/vpci/header.c >>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>>>> +} >>>> It's not really clear to me whether the TODO warrants this being a >>>> separate function. Personally I'd find it preferable if the logic >>>> was folded into cmd_write(). >>> Not sure cmd_write needs to have guest's logic. And what's the >>> profit? Later on, when we decide how PCI_COMMAND can be emulated >>> this code will live in guest_cmd_write anyways >> Why "will"? There's nothing conceptually wrong with putting all the >> emulation logic into cmd_write(), inside an if(!hwdom) conditional. >> If and when we gain CET-IBT support on the x86 side (and I'm told >> there's an Arm equivalent of this), then to make this as useful as >> possible it is going to be desirable to limit the number of functions >> called through function pointers. You may have seen Andrew's huge >> "x86: Support for CET Indirect Branch Tracking" series. We want to >> keep down the number of such annotations; the vast part of the series >> is about adding of such. > Well, while I see nothing bad with that, from the code organization > it would look a bit strange: we don't differentiate hwdom in vpci > handlers, but instead provide one for hwdom and one for guests. > While I understand your concern I still think that at the moment > it will be more in line with the existing code if we provide a dedicated > handler. The existing code only deals with Dom0, and hence doesn't have any pairs of handlers. FTAOD what I said above applies equally to other separate guest read/write handlers you may be introducing. The exception being when e.g. a hardware access handler is put in place for Dom0 (for obvious reasons, I think). Jan
On 08.02.22 11:52, Jan Beulich wrote: > On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: >> >> On 08.02.22 11:33, Jan Beulich wrote: >>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: >>>> On 04.02.22 16:25, Jan Beulich wrote: >>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>>> --- a/xen/drivers/vpci/header.c >>>>>> +++ b/xen/drivers/vpci/header.c >>>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>>>>> +} >>>>> It's not really clear to me whether the TODO warrants this being a >>>>> separate function. Personally I'd find it preferable if the logic >>>>> was folded into cmd_write(). >>>> Not sure cmd_write needs to have guest's logic. And what's the >>>> profit? Later on, when we decide how PCI_COMMAND can be emulated >>>> this code will live in guest_cmd_write anyways >>> Why "will"? There's nothing conceptually wrong with putting all the >>> emulation logic into cmd_write(), inside an if(!hwdom) conditional. >>> If and when we gain CET-IBT support on the x86 side (and I'm told >>> there's an Arm equivalent of this), then to make this as useful as >>> possible it is going to be desirable to limit the number of functions >>> called through function pointers. You may have seen Andrew's huge >>> "x86: Support for CET Indirect Branch Tracking" series. We want to >>> keep down the number of such annotations; the vast part of the series >>> is about adding of such. >> Well, while I see nothing bad with that, from the code organization >> it would look a bit strange: we don't differentiate hwdom in vpci >> handlers, but instead provide one for hwdom and one for guests. >> While I understand your concern I still think that at the moment >> it will be more in line with the existing code if we provide a dedicated >> handler. > The existing code only deals with Dom0, and hence doesn't have any > pairs of handlers. This is fair > FTAOD what I said above applies equally to other > separate guest read/write handlers you may be introducing. The > exception being when e.g. a hardware access handler is put in place > for Dom0 (for obvious reasons, I think). @Roger, what's your preference here? > > Jan > Thank you, Oleksandr
On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote: > > > On 08.02.22 11:52, Jan Beulich wrote: > > On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: > >> > >> On 08.02.22 11:33, Jan Beulich wrote: > >>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: > >>>> On 04.02.22 16:25, Jan Beulich wrote: > >>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: > >>>>>> --- a/xen/drivers/vpci/header.c > >>>>>> +++ b/xen/drivers/vpci/header.c > >>>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); > >>>>>> +} > >>>>> It's not really clear to me whether the TODO warrants this being a > >>>>> separate function. Personally I'd find it preferable if the logic > >>>>> was folded into cmd_write(). > >>>> Not sure cmd_write needs to have guest's logic. And what's the > >>>> profit? Later on, when we decide how PCI_COMMAND can be emulated > >>>> this code will live in guest_cmd_write anyways > >>> Why "will"? There's nothing conceptually wrong with putting all the > >>> emulation logic into cmd_write(), inside an if(!hwdom) conditional. > >>> If and when we gain CET-IBT support on the x86 side (and I'm told > >>> there's an Arm equivalent of this), then to make this as useful as > >>> possible it is going to be desirable to limit the number of functions > >>> called through function pointers. You may have seen Andrew's huge > >>> "x86: Support for CET Indirect Branch Tracking" series. We want to > >>> keep down the number of such annotations; the vast part of the series > >>> is about adding of such. > >> Well, while I see nothing bad with that, from the code organization > >> it would look a bit strange: we don't differentiate hwdom in vpci > >> handlers, but instead provide one for hwdom and one for guests. > >> While I understand your concern I still think that at the moment > >> it will be more in line with the existing code if we provide a dedicated > >> handler. > > The existing code only deals with Dom0, and hence doesn't have any > > pairs of handlers. > This is fair > > FTAOD what I said above applies equally to other > > separate guest read/write handlers you may be introducing. The > > exception being when e.g. a hardware access handler is put in place > > for Dom0 (for obvious reasons, I think). > @Roger, what's your preference here? > > The newly introduced handler ends up calling the existing one, so in this case it might make sense to expand cmd_write to also cater for the domU case? I think we need to be sensible here in that we don't want to end up with handlers like: register_read(...) { if ( is_hardware_domain() ) .... else ... } If there's shared code it's IMO better to not create as guest specific handler. It's also more risky to use the same handlers for dom0 and domU, as a change intended to dom0 only might end up leaking in the domU path and that could easily become a security issue. Thanks, Roger.
On 08.02.22 13:11, Roger Pau Monné wrote: > On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote: >> >> On 08.02.22 11:52, Jan Beulich wrote: >>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: >>>> On 08.02.22 11:33, Jan Beulich wrote: >>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: >>>>>> On 04.02.22 16:25, Jan Beulich wrote: >>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>>>>>>> +} >>>>>>> It's not really clear to me whether the TODO warrants this being a >>>>>>> separate function. Personally I'd find it preferable if the logic >>>>>>> was folded into cmd_write(). >>>>>> Not sure cmd_write needs to have guest's logic. And what's the >>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated >>>>>> this code will live in guest_cmd_write anyways >>>>> Why "will"? There's nothing conceptually wrong with putting all the >>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional. >>>>> If and when we gain CET-IBT support on the x86 side (and I'm told >>>>> there's an Arm equivalent of this), then to make this as useful as >>>>> possible it is going to be desirable to limit the number of functions >>>>> called through function pointers. You may have seen Andrew's huge >>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to >>>>> keep down the number of such annotations; the vast part of the series >>>>> is about adding of such. >>>> Well, while I see nothing bad with that, from the code organization >>>> it would look a bit strange: we don't differentiate hwdom in vpci >>>> handlers, but instead provide one for hwdom and one for guests. >>>> While I understand your concern I still think that at the moment >>>> it will be more in line with the existing code if we provide a dedicated >>>> handler. >>> The existing code only deals with Dom0, and hence doesn't have any >>> pairs of handlers. >> This is fair >>> FTAOD what I said above applies equally to other >>> separate guest read/write handlers you may be introducing. The >>> exception being when e.g. a hardware access handler is put in place >>> for Dom0 (for obvious reasons, I think). >> @Roger, what's your preference here? > The newly introduced handler ends up calling the existing one, But before doing so it implements guest specific logic which will be extended as we add more bits of emulation > so in > this case it might make sense to expand cmd_write to also cater for > the domU case? So, from the above I thought is was ok to have a dedicated handler > > I think we need to be sensible here in that we don't want to end up > with handlers like: > > register_read(...) > { > if ( is_hardware_domain() ) > .... > else > ... > } > > If there's shared code it's IMO better to not create as guest specific > handler. > > It's also more risky to use the same handlers for dom0 and domU, as a > change intended to dom0 only might end up leaking in the domU path and > that could easily become a security issue. So, just for your justification: BARs. Is this something we also want to be kept separate or we want if (is_hwdom)? I guess the former. > > Thanks, Roger. Thank you, Oleksandr
On Tue, Feb 08, 2022 at 11:29:07AM +0000, Oleksandr Andrushchenko wrote: > > > On 08.02.22 13:11, Roger Pau Monné wrote: > > On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote: > >> > >> On 08.02.22 11:52, Jan Beulich wrote: > >>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: > >>>> On 08.02.22 11:33, Jan Beulich wrote: > >>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: > >>>>>> On 04.02.22 16:25, Jan Beulich wrote: > >>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: > >>>>>>>> --- a/xen/drivers/vpci/header.c > >>>>>>>> +++ b/xen/drivers/vpci/header.c > >>>>>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); > >>>>>>>> +} > >>>>>>> It's not really clear to me whether the TODO warrants this being a > >>>>>>> separate function. Personally I'd find it preferable if the logic > >>>>>>> was folded into cmd_write(). > >>>>>> Not sure cmd_write needs to have guest's logic. And what's the > >>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated > >>>>>> this code will live in guest_cmd_write anyways > >>>>> Why "will"? There's nothing conceptually wrong with putting all the > >>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional. > >>>>> If and when we gain CET-IBT support on the x86 side (and I'm told > >>>>> there's an Arm equivalent of this), then to make this as useful as > >>>>> possible it is going to be desirable to limit the number of functions > >>>>> called through function pointers. You may have seen Andrew's huge > >>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to > >>>>> keep down the number of such annotations; the vast part of the series > >>>>> is about adding of such. > >>>> Well, while I see nothing bad with that, from the code organization > >>>> it would look a bit strange: we don't differentiate hwdom in vpci > >>>> handlers, but instead provide one for hwdom and one for guests. > >>>> While I understand your concern I still think that at the moment > >>>> it will be more in line with the existing code if we provide a dedicated > >>>> handler. > >>> The existing code only deals with Dom0, and hence doesn't have any > >>> pairs of handlers. > >> This is fair > >>> FTAOD what I said above applies equally to other > >>> separate guest read/write handlers you may be introducing. The > >>> exception being when e.g. a hardware access handler is put in place > >>> for Dom0 (for obvious reasons, I think). > >> @Roger, what's your preference here? > > The newly introduced handler ends up calling the existing one, > But before doing so it implements guest specific logic which will be > extended as we add more bits of emulation > > so in > > this case it might make sense to expand cmd_write to also cater for > > the domU case? > So, from the above I thought is was ok to have a dedicated handler Given the current proposal where you are only dealing with INTx I don't think it makes much sense to have a separate handler because you end up calling cmd_write anyway, so what's added there could very well be added at the top of cmd_write. > > > > I think we need to be sensible here in that we don't want to end up > > with handlers like: > > > > register_read(...) > > { > > if ( is_hardware_domain() ) > > .... > > else > > ... > > } > > > > If there's shared code it's IMO better to not create as guest specific > > handler. > > > > It's also more risky to use the same handlers for dom0 and domU, as a > > change intended to dom0 only might end up leaking in the domU path and > > that could easily become a security issue. > So, just for your justification: BARs. Is this something we also want > to be kept separate or we want if (is_hwdom)? > I guess the former. I think BAR access handling is sufficiently different between dom0 and domU that we want separate handlers. Thanks, Roger.
On 08.02.22 16:09, Roger Pau Monné wrote: > On Tue, Feb 08, 2022 at 11:29:07AM +0000, Oleksandr Andrushchenko wrote: >> >> On 08.02.22 13:11, Roger Pau Monné wrote: >>> On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote: >>>> On 08.02.22 11:52, Jan Beulich wrote: >>>>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote: >>>>>> On 08.02.22 11:33, Jan Beulich wrote: >>>>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote: >>>>>>>> On 04.02.22 16:25, Jan Beulich wrote: >>>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote: >>>>>>>>>> --- a/xen/drivers/vpci/header.c >>>>>>>>>> +++ b/xen/drivers/vpci/header.c >>>>>>>>>> @@ -454,6 +454,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 || pdev->vpci->msix->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); >>>>>>>>>> +} >>>>>>>>> It's not really clear to me whether the TODO warrants this being a >>>>>>>>> separate function. Personally I'd find it preferable if the logic >>>>>>>>> was folded into cmd_write(). >>>>>>>> Not sure cmd_write needs to have guest's logic. And what's the >>>>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated >>>>>>>> this code will live in guest_cmd_write anyways >>>>>>> Why "will"? There's nothing conceptually wrong with putting all the >>>>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional. >>>>>>> If and when we gain CET-IBT support on the x86 side (and I'm told >>>>>>> there's an Arm equivalent of this), then to make this as useful as >>>>>>> possible it is going to be desirable to limit the number of functions >>>>>>> called through function pointers. You may have seen Andrew's huge >>>>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to >>>>>>> keep down the number of such annotations; the vast part of the series >>>>>>> is about adding of such. >>>>>> Well, while I see nothing bad with that, from the code organization >>>>>> it would look a bit strange: we don't differentiate hwdom in vpci >>>>>> handlers, but instead provide one for hwdom and one for guests. >>>>>> While I understand your concern I still think that at the moment >>>>>> it will be more in line with the existing code if we provide a dedicated >>>>>> handler. >>>>> The existing code only deals with Dom0, and hence doesn't have any >>>>> pairs of handlers. >>>> This is fair >>>>> FTAOD what I said above applies equally to other >>>>> separate guest read/write handlers you may be introducing. The >>>>> exception being when e.g. a hardware access handler is put in place >>>>> for Dom0 (for obvious reasons, I think). >>>> @Roger, what's your preference here? >>> The newly introduced handler ends up calling the existing one, >> But before doing so it implements guest specific logic which will be >> extended as we add more bits of emulation >>> so in >>> this case it might make sense to expand cmd_write to also cater for >>> the domU case? >> So, from the above I thought is was ok to have a dedicated handler > Given the current proposal where you are only dealing with INTx I don't > think it makes much sense to have a separate handler because you end > up calling cmd_write anyway, so what's added there could very well be > added at the top of cmd_write. Good > >>> I think we need to be sensible here in that we don't want to end up >>> with handlers like: >>> >>> register_read(...) >>> { >>> if ( is_hardware_domain() ) >>> .... >>> else >>> ... >>> } >>> >>> If there's shared code it's IMO better to not create as guest specific >>> handler. >>> >>> It's also more risky to use the same handlers for dom0 and domU, as a >>> change intended to dom0 only might end up leaking in the domU path and >>> that could easily become a security issue. >> So, just for your justification: BARs. Is this something we also want >> to be kept separate or we want if (is_hwdom)? >> I guess the former. > I think BAR access handling is sufficiently different between dom0 and > domU that we want separate handlers. Makes sense > Thanks, Roger. Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 88ca1ad8211d..33d8c15ae6e8 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -454,6 +454,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 || pdev->vpci->msix->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) { @@ -661,8 +677,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; diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index e3ce46869dad..90465dcb4831 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, if ( vpci_msi_arch_enable(msi, pdev, vectors) ) return; + + /* Make sure guest doesn't enable INTx while enabling MSI. */ + if ( !is_hardware_domain(pdev->domain) ) + pci_intx(pdev, false); } else vpci_msi_arch_disable(msi, pdev); diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index d1dbfc6e0ffd..4c0e1836b589 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg, for ( i = 0; i < msix->max_entries; i++ ) if ( !msix->entries[i].masked && msix->entries[i].updated ) update_entry(&msix->entries[i], pdev, i); + + /* Make sure guest doesn't enable INTx while enabling MSI-X. */ + if ( !is_hardware_domain(pdev->domain) ) + pci_intx(pdev, false); } else if ( !new_enabled && msix->enabled ) {