On Thu, Jun 12, 2025 at 06:13:48PM +0200, Anthony PERARD wrote: > 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".
I've already adjusted everything to xen_platform_pci_bar_uc, plus the text to s/Xen PCI/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. OK, I think I've misunderstood from your previous reply that you wanted me to introduce a public pci.h header to contain those values. > 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". Yes, but if I had to add a new header I got the feeling I would get questions about which device IDs should be listed there. Thanks, Roger.
