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