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.
