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.