Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> Writes to the BARs are ignored if memory decoding is enabled for the
> device, and the same happen with ROM BARs if the write is an attempt
> to change the position of the BAR without disabling it first.
> 
> The reason of ignoring such writes is a limitation in Xen, as it would
> need to unmap the BAR, change the address, and remap the BAR at the
> new position, which the current logic doesn't support.
> 
> Some devices however seem to (wrongly) have the memory decoding bit
> hardcoded to enabled, and attempts to disable it don't get reflected
> on the command register.
> 
> This causes issues for well behaved guests that disable memory
> decoding and then try to size the BARs, as vPCI will think memory
> decoding is still enabled and ignore the write.

Just to avoid misunderstandings: "guests" here includes Dom0? In such
cases we typically prefer to say "domains". This then also affects
the next (final) paragraph.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -128,7 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>      }
>  
>      if ( !rom_only )
> +    {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> +        header->bars_mapped = map;
> +    }
>      else
>          ASSERT_UNREACHABLE();
>  }
> @@ -355,13 +358,13 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>  static void cf_check cmd_write(
>      const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
>  {
> -    uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> +    struct vpci_header *header = data;
>  
>      /*
>       * Let Dom0 play with all the bits directly except for the memory
>       * decoding one.
>       */
> -    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> +    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
>          /*
>           * Ignore the error. No memory has been added or removed from the p2m
>           * (because the actual p2m changes are deferred in defer_map) and the
> @@ -388,12 +391,12 @@ static void cf_check bar_write(
>      else
>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>  
> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
> +    if ( bar->enabled )

In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
not clear to me why you don't here, could you explain this to me? (I'm
therefore undecided whether this is merely a cosmetic [consistency] issue.)

>      {
>          /* If the value written is the current one avoid printing a warning. 
> */
>          if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>              gprintk(XENLOG_WARNING,
> -                    "%pp: ignored BAR %zu write with memory decoding 
> enabled\n",
> +                    "%pp: ignored BAR %zu write while mapped\n",
>                      &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>          return;
>      }
> @@ -422,13 +425,12 @@ static void cf_check rom_write(
>  {
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *rom = data;
> -    uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>      bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE;
>  
> -    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled )
> +    if ( header->bars_mapped && header->rom_enabled && new_enabled )
>      {
>          gprintk(XENLOG_WARNING,
> -                "%pp: ignored ROM BAR write with memory decoding enabled\n",
> +                "%pp: ignored ROM BAR write while mapped\n",
>                  &pdev->sbdf);
>          return;
>      }
> @@ -440,7 +442,7 @@ static void cf_check rom_write(
>           */
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  
> -    if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled )
> +    if ( !header->bars_mapped || header->rom_enabled == new_enabled )
>      {
>          /* Just update the ROM BAR field. */
>          header->rom_enabled = new_enabled;