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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Harsha 
> Shamsundara Havanur
> Sent: 15 April 2020 13:28
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Liu <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Ian 
> Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Harsha 
> Shamsundara Havanur
> <havanur@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: [XEN PATCH v5] hvmloader: Enable MMIO and I/O decode, after all 
> resource allocation
> 
> It was observed that PCI MMIO and/or IO BARs were programmed with
> memory and I/O decodes (bits 0 and 1 of PCI COMMAND register) enabled,
> during PCI setup phase. This resulted in incorrect memory mapping as
> soon as the lower half of the 64 bit bar is programmed.
> This displaced any RAM mappings under 4G. After the
> upper half is programmed PCI memory mapping is restored to its
> intended high mem location, but the RAM displaced is not restored.
> The OS then continues to boot and function until it tries to access
> the displaced RAM at which point it suffers a page fault and crashes.
> 
> This patch address the issue by deferring enablement of memory and
> I/O decode in command register until all the resources, like interrupts
> I/O and/or MMIO BARs for all the PCI device functions are programmed,
> in the descending order of memory requested.
> 
> Signed-off-by: Harsha Shamsundara Havanur <havanur@xxxxxxxxxx>
> 
> ---
> Changes in v5:
>   - Fix style and comment
>   - Enable Bus master for all valid device functions
> 
> Changes in v4:
>   Addressed review comments from Jan Beulich <jbeulich@xxxxxxxx>
>   - Disable decodes before writing ~0 to BARs
>   - Enable BUS MASTER at the end along with decode bits
> 
> Changes in v3:
>   - Retained enabling of BUS master for all device functions as
>     was in original commit
> 
> Changes in v2:
>   - BUS MASTER state was captured and restored to the same state
>     while enabling decode bits.
> ---
> ---
>  tools/firmware/hvmloader/pci.c | 49 
> +++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 0b708bf578..47cba69ce4 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -84,6 +84,7 @@ void pci_setup(void)
>      uint32_t vga_devfn = 256;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
> +    uint8_t pci_devfn_decode_type[256] = {};
> 
>      /* Resources assignable to PCI devices via BARs. */
>      struct resource {
> @@ -120,6 +121,13 @@ void pci_setup(void)
>       */
>      bool allow_memory_relocate = 1;
> 
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
> +            PCI_COMMAND_IO);
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMORY !=
> +            PCI_COMMAND_MEMORY);
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MASTER !=
> +            PCI_COMMAND_MASTER);
> +

These don't align. Looks like you used an indent of 8 which seems entirely 
arbitrary.

The rest LGTM.

  Paul