Re: [PATCH v6 09/13] vpci/header: emulate PCI_COMMAND register for guests

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.