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

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

On Wed, Jun 11, 2025 at 07:26:06PM +0200, Anthony PERARD wrote:
> On Tue, Jun 10, 2025 at 06:29:30PM +0200, Roger Pau Monne wrote:
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index c388899306c2..ddbff6fffc16 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2351,6 +2351,14 @@ Windows 
> > L<https://xenproject.org/windows-pv-drivers/>.
> >  Setting B<xen_platform_pci=0> with the default device_model "qemu-xen"
> >  requires at least QEMU 1.6.
> >  
> > +
> > +=item B<xenpci_bar_uc=BOOLEAN>
> > +
> > +B<x86 only:> Select whether the memory BAR of the Xen PCI device should 
> > have
> > +uncacheable (UC) cache attribute set in MTRR.
> 
> For information, here are different name used for this pci device:
> 
> - man xl.cfg:
>     xen_platform_pci=<bool>
>         Xen platform PCI device
> - QEMU:
>     -device xen-platform
>     in comments: XEN platform pci device
>     with pci device-id PCI_DEVICE_ID_XEN_PLATFORM
> - EDK2 / OVMF:
>     XenIoPci
>         described virtual Xen PCI device
>         But XenIo is a generic protocol in EDK2
>     Before XenIo, the pci device would be linked to XenBus, and
>     loaded with PCI_DEVICE_ID_XEN_PLATFORM
> - Linux:
>     Seems to be called "xen-platform-pci"
> 
> Overall, this PCI device is mostly referenced as the Xen Platform PCI
> device. So "xenpci" or "Xen PCI device" is surprising to me, and I'm not
> quite sure what it is.

I can do xen_platform_pci_bar_uc, but it seems a bit long.

> 
> > +
> > +Default is B<true>.
> > +
> >  =item B<viridian=[ "GROUP", "GROUP", ...]> or B<viridian=BOOLEAN>
> >  
> >  The groups of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> > diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> > index cc67b18c0361..cfd39cc37cdc 100644
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -116,6 +116,8 @@ void pci_setup(void)
> >       * experience the memory relocation bug described below.
> >       */
> >      bool allow_memory_relocate = 1;
> > +    /* Select the MTRR cache attribute of the xenpci device BAR. */
> > +    bool xenpci_bar_uc = false;
> 
> This default value for `xenpci_bar_uc` mean that hvmloader changes
> behavior compared to previous version, right? Shouldn't we instead have
> hvmloader keep the same behavior unless the toolstack want to use the
> new behavior? (Like it's done for `allow_memory_relocate`,
> "platform/mmio_hole_size")
> 
> It would just mean that toolstack other than `xl` won't be surprised by
> a change of behavior.

My plan was that we didn't need changes to XAPI to implement this new
mode, but given the comment I will change to keep the previous
behavior in absence of a xenstore node.

> 
> >      BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
> >                   PCI_COMMAND_IO);
> > @@ -130,6 +132,12 @@ void pci_setup(void)
> >      printf("Relocating guest memory for lowmem MMIO space %s\n",
> >             allow_memory_relocate?"enabled":"disabled");
> >  
> > +    s = xenstore_read(HVM_XS_XENPCI_BAR_UC, NULL);
> > +    if ( s )
> > +        xenpci_bar_uc = strtoll(s, NULL, 0);
> > +    printf("XenPCI device BAR MTRR cache attribute set to %s\n",
> > +           xenpci_bar_uc ? "UC" : "WB");
> > +
> >      s = xenstore_read("platform/mmio_hole_size", NULL);
> >      if ( s )
> >          mmio_hole_size = strtoll(s, NULL, 0);
> > @@ -271,6 +279,44 @@ void pci_setup(void)
> >              if ( bar_sz == 0 )
> >                  continue;
> >  
> > +            if ( !xenpci_bar_uc &&
> > +                 ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
> > +                   PCI_BASE_ADDRESS_SPACE_MEMORY) &&
> > +                 vendor_id == 0x5853 &&
> > +                 (device_id == 0x0001 || device_id == 0x0002) )
> 
> We don't have defines for 0x5853 in the tree (and those device_id)?
> Maybe introduce at least one for the vendor_id:
> 
> These two names are use by QEMU, OVMF, Linux, for example.
> 
> #define PCI_VENDOR_ID_XEN           0x5853
> #define PCI_DEVICE_ID_XEN_PLATFORM  0x0001
> 
> There's even PCI_DEVICE_ID_XEN_PLATFORM_XS61 in Linux

You mean in the public headers?

For Device IDs we have ranges allocated to downstream vendors, I'm not
sure we want to keep track of whatever IDs they use for their devices.
OTOH, not tracking those IDs here means OSes are likely to miss
additions of new Xen platform PCI devices?

I could introduce public/pci.h to contain those IDs, but I would like
consensus on what should be there, otherwise this patch will get
stuck.

> 
> > diff --git a/tools/firmware/hvmloader/util.c 
> > b/tools/firmware/hvmloader/util.c
> > index 79c0e6bd4ad2..31b4411db7b4 100644
> > --- a/tools/firmware/hvmloader/util.c
> > +++ b/tools/firmware/hvmloader/util.c
> > @@ -867,7 +867,7 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> > *config,
> >          config->table_flags |= ACPI_HAS_HPET;
> >  
> >      config->pci_start = pci_mem_start;
> > -    config->pci_len = pci_mem_end - pci_mem_start;
> > +    config->pci_len = RESERVED_MEMBASE - pci_mem_start;
> >      if ( pci_hi_mem_end > pci_hi_mem_start )
> >      {
> >          config->pci_hi_start = pci_hi_mem_start;
> > diff --git a/tools/libs/light/libxl_create.c 
> > b/tools/libs/light/libxl_create.c
> > index 8bc768b5156c..962fa820faec 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -313,6 +313,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> >          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
> >          libxl_defbool_setdefault(&b_info->u.hvm.vkb_device,         true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
> > +        libxl_defbool_setdefault(&b_info->u.hvm.xenpci_bar_uc,      true);
> >          libxl_defbool_setdefault(&b_info->u.hvm.pirq,               false);
> >  
> >          libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
> > diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> > index 4d67b0d28294..60ec0354d19a 100644
> > --- a/tools/libs/light/libxl_dom.c
> > +++ b/tools/libs/light/libxl_dom.c
> > @@ -819,6 +819,15 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
> >              goto err;
> >      }
> >  
> > +    if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
> > +        libxl_defbool_val(info->u.hvm.xenpci_bar_uc)) {
> 
> I think this condition is wrong. You should always write the value of
> xenpci_bar_uc into xenstore, or only write it if a value have been
> selected. But I guess we already lost the information here about whether
> the value is the default or not, and it's probably not important, so I
> think you should always write the value.

Indeed, whether the value is the default or the user-selected one is
lost by the time we get here.

I would adjust according to the above comments, but I would suggest we
leave out the addition of the Xen platform PCI device IDs to a
separate patch, as I fear it will block the patch otherwise.

Thanks, Roger.