On Thu, Jun 12, 2025 at 04:56:17PM +0200, Roger Pau Monné wrote: > 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.
I don't think it matter much how long it is, it is just a word that is surly copy-past from the man page. What I think matter more is that it's descriptive enough and match the existing option name for the same device, which is "xen_platform_pci". > > > + /* 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. Why would guests created with XAPI get the new behavior, but guest created with libxl have to stick with the old one? I do like that there's an option for libxl to choose between the old and new behavior, and allow to revert in case someone got an issue for a particular guest, but otherwise, it is probably better to have the same default for both XAPI and libxl. > > > @@ -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. I guess, just start with adding the vendor_id define in this same file (pci.c), that would be a good start, it would give a name to an otherwise magic number. Reading `vendor_id == PCI_VENDOR_ID_XEN` is better than reading `vendor_id == 0x5853`. If for some reason, we want to use the value in a different part of the repo, we could introduce or edit a common header and move the define there. For the device ids, using a define is less of a need, we would at least know we have a condition on Xen specific PCI device. This patch is only about a single device, isn't speaking about ID of other device a bit out of scope? And anyway, there's already a document about those, that is "xen-pci-device-reservations.7.pod". Thanks, -- Anthony PERARD
