Re: [PATCH v2] x86/hvmloader: select xenpci MMIO BAR UC or WB MTRR cache attribute

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 04, 2025 at 12:33:55PM +0200, Jürgen Groß wrote:
> On 03.06.25 13:18, Roger Pau Monne wrote:
> > The Xen PCI device (vendor ID 0x5853) exposed to x86 HVM guests doesn't
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index cc67b18c0361..97933b44a82b 100644
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -30,7 +30,7 @@
> >   #include <xen/hvm/e820.h>
> >   uint32_t pci_mem_start = HVM_BELOW_4G_MMIO_START;
> > -const uint32_t pci_mem_end = RESERVED_MEMBASE;
> > +uint32_t pci_mem_end = RESERVED_MEMBASE;
> >   uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
> >   /*
> > @@ -116,6 +116,8 @@ void pci_setup(void)
> >        * experience the memory relocation bug described below.
> >        */
> >       bool allow_memory_relocate = 1;
> > +    /* Set the MTRR type of the xenpci device BAR as UC. */
> > +    bool xenpci_bar_uc = false;
> 
> Hmm, shouldn't this be "true"? Or should the comment be changed?

The intention of the comment was to signal that this variable is used
to set the BAR cache attributes to UC possibly, but I can see how
that's confusing.  What about using:

    /* Select the MTRR cache attributes of the xenpci device BAR. */
    bool xenpci_bar_uc = false;

Thanks, Roger.