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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2022 11:46, 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 have the memory decoding bit hardcoded to
> enabled, and attempts to disable it don't get reflected on the
> command register.

This isn't compliant with the spec, is it? It looks to contradict both
"When a 0 is written to this register, the device is logically
disconnected from the PCI bus for all accesses except configuration
accesses" and "Devices typically power up with all 0's in this
register, but Section 6.6 explains some exceptions" (quoting from the
old 3.0 spec, which I have readily to hand). The referenced section
then says "Such devices are required to support the Command register
disabling function described in Section 6.2.2".

How does any arbitrary OS go about sizing the BARs of such a device?

> 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.
> 
> Since vPCI doesn't explicitly care about whether the memory decoding
> bit is disabled as long as the BAR is not mapped in the guest p2m use
> the information in the vpci_bar to check whether the BAR is mapped,
> and refuse writes only based on that information.

>From purely a vPCI pov this looks to be a plausible solution (or
should I better say workaround). I guess the two pieces of code that
you alter would benefit from a comment as to it being intentional to
_not_ check the command register (anymore).

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -388,7 +388,7 @@ 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 )
>      {
>          /* If the value written is the current one avoid printing a warning. 
> */
>          if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
> @@ -425,7 +425,7 @@ static void cf_check rom_write(
>      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 ( rom->enabled && new_enabled )
>      {
>          gprintk(XENLOG_WARNING,
>                  "%pp: ignored ROM BAR write with memory decoding enabled\n",

The log message wording then wants adjustment, I guess?

What about

    if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled )

a few lines down from here? Besides still using the command register
value here not looking very consistent, wouldn't header->rom_enabled
here an in the intermediate if() also better be converted to
rom->enabled for consistency?

Then again - is you also dropping the check of header->rom_enabled
actually correct? While both are written to the same value by
modify_decoding(), both rom_write() and init_bars() can bring the
two booleans out of sync afaics.

Jan