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

[Jan Beulich <jbeulich@xxxxxxxx>]

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