Re: [XEN PATCH v4] hvmloader: Enable MMIO and I/O decode, after all resource allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2020 19:15, Harsha Shamsundara Havanur wrote:
> @@ -120,6 +121,11 @@ void pci_setup(void)
>       */
>      bool allow_memory_relocate = 1;
>  
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMORY
> +            != PCI_COMMAND_MEMORY);
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO
> +            != PCI_COMMAND_IO);

This still isn't in line with our default style, you will want eg:

    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMORY !=
                 PCI_COMMAND_MEMORY);
    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
                 PCI_COMMAND_IO);

> @@ -208,6 +214,20 @@ void pci_setup(void)
>              break;
>          }
>  
> +        /*
> +         * Disable memory and I/O decode,
> +         * It is recommended that BAR programming be done whilst
> +         * decode bits are cleared to avoid incorrect mappings being created,
> +         * when 64-bit memory BAR is programmed first by writing the lower 
> half
> +         * and then the upper half, which first maps to an address under 4G
> +         * replacing any RAM mapped in that address, which is not restored
> +         * back after the upper half is written and PCI memory is correctly
> +         * mapped to its intended high mem address.
> +         */

Please can you bring this comment into shape? The comma on the first
line doesn't fit with the capital letter the second line starts with.
Plus, if you really mean what is now on the second line to start on a
new one, then please also insert a line consisting of just * at the
properly indented position between the two. Additionally there's at
least one line exceeding the 80-chars-per-line limit.

> @@ -289,10 +309,6 @@ void pci_setup(void)
>                     devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
>          }
>  
> -        /* Enable bus mastering. */
> -        cmd = pci_readw(devfn, PCI_COMMAND);
> -        cmd |= PCI_COMMAND_MASTER;
> -        pci_writew(devfn, PCI_COMMAND, cmd);

The movement of this wants mentioning in the description.

> @@ -526,10 +538,17 @@ void pci_setup(void)
>           * has IO enabled, even if there is no I/O BAR on that
>           * particular device.
>           */
> -        cmd = pci_readw(vga_devfn, PCI_COMMAND);
> -        cmd |= PCI_COMMAND_IO;
> -        pci_writew(vga_devfn, PCI_COMMAND, cmd);
> +        pci_devfn_decode_type[vga_devfn] |= PCI_COMMAND_IO;
>      }
> +
> +    /* Enable bus master, memory and I/O decode. */
> +    for ( devfn = 0; devfn < 256; devfn++ )
> +        if ( pci_devfn_decode_type[devfn] )
> +        {
> +            cmd = pci_readw(devfn, PCI_COMMAND);
> +            cmd |= (PCI_COMMAND_MASTER | pci_devfn_decode_type[devfn]);
> +            pci_writew(devfn, PCI_COMMAND, cmd);
> +        }

This still regresses the setting of MASTER afaict: You only set
that bit now if either IO or MEMORY would also get set. But be
sure to honor the original code not doing the write when vendor/
device IDs are all ones.

Jan