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

[Jan Beulich <jbeulich@xxxxxxxx>]

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